Add validation for objects, fixes #128429 (#129066)

* Add validation for objects, fixes #128429

src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts

@@ -123,6 +123,7 @@ export class SettingsEditor2 extends EditorPane {
 		return type === SettingValueType.Enum ||
 			type === SettingValueType.StringOrEnumArray ||
 			type === SettingValueType.BooleanObject ||
+			type === SettingValueType.Object ||
 			type === SettingValueType.Complex ||
 			type === SettingValueType.Boolean ||
 			type === SettingValueType.Exclude;

src/vs/workbench/contrib/preferences/browser/settingsTree.ts

@@ -514,9 +514,10 @@ interface ISettingExcludeItemTemplate extends ISettingItemTemplate<void> {
 	excludeWidget: ListSettingWidget;
 }
 
-interface ISettingObjectItemTemplate extends ISettingItemTemplate<void> {
+interface ISettingObjectItemTemplate extends ISettingItemTemplate<Record<string, unknown> | undefined> {
 	objectDropdownWidget?: ObjectSettingDropdownWidget,
 	objectCheckboxWidget?: ObjectSettingCheckboxWidget;
+	validationErrorMessageElement: HTMLElement;
 }
 
 interface ISettingNewExtensionsTemplate extends IDisposableTemplate {
@@ -1140,8 +1141,13 @@ abstract class AbstractSettingObjectRenderer extends AbstractSettingRenderer imp
 		widget.domNode.classList.add(AbstractSettingRenderer.CONTROL_CLASS);
 		common.toDispose.add(widget);
 
+		const descriptionElement = common.containerElement.querySelector('.setting-item-description')!;
+		const validationErrorMessageElement = $('.setting-item-validation-message');
+		descriptionElement.after(validationErrorMessageElement);
+
 		const template: ISettingObjectItemTemplate = {
-			...common
+			...common,
+			validationErrorMessageElement
 		};
 		if (widget instanceof ObjectSettingCheckboxWidget) {
 			template.objectCheckboxWidget = widget;
@@ -1151,7 +1157,9 @@ abstract class AbstractSettingObjectRenderer extends AbstractSettingRenderer imp
 
 		this.addSettingElementFocusHandler(template);
 
-		common.toDispose.add(widget.onDidChangeList(e => this.onDidChangeObject(template, e)));
+		common.toDispose.add(widget.onDidChangeList(e => {
+			this.onDidChangeObject(template, e);
+		}));
 
 		return template;
 	}
@@ -1210,17 +1218,17 @@ abstract class AbstractSettingObjectRenderer extends AbstractSettingRenderer imp
 				}
 			});
 
-			this._onDidChangeSetting.fire({
-				key: template.context.setting.key,
-				value: Object.keys(newValue).length === 0 ? undefined : newValue,
-				type: template.context.valueType
-			});
+			const newObject = Object.keys(newValue).length === 0 ? undefined : newValue;
 
 			if (template.objectCheckboxWidget) {
 				template.objectCheckboxWidget.setValue(newItems);
 			} else {
 				template.objectDropdownWidget!.setValue(newItems);
 			}
+
+			if (template.onChange) {
+				template.onChange(newObject);
+			}
 		}
 	}
 
@@ -1238,7 +1246,7 @@ export class SettingObjectRenderer extends AbstractSettingObjectRenderer impleme
 		return this.renderTemplateWithWidget(common, widget);
 	}
 
-	protected renderValue(dataElement: SettingsTreeSettingElement, template: ISettingObjectItemTemplate, onChange: (value: string) => void): void {
+	protected renderValue(dataElement: SettingsTreeSettingElement, template: ISettingObjectItemTemplate, onChange: (value: Record<string, unknown> | undefined) => void): void {
 		const items = getObjectDisplayValue(dataElement);
 		const { key, objectProperties, objectPatternProperties, objectAdditionalProperties } = dataElement.setting;
 
@@ -1255,6 +1263,11 @@ export class SettingObjectRenderer extends AbstractSettingObjectRenderer impleme
 		});
 
 		template.context = dataElement;
+		template.onChange = (v: Record<string, unknown> | undefined) => {
+			onChange(v);
+			renderArrayValidations(dataElement, template, v, false);
+		};
+		renderArrayValidations(dataElement, template, dataElement.value, true);
 	}
 }
 
@@ -1278,7 +1291,7 @@ export class SettingBoolObjectRenderer extends AbstractSettingObjectRenderer imp
 		}
 	}
 
-	protected renderValue(dataElement: SettingsTreeSettingElement, template: ISettingObjectItemTemplate, onChange: (value: string) => void): void {
+	protected renderValue(dataElement: SettingsTreeSettingElement, template: ISettingObjectItemTemplate, onChange: (value: Record<string, unknown> | undefined) => void): void {
 		const items = getObjectDisplayValue(dataElement);
 		const { key } = dataElement.setting;
 
@@ -1287,6 +1300,9 @@ export class SettingBoolObjectRenderer extends AbstractSettingObjectRenderer imp
 		});
 
 		template.context = dataElement;
+		template.onChange = (v: Record<string, unknown> | undefined) => {
+			onChange(v);
+		};
 	}
 }
 
@@ -1895,8 +1911,8 @@ function renderValidations(dataElement: SettingsTreeSettingElement, template: IS
 
 function renderArrayValidations(
 	dataElement: SettingsTreeSettingElement,
-	template: ISettingListItemTemplate,
-	value: string[] | undefined,
+	template: ISettingListItemTemplate | ISettingObjectItemTemplate,
+	value: string[] | Record<string, unknown> | undefined,
 	calledOnStartup: boolean
 ) {
 	template.containerElement.classList.add('invalid-input');

src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts

@@ -597,7 +597,7 @@ function isObjectSetting({
 	}
 
 	// object additional properties allow it to have any shape
-	if (objectAdditionalProperties === true) {
+	if (objectAdditionalProperties === true || objectAdditionalProperties === undefined) {
 		return false;
 	}
 

src/vs/workbench/services/preferences/common/preferencesValidation.ts

@@ -6,7 +6,7 @@
 import * as nls from 'vs/nls';
 import { JSONSchemaType } from 'vs/base/common/jsonSchema';
 import { Color } from 'vs/base/common/color';
-import { isArray } from 'vs/base/common/types';
+import { isArray, isObject, isUndefinedOrNull, isString, isStringArray } from 'vs/base/common/types';
 import { IConfigurationPropertySchema } from 'vs/platform/configuration/common/configurationRegistry';
 
 type Validator<T> = { enabled: boolean, isValid: (value: T) => boolean; message: string };
@@ -15,18 +15,22 @@ function canBeType(propTypes: (string | undefined)[], ...types: JSONSchemaType[]
 	return types.some(t => propTypes.includes(t));
 }
 
+function isNullOrEmpty(value: unknown): boolean {
+	return value === '' || isUndefinedOrNull(value);
+}
+
 export function createValidator(prop: IConfigurationPropertySchema): (value: any) => (string | null) {
-	const type: (string | undefined)[] = Array.isArray(prop.type) ? prop.type : [prop.type];
+	const type: (string | undefined)[] = isArray(prop.type) ? prop.type : [prop.type];
 	const isNullable = canBeType(type, 'null');
 	const isNumeric = (canBeType(type, 'number') || canBeType(type, 'integer')) && (type.length === 1 || type.length === 2 && isNullable);
 
 	const numericValidations = getNumericValidators(prop);
 	const stringValidations = getStringValidators(prop);
 	const stringArrayValidator = getArrayOfStringValidator(prop);
+	const objectValidator = getObjectValidator(prop);
 
 	return value => {
-		if (prop.type === 'string' && stringValidations.length === 0) { return null; }
-		if (isNullable && value === '') { return ''; }
+		if (isNullable && isNullOrEmpty(value)) { return ''; }
 
 		const errors: string[] = [];
 		if (stringArrayValidator) {
@@ -36,8 +40,15 @@ export function createValidator(prop: IConfigurationPropertySchema): (value: any
 			}
 		}
 
+		if (objectValidator) {
+			const err = objectValidator(value);
+			if (err) {
+				errors.push(err);
+			}
+		}
+
 		if (isNumeric) {
-			if (value === '' || isNaN(+value)) {
+			if (isNullOrEmpty(value) || isNaN(+value)) {
 				errors.push(nls.localize('validations.expectedNumeric', "Value must be a number."));
 			} else {
 				errors.push(...numericValidations.filter(validator => !validator.isValid(+value)).map(validator => validator.message));
@@ -45,7 +56,11 @@ export function createValidator(prop: IConfigurationPropertySchema): (value: any
 		}
 
 		if (prop.type === 'string') {
-			errors.push(...stringValidations.filter(validator => !validator.isValid('' + value)).map(validator => validator.message));
+			if (!isString(value)) {
+				errors.push(nls.localize('validations.incorrectType', 'Incorrect type. Expected "string".'));
+			} else {
+				errors.push(...stringValidations.filter(validator => !validator.isValid(value)).map(validator => validator.message));
+			}
 		}
 
 		if (errors.length) {
@@ -64,7 +79,7 @@ export function getInvalidTypeError(value: any, type: undefined | string | strin
 		return;
 	}
 
-	const typeArr = Array.isArray(type) ? type : [type];
+	const typeArr = isArray(type) ? type : [type];
 	if (!typeArr.some(_type => valueValidatesAsType(value, _type))) {
 		return nls.localize('invalidTypeError', "Setting has an invalid type, expected {0}. Fix in JSON.", JSON.stringify(type));
 	}
@@ -77,11 +92,11 @@ function valueValidatesAsType(value: any, type: string): boolean {
 	if (type === 'boolean') {
 		return valueType === 'boolean';
 	} else if (type === 'object') {
-		return value && !Array.isArray(value) && valueType === 'object';
+		return value && !isArray(value) && valueType === 'object';
 	} else if (type === 'null') {
 		return value === null;
 	} else if (type === 'array') {
-		return Array.isArray(value);
+		return isArray(value);
 	} else if (type === 'string') {
 		return valueType === 'string';
 	} else if (type === 'number' || type === 'integer') {
@@ -137,11 +152,19 @@ function getStringValidators(prop: IConfigurationPropertySchema) {
 			}),
 			message: nls.localize('validations.uriSchemeMissing', "URI with a scheme is expected.")
 		},
+		{
+			enabled: prop.enum !== undefined,
+			isValid: ((value: string) => {
+				return prop.enum!.includes(value);
+			}),
+			message: nls.localize('validations.invalidStringEnumValue', "Value is not accepted. Valid values: {0}.",
+				prop.enum ? prop.enum.map(key => `"${key}"`).join(', ') : '[]')
+		}
 	].filter(validation => validation.enabled);
 }
 
 function getNumericValidators(prop: IConfigurationPropertySchema): Validator<number>[] {
-	const type: (string | undefined)[] = Array.isArray(prop.type) ? prop.type : [prop.type];
+	const type: (string | undefined)[] = isArray(prop.type) ? prop.type : [prop.type];
 
 	const isNullable = canBeType(type, 'null');
 	const isIntegral = (canBeType(type, 'integer')) && (type.length === 1 || type.length === 2 && isNullable);
@@ -212,7 +235,13 @@ function getArrayOfStringValidator(prop: IConfigurationPropertySchema): ((value:
 
 				let message = '';
 
-				const stringArrayValue = value as string[];
+				if (!isStringArray(value)) {
+					message += nls.localize('validations.stringArrayIncorrectType', 'Incorrect type. Expected a string array.');
+					message += '\n';
+					return message;
+				}
+
+				const stringArrayValue = value;
 
 				if (prop.uniqueItems) {
 					if (new Set(stringArrayValue).size < stringArrayValue.length) {
@@ -269,3 +298,66 @@ function getArrayOfStringValidator(prop: IConfigurationPropertySchema): ((value:
 
 	return null;
 }
+
+function getObjectValidator(prop: IConfigurationPropertySchema): ((value: any) => (string | null)) | null {
+	if (prop.type === 'object') {
+		const { properties, patternProperties, additionalProperties } = prop;
+		return value => {
+			if (!value) {
+				return null;
+			}
+
+			const errors: string[] = [];
+
+			if (!isObject(value)) {
+				errors.push(nls.localize('validations.objectIncorrectType', 'Incorrect type. Expected an object.'));
+			} else {
+				Object.keys(value).forEach((key: string) => {
+					const data = value[key];
+					if (properties && key in properties) {
+						const errorMessage = getErrorsForSchema(properties[key], data);
+						if (errorMessage) {
+							errors.push(`${key}: ${errorMessage}\n`);
+						}
+						return;
+					}
+
+					if (patternProperties) {
+						for (const pattern in patternProperties) {
+							if (RegExp(pattern).test(key)) {
+								const errorMessage = getErrorsForSchema(patternProperties[pattern], data);
+								if (errorMessage) {
+									errors.push(`${key}: ${errorMessage}\n`);
+								}
+								return;
+							}
+						}
+					}
+
+					if (additionalProperties === false) {
+						errors.push(nls.localize('validations.objectPattern', 'Property {0} is not allowed.\n', key));
+					} else if (typeof additionalProperties === 'object') {
+						const errorMessage = getErrorsForSchema(additionalProperties, data);
+						if (errorMessage) {
+							errors.push(`${key}: ${errorMessage}\n`);
+						}
+					}
+				});
+			}
+
+			if (errors.length) {
+				return prop.errorMessage ? [prop.errorMessage, ...errors].join(' ') : errors.join(' ');
+			}
+
+			return '';
+		};
+	}
+
+	return null;
+}
+
+function getErrorsForSchema(propertySchema: IConfigurationPropertySchema, data: any): string | null {
+	const validator = createValidator(propertySchema);
+	const errorMessage = validator(data);
+	return errorMessage;
+}

src/vs/workbench/services/preferences/test/common/preferencesValidation.test.ts

@@ -16,12 +16,12 @@ suite('Preferences Validation', () => {
 			this.validator = createValidator(settings)!;
 		}
 
-		public accepts(input: string) {
-			assert.strictEqual(this.validator(input), '', `Expected ${JSON.stringify(this.settings)} to accept \`${input}\`. Got ${this.validator(input)}.`);
+		public accepts(input: any) {
+			assert.strictEqual(this.validator(input), '', `Expected ${JSON.stringify(this.settings)} to accept \`${JSON.stringify(input)}\`. Got ${this.validator(input)}.`);
 		}
 
-		public rejects(input: string) {
-			assert.notStrictEqual(this.validator(input), '', `Expected ${JSON.stringify(this.settings)} to reject \`${input}\`.`);
+		public rejects(input: any) {
+			assert.notStrictEqual(this.validator(input), '', `Expected ${JSON.stringify(this.settings)} to reject \`${JSON.stringify(input)}\`.`);
 			return {
 				withMessage:
 					(message: string) => {
@@ -33,7 +33,6 @@ suite('Preferences Validation', () => {
 			};
 		}
 
-
 		public validatesNumeric() {
 			this.accepts('3');
 			this.accepts('3.');
@@ -42,16 +41,24 @@ suite('Preferences Validation', () => {
 			this.accepts(' 3.0');
 			this.accepts(' 3.0  ');
 			this.rejects('3f');
+			this.accepts(3);
+			this.rejects('test');
 		}
 
 		public validatesNullableNumeric() {
 			this.validatesNumeric();
+			this.accepts(0);
 			this.accepts('');
+			this.accepts(null);
+			this.accepts(undefined);
 		}
 
 		public validatesNonNullableNumeric() {
 			this.validatesNumeric();
+			this.accepts(0);
 			this.rejects('');
+			this.rejects(null);
+			this.rejects(undefined);
 		}
 
 		public validatesString() {
@@ -64,6 +71,7 @@ suite('Preferences Validation', () => {
 			this.accepts('');
 			this.accepts('3f');
 			this.accepts('hello');
+			this.rejects(6);
 		}
 	}
 
@@ -225,6 +233,42 @@ suite('Preferences Validation', () => {
 		}
 	});
 
+	test('objects work', () => {
+		{
+			const obj = new Tester({ type: 'object', properties: { 'a': { type: 'string', maxLength: 2 } }, additionalProperties: false });
+			obj.rejects({ 'a': 'string' });
+			obj.accepts({ 'a': 'st' });
+			obj.rejects({ 'a': null });
+			obj.rejects({ 'a': 7 });
+			obj.accepts({});
+			obj.rejects('test');
+			obj.rejects(7);
+			obj.rejects([1, 2, 3]);
+		}
+		{
+			const pattern = new Tester({ type: 'object', patternProperties: { '^a[a-z]$': { type: 'string', minLength: 2 } }, additionalProperties: false });
+			pattern.accepts({ 'ab': 'string' });
+			pattern.accepts({ 'ab': 'string', 'ac': 'hmm' });
+			pattern.rejects({ 'ab': 'string', 'ac': 'h' });
+			pattern.rejects({ 'ab': 'string', 'ac': 99999 });
+			pattern.rejects({ 'abc': 'string' });
+			pattern.rejects({ 'a0': 'string' });
+			pattern.rejects({ 'ab': 'string', 'bc': 'hmm' });
+			pattern.rejects({ 'be': 'string' });
+			pattern.rejects({ 'be': 'a' });
+			pattern.accepts({});
+		}
+		{
+			const pattern = new Tester({ type: 'object', patternProperties: { '^#': { type: 'string', minLength: 3 } }, additionalProperties: { type: 'string', maxLength: 3 } });
+			pattern.accepts({ '#ab': 'string' });
+			pattern.accepts({ 'ab': 'str' });
+			pattern.rejects({ '#ab': 's' });
+			pattern.rejects({ 'ab': 99999 });
+			pattern.rejects({ '#ab': 99999 });
+			pattern.accepts({});
+		}
+	});
+
 	test('patterns work', () => {
 		{
 			const urls = new Tester({ pattern: '^(hello)*$', type: 'string' });
@@ -262,7 +306,7 @@ suite('Preferences Validation', () => {
 			assert.strictEqual(this.validator(input), '', `Expected ${JSON.stringify(this.settings)} to accept \`${JSON.stringify(input)}\`. Got ${this.validator(input)}.`);
 		}
 
-		public rejects(input: any[]) {
+		public rejects(input: any) {
 			assert.notStrictEqual(this.validator(input), '', `Expected ${JSON.stringify(this.settings)} to reject \`${JSON.stringify(input)}\`.`);
 			return {
 				withMessage:
@@ -282,6 +326,8 @@ suite('Preferences Validation', () => {
 			arr.accepts([]);
 			arr.accepts(['foo']);
 			arr.accepts(['foo', 'bar']);
+			arr.rejects(76);
+			arr.rejects([6, '3', 7]);
 		}
 	});