fix: Add property tag support for pom.xml (#172)

- Use @xml-tools/ast for pom.xml parsing (AST based, previous one is based on SAX parsing)
- Refactor pom collector into separate module

Author: arajkumar

README.md

@@ -2,7 +2,7 @@
 
 [![NPM Publish](https://ci.centos.org/job/devtools-fabric8-analytics-lsp-server-npm-publish-build-master/badge/icon)](https://ci.centos.org/job/devtools-fabric8-analytics-lsp-server-npm-publish-build-master/)
 [![NPM Version](https://img.shields.io/npm/v/fabric8-analytics-lsp-server.svg)](https://www.npmjs.com/package/fabric8-analytics-lsp-server)
-![CI Build](https://github.com/fabric8-analytics/fabric8-analytics-lsp-server/workflows/CI%20Build/badge.svg)
+![CI Build](https://github.com/fabric8-analytics/fabric8-analytics-lsp-server/workflows/CI%20Build/badge.svg?branch=master)
 [![codecov](https://codecov.io/gh/fabric8-analytics/fabric8-analytics-lsp-server/branch/master/graph/badge.svg?token=aVThXjheDf)](https://codecov.io/gh/fabric8-analytics/fabric8-analytics-lsp-server)
 
 Language Server(LSP) that can analyze your dependencies specified in `package.json` and `pom.xml`.

package-lock.json

@@ -30,12 +30,13 @@
     "url": "https://github.com/fabric8-analytics/fabric8-analytics-lsp-server.git"
   },
   "dependencies": {
+    "@xml-tools/ast": "^5.0.0",
+    "@xml-tools/parser": "^1.0.7",
+    "compare-versions": "3.6.0",
     "json-to-ast": "^2.1.0",
     "node-fetch": "^2.6.0",
     "vscode-languageserver": "^5.3.0-next.9",
-    "winston": "3.2.1",
-    "xml2object": "0.1.2",
-    "compare-versions": "3.6.0"
+    "winston": "3.2.1"
   },
   "devDependencies": {
     "@semantic-release/exec": "^5.0.0",

package.json

@@ -4,12 +4,13 @@
  * ------------------------------------------------------------------------------------------ */
 'use strict';
 import { Stream } from 'stream';
-import * as Xml2Object from 'xml2object';
 import * as jsonAst from 'json-to-ast';
-import { IPosition, IKeyValueEntry, KeyValueEntry, Variant, ValueType } from './types';
+import { IPosition, IKeyValueEntry, KeyValueEntry, Variant, ValueType, IDependency, IPositionedString, IDependencyCollector, Dependency } from './types';
 import { stream_from_string, getGoLangImportsCmd } from './utils';
 import { config } from './config';
 import { exec } from 'child_process';
+import { parse, DocumentCstNode } from "@xml-tools/parser";
+import { buildAst, accept, XMLElement, XMLDocument } from "@xml-tools/ast";
 
 /* Please note :: There was issue with semverRegex usage in the code. During run time, it extracts 
  * version with 'v' prefix, but this is not be behavior of semver in CLI and test environment. 
@@ -20,40 +21,6 @@ function semVerRegExp(line: string): RegExpExecArray {
     return regExp.exec(line);
 }
 
-/* String value with position */
-interface IPositionedString {
-  value:    string;
-  position: IPosition;
-}
-
-/* Dependency specification */
-interface IDependency {
-  name:    IPositionedString;
-  version: IPositionedString;
-}
-
-/* Dependency collector interface */
-interface IDependencyCollector {
-  classes: Array<string>;
-  collect(contents: string): Promise<Array<IDependency>>;
-}
-
-/* Dependency class that can be created from `IKeyValueEntry` */
-class Dependency implements IDependency {
-  name:    IPositionedString;
-  version: IPositionedString;
-  constructor(dependency: IKeyValueEntry) {
-    this.name = {
-        value: dependency.key,
-        position: dependency.key_position
-    };
-    this.version = {
-        value: dependency.value.object,
-        position: dependency.value_position
-    };
-  }
-}
-
 class NaivePyParser {
     constructor(contents: string) {
         this.dependencies = NaivePyParser.parseDependencies(contents);
@@ -245,87 +212,6 @@ class GomodDependencyCollector implements IDependencyCollector {
 
 }
 
-class NaivePomXmlSaxParser {
-    constructor(stream: Stream) {
-        this.stream = stream;
-        this.parser = this.createParser();
-    }
-
-    stream: Stream;
-    parser: Xml2Object;
-    dependencies: Array<IDependency> = [];
-    isDependency: boolean = false;
-    versionStartLine: number = 0;
-    versionStartColumn: number = 0;
-
-    createParser(): Xml2Object {
-        let parser = new Xml2Object([ "dependency" ], {strict: true, trackPosition: true});
-        let deps = this.dependencies;
-        let versionLine = this.versionStartLine;
-        let versionColumn = this.versionStartColumn;
-
-        parser.on("object", function (name, obj) {
-            if (obj.hasOwnProperty("groupId") && obj.hasOwnProperty("artifactId") && obj.hasOwnProperty("version") && 
-                (!obj.hasOwnProperty("scope") || (obj.hasOwnProperty("scope") && obj["scope"] != "test"))) {
-                let ga = `${obj["groupId"]}:${obj["artifactId"]}`;
-                let entry: IKeyValueEntry = new KeyValueEntry(ga, {line: 0, column: 0});
-                entry.value = new Variant(ValueType.String, obj["version"]);
-                entry.value_position = {line: versionLine, column: versionColumn};
-                let dep: IDependency = new Dependency(entry);
-                deps.push(dep)
-            }
-        });
-        parser.saxStream.on("opentag", function (node) {
-            if (node.name == "dependency") {
-                this.isDependency = true;
-            }
-            if (this.isDependency && node.name == "version") {
-                versionLine = parser.saxStream._parser.line + 1;
-                versionColumn = parser.saxStream._parser.column +1;
-            }
-        });
-        parser.saxStream.on("closetag", function (nodeName) {
-            // TODO: nested deps!
-            if (nodeName == "dependency") {
-                this.isDependency = false;
-            }
-        });
-        parser.on("error", function (e) {
-            // the XML document doesn't have to be well-formed, that's fine
-            parser.error = null;
-        });
-        parser.on("end", function () {
-            // the XML document doesn't have to be well-formed, that's fine
-            // parser.error = null;
-            this.dependencies = deps;
-        });
-        return parser
-    }
-
-    async parse() {
-        return new Promise(resolve => {
-            this.stream.pipe(this.parser.saxStream).on('end', (data) => {
-                resolve(this.dependencies);
-           });
-        });
-
-    }
-}
-
-class PomXmlDependencyCollector implements IDependencyCollector {
-    constructor(public classes: Array<string> = ["dependencies"]) {}
-
-    async collect(contents: string): Promise<Array<IDependency>> {
-        const file = stream_from_string(contents);
-        let parser = new NaivePomXmlSaxParser(file);
-        let dependencies;
-         await parser.parse().then(data => {
-            dependencies = data;
-        });
-        return dependencies || [];
-    }
-}
-
 class PackageJsonCollector implements IDependencyCollector {
     constructor(public classes: Array<string> = ["dependencies"]) {}
 
@@ -343,4 +229,4 @@ class PackageJsonCollector implements IDependencyCollector {
     }
 }
 
-export { IDependencyCollector, PackageJsonCollector, PomXmlDependencyCollector, ReqDependencyCollector, GomodDependencyCollector, IPositionedString, IDependency };
+export { IDependencyCollector, PackageJsonCollector, ReqDependencyCollector, GomodDependencyCollector, IPositionedString, IDependency };

src/collector.ts

@@ -0,0 +1,100 @@
+'use strict';
+import { IKeyValueEntry, KeyValueEntry, Variant, ValueType, IDependency, IDependencyCollector, Dependency, IPositionedString, IPosition } from './types';
+import { parse, DocumentCstNode } from "@xml-tools/parser";
+import { buildAst, accept, XMLElement, XMLDocument } from "@xml-tools/ast";
+
+export class PomXmlDependencyCollector implements IDependencyCollector {
+    private xmlDocAst: XMLDocument;
+
+    constructor(public classes: Array<string> = ["dependencies"]) {}
+
+    private findRootNodes(rootElementName: string): Array<XMLElement> {
+        const properties: Array<XMLElement> = [];
+        const propertiesElement = {
+            // Will be invoked once for each Element node in the AST.
+            visitXMLElement: (node: XMLElement) => {
+                if (node.name === rootElementName) {
+                    properties.push(node);
+                }
+            },
+        };
+        accept(this.xmlDocAst, propertiesElement);
+        return properties;
+    }
+
+    private parseXml(contents: string): void {
+        const { cst, tokenVector } = parse(contents);
+        this.xmlDocAst = buildAst(cst as DocumentCstNode, tokenVector);
+    }
+
+    private mapToDependency(dependenciesNode: XMLElement): Array<IDependency> {
+        class PomDependency {
+            public groupId: XMLElement;
+            public artifactId: XMLElement;
+            public version: XMLElement;
+            constructor(e: XMLElement) {
+                this.groupId = e.subElements.find(e => e.name === 'groupId');
+                this.artifactId = e.subElements.find(e => e.name === 'artifactId');
+                this.version = e.subElements.find(e => e.name === 'version');
+            }
+
+            isValid(): boolean {
+                // none should have a empty text.
+                return [this.groupId, this.artifactId, this.version].find(e => !e.textContents[0]?.text) === undefined;
+            }
+
+            toDependency(): Dependency {
+                const dep: IKeyValueEntry = new KeyValueEntry(`${this.groupId.textContents[0].text}:${this.artifactId.textContents[0].text}`, {line: 0, column: 0});
+                const versionVal = this.version.textContents[0];
+                dep.value = new Variant(ValueType.String, versionVal.text);
+                dep.value_position = {line: versionVal.position.startLine, column: versionVal.position.startColumn};
+                return new Dependency(dep);
+            }
+        };
+        const validElementNames = ['groupId', 'artifactId', 'version'];
+        const dependencies = dependenciesNode?.
+            subElements.
+            filter(e => e.name === 'dependency').
+            // must include all validElementNames
+            filter(e => e.subElements.filter(e => validElementNames.includes(e.name)).length == validElementNames.length).
+            // no test dependencies
+            filter(e => !e.subElements.find(e => (e.name === 'scope' && e.textContents[0].text === 'test'))).
+            map(e => new PomDependency(e)).
+            filter(d => d.isValid()).
+            map(d => d.toDependency());
+        return dependencies || [];
+    }
+
+    private createPropertySubstitution(e: XMLElement): Map<string, IPositionedString> {
+        return new Map(e?.subElements?.
+            filter(e => e.textContents[0]?.text).
+            map(e => {
+                const propertyValue = e.textContents[0];
+                const position: IPosition = {line: propertyValue.position.startLine, column: propertyValue.position.startColumn};
+                const value = {value: propertyValue.text, position: position} as IPositionedString;
+                // key should be equivalent to pom.xml property format. i.e ${property.value}
+                return [`\$\{${e.name}\}`, value];
+        }));
+    }
+
+    private applyProperty(dependency: IDependency, map: Map<string, IPositionedString>): IDependency {
+        // FIXME: Do the groupId and artifactId will also be expressed through properties?
+        dependency.version = map.get(dependency.version.value) ?? dependency.version;
+        return dependency; 
+    }
+
+    async collect(contents: string): Promise<Array<IDependency>> {
+        this.parseXml(contents);
+        const deps = this.findRootNodes("dependencies");
+        // lazy eval
+        const getPropertyMap = (() => {
+            let propertyMap = null;
+            return () => {
+                propertyMap = propertyMap ?? this.createPropertySubstitution(this.findRootNodes("properties")[0]);
+                return propertyMap;
+            };
+        })();
+
+        return deps.flatMap(dep => this.mapToDependency(dep)).map(d => this.applyProperty(d, getPropertyMap()));
+    }
+}

src/maven.collector.ts

@@ -8,7 +8,8 @@ import * as fs from 'fs';
 import {
 	IPCMessageReader, IPCMessageWriter, createConnection, IConnection,
 	TextDocuments, InitializeResult, CodeLens, CodeAction, CodeActionKind} from 'vscode-languageserver';
-import { IDependencyCollector, PackageJsonCollector, PomXmlDependencyCollector, ReqDependencyCollector, GomodDependencyCollector } from './collector';
+import { IDependencyCollector, PackageJsonCollector, ReqDependencyCollector, GomodDependencyCollector } from './collector';
+import { PomXmlDependencyCollector } from './maven.collector';
 import { SecurityEngine, DiagnosticsPipeline, codeActionsMap } from './consumers';
 import { NoopVulnerabilityAggregator, GolangVulnerabilityAggregator } from './aggregators';
 import { AnalyticsSource } from './vulnerability';
@@ -250,11 +251,14 @@ const sendDiagnostics = async (ecosystem: string, diagnosticFilePath: string, co
     connection.sendNotification('caNotification', {data: caDefaultMsg, done: false, uri: diagnosticFilePath});
     let deps = null;
     try {
+        const start = new Date().getTime();
         deps = await collector.collect(contents);
+        const end = new Date().getTime();
+        connection.console.log(`manifest parse took ${end - start} ms`);
     } catch (error) {
         // Error can be raised during golang `go list ` command only.
         if (ecosystem == "golang") {
-            connection.console.error(`Command execution failed with error: ${error}`);
+            connection.console.warn(`Command execution failed with error: ${error}`);
             connection.sendNotification('caError', {data: error, uri: diagnosticFilePath});
             connection.sendDiagnostics({ uri: diagnosticFilePath, diagnostics: [] });
             return;
@@ -282,7 +286,7 @@ const sendDiagnostics = async (ecosystem: string, diagnosticFilePath: string, co
     await Promise.allSettled(allRequests);
     const end = new Date().getTime();
 
-    connection.console.log('Time taken to fetch vulnerabilities: ' + ((end - start) / 1000).toFixed(1) + ' sec.');
+    connection.console.log(`fetch vulns took ${end - start} ms`);
     connection.sendNotification('caNotification', {data: getCAmsg(deps, diagnostics, totalCount), diagCount : diagnostics.length || 0, vulnCount: totalCount, depCount: deps.length || 0, done: true, uri: diagnosticFilePath});
 };
 

src/server.ts

@@ -53,6 +53,38 @@ class Variant implements IVariant {
     constructor(public type: ValueType, public object: any) {}
 }
 
+/* String value with position */
+interface IPositionedString {
+  value:    string;
+  position: IPosition;
+}
+
+/* Dependency specification */
+interface IDependency {
+  name:    IPositionedString;
+  version: IPositionedString;
+}
 
-export { IPosition, IKeyValueEntry, KeyValueEntry, Variant, ValueType };
+/* Dependency collector interface */
+interface IDependencyCollector {
+  classes: Array<string>;
+  collect(contents: string): Promise<Array<IDependency>>;
+}
+
+/* Dependency class that can be created from `IKeyValueEntry` */
+class Dependency implements IDependency {
+  name:    IPositionedString;
+  version: IPositionedString;
+  constructor(dependency: IKeyValueEntry) {
+    this.name = {
+        value: dependency.key,
+        position: dependency.key_position
+    };
+    this.version = {
+        value: dependency.value.object,
+        position: dependency.value_position
+    };
+  }
+}
 
+export { IPosition, IKeyValueEntry, KeyValueEntry, Variant, ValueType, IDependency, IPositionedString, IDependencyCollector, Dependency };