Code review changes part 2

index.js

@@ -14,6 +14,15 @@ module.exports = {
 		Openui5Resolver: require("./lib/ui5Framework/Openui5Resolver"),
 		Sapui5Resolver: require("./lib/ui5Framework/Sapui5Resolver")
 	},
+	/**
+	 * @public
+	 * @see module:@ui5/project.validation
+	 * @namespace
+	 */
+	validation: {
+		validator: require("./lib/validation/validator"),
+		ValidationError: require("./lib/validation/ValidationError")
+	},
 	/**
 	 * @private
 	 * @see module:@ui5/project.translators

lib/projectPreprocessor.js

@@ -201,9 +201,8 @@ class ProjectPreprocessor {
 				return {};
 			}
 
-			await this.validateExistingProject(project);
+			await this.validateAndNormalizeExistingProject(project);
 
-			this.normalizeConfig(project);
 			return {};
 		}
 
@@ -538,7 +537,7 @@ class ProjectPreprocessor {
 			Object.assign(project, configShim);
 			delete project.shimDependenciesResolved; // Remove shim processing metadata from project
 
-			await this.validateExistingProject(project);
+			await this.validateAndNormalizeExistingProject(project);
 		}
 
 		// Apply collections
@@ -597,7 +596,7 @@ class ProjectPreprocessor {
 		middlewareRepository.addMiddleware(extension.metadata.name, middlewarePath);
 	}
 
-	async validateExistingProject(project) {
+	async validateAndNormalizeExistingProject(project) {
 		// Validate project config, but exclude additional properties
 		const excludedProperties = [
 			"id",
@@ -618,6 +617,8 @@ class ProjectPreprocessor {
 				id: project.id
 			}
 		});
+
+		this.normalizeConfig(project);
 	}
 }
 

lib/validation/ValidationError.js

@@ -2,18 +2,39 @@ const chalk = require("chalk");
 const escapeStringRegExp = require("escape-string-regexp");
 const matchAll = require("string.prototype.matchall");
 
+/**
+ * Error class for validation of project configuration.
+ *
+ * @public
+ * @hideconstructor
+ * @extends Error
+ * @memberof module:@ui5/project.validation
+ */
 class ValidationError extends Error {
-	constructor({errors, schema, data, project, yaml}) {
+	constructor({errors, project, yaml}) {
 		super();
 
+		/**
+		 * ValidationError
+		 *
+		 * @const
+		 * @readonly
+		 * @public
+		 */
 		this.name = "ValidationError";
 
-		this.schema = schema;
-		this.data = data;
 		this.project = project;
 		this.yaml = yaml;
+
 		this.errors = ValidationError.filterErrors(errors);
 
+		/**
+		 * Error message
+		 *
+		 * @type {string}
+		 * @readonly
+		 * @public
+		 */
 		this.message = this.formatErrors();
 
 		Error.captureStackTrace(this, this.constructor);
@@ -22,6 +43,7 @@ class ValidationError extends Error {
 	formatErrors() {
 		let separator = "\n\n";
 		if (process.stdout.isTTY) {
+			// Add a horizontal separator line between errors in case a terminal is used
 			separator += chalk.grey.dim("\u2500".repeat(process.stdout.columns || 80));
 		}
 		separator += "\n\n";
@@ -53,15 +75,19 @@ class ValidationError extends Error {
 			message += chalk.underline(chalk.red(error.dataPath.substr(1))) + " ";
 		}
 
-		if (error.keyword === "additionalProperties") {
+		switch (error.keyword) {
+		case "additionalProperties":
 			message += `property ${error.params.additionalProperty} is not expected to be here`;
-		} else if (error.keyword === "required") {
-			message += error.message;
-		} else if (error.keyword === "type") {
+			break;
+		case "type":
 			message += `should be of type '${error.params.type}'`;
-		} else if (error.keyword === "enum") {
+			break;
+		case "enum":
 			message += error.message + "\n";
 			message += "Allowed values: " + error.params.allowedValues.join(", ");
+			break;
+		default:
+			message += error.message;
 		}
 
 		return message;
@@ -94,6 +120,7 @@ class ValidationError extends Error {
 
 	static analyzeYamlError({error, yaml}) {
 		if (error.dataPath === "" && error.keyword === "required") {
+			// There is no line/column for a missing required property on root level
 			return {line: -1, column: -1};
 		}
 
@@ -104,27 +131,38 @@ class ValidationError extends Error {
 			objectPath.push(error.params.additionalProperty);
 		}
 
+		const matchArrayElement = /(^|\n)([ ]*-[^\n]*)/g;
+		const matchArrayElementIndentation = /([ ]*)-/;
+
 		let currentIndex = 0;
 		let currentSubstring = yaml.source;
 		for (let i = 0; i < objectPath.length; i++) {
 			const property = objectPath[i];
 			let newIndex;
 
 			if (isNaN(property)) {
+				// Try to find a property
+
+				// Creating a regular expression that matches the property name a line
+				// except for comments, indicated by a hash sign "#".
 				const propertyRegExp = new RegExp(`^[^#]*?${escapeStringRegExp(property)}`, "m");
+
 				const propertyMatch = propertyRegExp.exec(currentSubstring);
 				if (!propertyMatch) {
 					return {line: -1, column: -1};
 				}
 				newIndex = propertyMatch.index + propertyMatch[0].length;
 			} else {
+				// Try to find the right index within an array definition.
+				// This currently only works for arrays defined with "-" in multiple lines.
+				// Arrays using square brackets are not supported.
+
 				const arrayIndex = parseInt(property);
-				const matchArrayElement = /(^|\n)([ ]*-[^\n]*)/g;
 				const arrayIndicators = matchAll(currentSubstring, matchArrayElement);
 				let a = 0;
 				let firstIndentation = -1;
 				for (const match of arrayIndicators) {
-					const indentationMatch = match[2].match(/([ ]*)-/);
+					const indentationMatch = match[2].match(matchArrayElementIndentation);
 					if (!indentationMatch) {
 						return {line: -1, column: -1};
 					}

lib/validation/schema/specVersion/2.0/kind/extension.json

@@ -23,7 +23,8 @@
 	"if": {
 		"properties": {
 			"type": {"const": null}
-		}
+		},
+		"$comment": "Using 'if' with null and empty 'then' to ensure no other schemas are applied when the property is not set. Otherwise the first 'if' condition might still be met, causing unexpected errors."
 	},
 	"then": {},
 	"else": {
@@ -52,16 +53,6 @@
 				},
 				"then": {
 					"$ref": "extension/project-shim.json"
-				},
-				"else": {
-					"if": {
-						"properties": {
-							"type": {"const": null}
-						}
-					},
-					"then": {
-
-					}
 				}
 			}
 		}

lib/validation/schema/specVersion/2.0/kind/project.json

@@ -25,7 +25,8 @@
 	"if": {
 		"properties": {
 			"type": {"const": null}
-		}
+		},
+		"$comment": "Using 'if' with null and empty 'then' to ensure no other schemas are applied when the property is not set. Otherwise the first 'if' condition might still be met, causing unexpected errors."
 	},
 	"then": {},
 	"else": {

lib/validation/validator.js

@@ -12,6 +12,10 @@ async function loadSchema(schemaPath) {
 	return JSON.parse(schemaFile);
 }
 
+/**
+ * @private
+ * @memberof module:@ui5/project.validation
+ */
 class Validator {
 	constructor() {
 		this.ajv = new Ajv({
@@ -40,7 +44,6 @@ class Validator {
 			throw new ValidationError({
 				errors: fnValidate.errors,
 				schema: fnValidate.schema,
-				data: config,
 				project,
 				yaml
 			});
@@ -52,10 +55,30 @@ class Validator {
 
 const validator = new Validator();
 
+/**
+ * @public
+ * @namespace
+ * @alias module:@ui5/project.validation.validator
+ */
 module.exports = {
+	/**
+	 * Validates the given configuration.
+	 *
+	 * @param {Object} options
+	 * @param {Object} options.config UI5 Configuration to validate
+	 * @param {Object} options.project Project information
+	 * @param {string} options.project.id ID of the project
+	 * @param {Object} [options.yaml] YAML information
+	 * @param {string} options.yaml.path Path of the YAML file
+	 * @param {string} options.yaml.source Content of the YAML file
+	 * @param {number} [options.yaml.documentIndex=0] Document index in case the YAML file contains multiple documents
+	 * @throws {module:@ui5/project.validation.ValidationError}
+	 *   Throws a {@link module:@ui5/project.validation.ValidationError ValidationError} when the validation fails.
+	 * @returns {undefined} Returns when the validation succeeds
+	 * @public
+	 */
 	validate: (options) => {
 		return validator.validate(options);
 	},
-	ValidationError,
 	_Validator: Validator // For testing only
 };