From 95f6c5d7e41334f5bdf301bcce4edd7169036481 Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Mon, 17 Jan 2022 13:51:38 -0700 Subject: [PATCH] validation in preference proxy --- .../browser/preferences/preference-proxy.ts | 76 ++++++++++++------- .../preference-validation-service.spec.ts | 2 +- .../preference-validation-service.ts | 19 +++-- .../editor/src/browser/editor-preferences.ts | 12 ++- .../src/browser/monaco-editor-provider.ts | 27 ++----- 5 files changed, 78 insertions(+), 58 deletions(-) diff --git a/packages/core/src/browser/preferences/preference-proxy.ts b/packages/core/src/browser/preferences/preference-proxy.ts index bcbe2827b96a9..c82bd812e1c77 100644 --- a/packages/core/src/browser/preferences/preference-proxy.ts +++ b/packages/core/src/browser/preferences/preference-proxy.ts @@ -16,11 +16,12 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { Disposable, Event, MaybePromise } from '../../common'; +import { Disposable, Emitter, Event, MaybePromise } from '../../common'; import { PreferenceService } from './preference-service'; import { PreferenceSchema } from './preference-contribution'; import { PreferenceScope } from './preference-scope'; import { OverridePreferenceName } from './preference-language-override-service'; +import { PreferenceValidationService } from './preference-validation-service'; /** * It is worth explaining the type for `PreferenceChangeEvent`: @@ -167,7 +168,12 @@ export interface PreferenceProxyOptions { * * Note that if `schema` is a Promise, most actions will be no-ops until the promise is resolved. */ -export function createPreferenceProxy(preferences: PreferenceService, promisedSchema: MaybePromise, options?: PreferenceProxyOptions): PreferenceProxy { +export function createPreferenceProxy( + preferences: PreferenceService, + promisedSchema: MaybePromise, + options?: PreferenceProxyOptions, + validator?: PreferenceValidationService +): PreferenceProxy { const opts = options || {}; const prefix = opts.prefix || ''; const style = opts.style || 'flat'; @@ -179,31 +185,43 @@ export function createPreferenceProxy(preferences: PreferenceService, promise } else { promisedSchema.then(s => schema = s); } - const onPreferenceChanged = (listener: (e: PreferenceChangeEvent) => any, thisArgs?: any, disposables?: Disposable[]) => preferences.onPreferencesChanged(changes => { - if (schema) { - for (const key of Object.keys(changes)) { - const e = changes[key]; - const overridden = preferences.overriddenPreferenceName(e.preferenceName); - const preferenceName: any = overridden ? overridden.preferenceName : e.preferenceName; - if (preferenceName.startsWith(prefix) && (!overridden || !opts.overrideIdentifier || overridden.overrideIdentifier === opts.overrideIdentifier)) { - if (schema.properties[preferenceName]) { - const { newValue, oldValue } = e; - listener({ - newValue, oldValue, preferenceName, - affects: (resourceUri, overrideIdentifier) => { - if (overrideIdentifier !== undefined) { - if (overridden && overridden.overrideIdentifier !== overrideIdentifier) { - return false; + let emitter: Emitter | undefined; + const onPreferenceChanged = (listener: (e: PreferenceChangeEvent) => any, thisArgs?: any, disposables?: Disposable[]) => ensureEmitter() + .event(listener, thisArgs, disposables); + + const ensureEmitter = (): Emitter> => { + if (emitter !== undefined) { + return emitter; + } + emitter = new Emitter>(); + preferences.onPreferencesChanged(changes => { + if (schema) { + for (const key of Object.keys(changes)) { + const e = changes[key]; + const overridden = preferences.overriddenPreferenceName(e.preferenceName); + const preferenceName: any = overridden ? overridden.preferenceName : e.preferenceName; + if (preferenceName.startsWith(prefix) && (!overridden || !opts.overrideIdentifier || overridden.overrideIdentifier === opts.overrideIdentifier)) { + if (schema.properties[preferenceName]) { + const { newValue, oldValue } = e; + const validatedNewValue = validator ? validator.validateByName(preferenceName, newValue) : newValue; + emitter!.fire(>{ + newValue: validatedNewValue, oldValue, preferenceName, + affects: (resourceUri, overrideIdentifier) => { + if (overrideIdentifier !== undefined) { + if (overridden && overridden.overrideIdentifier !== overrideIdentifier) { + return false; + } } + return e.affects(resourceUri); } - return e.affects(resourceUri); - } - }); + }); + } } } } - } - }, thisArgs, disposables); + }); + return emitter; + }; const unsupportedOperation = (_: any, __: string) => { throw new Error('Unsupported operation'); @@ -213,7 +231,8 @@ export function createPreferenceProxy(preferences: PreferenceService, promise const preferenceName = OverridePreferenceName.is(arg) ? preferences.overridePreferenceName(arg) : arg; - return preferences.get(preferenceName, defaultValue, resourceUri || opts.resourceUri); + const value = preferences.get(preferenceName, defaultValue, resourceUri || opts.resourceUri); + return validator ? validator.validateByName(preferenceName, value) : value; }; const ownKeys: () => string[] = () => { @@ -259,7 +278,7 @@ export function createPreferenceProxy(preferences: PreferenceService, promise resourceUri: opts.resourceUri, overrideIdentifier: opts.overrideIdentifier, style - }); + }, validator); for (const k of Object.keys(value)) { subProxy[k] = value[k]; } @@ -275,7 +294,7 @@ export function createPreferenceProxy(preferences: PreferenceService, promise } const fullProperty = prefix ? prefix + property : property; if (schema) { - if (isFlat || property.indexOf('.') === -1) { + if (isFlat || !property.includes('.')) { if (schema.properties[fullProperty]) { let value; if (opts.overrideIdentifier) { @@ -287,7 +306,7 @@ export function createPreferenceProxy(preferences: PreferenceService, promise if (value === undefined) { value = preferences.get(fullProperty, undefined, opts.resourceUri); } - return value; + return (validator && value !== undefined) ? validator.validateByName(fullProperty, value) : value; } } } @@ -310,7 +329,8 @@ export function createPreferenceProxy(preferences: PreferenceService, promise const newPrefix = fullProperty + '.'; for (const p of Object.keys(schema.properties)) { if (p.startsWith(newPrefix)) { - return createPreferenceProxy(preferences, schema, { prefix: newPrefix, resourceUri: opts.resourceUri, overrideIdentifier: opts.overrideIdentifier, style }); + return createPreferenceProxy(preferences, schema, + { prefix: newPrefix, resourceUri: opts.resourceUri, overrideIdentifier: opts.overrideIdentifier, style }, validator); } } @@ -330,7 +350,7 @@ export function createPreferenceProxy(preferences: PreferenceService, promise while (typeof value === 'object' && (segment = segments.pop())) { value = value[segment]; } - return segments.length ? undefined : value; + return segments.length ? undefined : validator?.validateByName(fullProperty, value) ?? value; } return undefined; }; diff --git a/packages/core/src/browser/preferences/preference-validation-service.spec.ts b/packages/core/src/browser/preferences/preference-validation-service.spec.ts index b4d629b7ef745..8c066605de83a 100644 --- a/packages/core/src/browser/preferences/preference-validation-service.spec.ts +++ b/packages/core/src/browser/preferences/preference-validation-service.spec.ts @@ -28,7 +28,7 @@ describe('Preference Validation Service', () => { container.bind(PreferenceSchemaProvider).toConstantValue({ getDefaultValue: PreferenceSchemaProvider.prototype.getDefaultValue } as PreferenceSchemaProvider); container.bind(PreferenceLanguageOverrideService).toSelf().inSingletonScope(); const validator = container.resolve(PreferenceValidationService); - const validateBySchema: (value: JSONValue, schema: PreferenceItem) => JSONValue = validator.validateBySchema.bind(validator, 'dummy'); + const validateBySchema: (value: JSONValue, schema: PreferenceItem) => JSONValue = validator['doValidateBySchema'].bind(validator, 'dummy'); describe('should validate strings', () => { const expected = 'expected'; diff --git a/packages/core/src/browser/preferences/preference-validation-service.ts b/packages/core/src/browser/preferences/preference-validation-service.ts index ca81df3f25d8c..4d65e04788eda 100644 --- a/packages/core/src/browser/preferences/preference-validation-service.ts +++ b/packages/core/src/browser/preferences/preference-validation-service.ts @@ -49,7 +49,6 @@ export class PreferenceValidationService { const validValue = this.validateByName(preferenceName, value); if (validValue !== value) { problemsDetected = true; - console.warn(`While validating options, found impermissible value for ${preferenceName}. Using valid value`, validValue, 'instead of configured value', value); } valid[preferenceName] = validValue; } @@ -62,6 +61,14 @@ export class PreferenceValidationService { } validateBySchema(key: string, value: JSONValue, schema: ValidatablePreference | undefined): JSONValue { + const validated = this.doValidateBySchema(key, value, schema); + if (validated !== value) { + console.warn(`While validating prefrences, found impermissible value for ${key}. Using valid value`, validated, 'instead of configured value', value, new Error()); + } + return validated; + } + + protected doValidateBySchema(key: string, value: JSONValue, schema: ValidatablePreference | undefined): JSONValue { try { if (!schema) { console.warn('Request to validate preference with no schema registered:', key); @@ -118,7 +125,7 @@ export class PreferenceValidationService { const candidate = this.mapValidators(key, value, (function* (this: PreferenceValidationService): Iterable { for (const type of schema.type) { validation.type = type as JsonType; - yield toValidate => this.validateBySchema(key, toValidate, validation); + yield toValidate => this.doValidateBySchema(key, toValidate, validation); } }).bind(this)()); if (candidate !== value && (schema.default !== undefined || schema.defaultValue !== undefined)) { @@ -131,7 +138,7 @@ export class PreferenceValidationService { protected validateAnyOf(key: string, value: JSONValue, schema: ValidatablePreference & { anyOf: ValidatablePreference[] }): JSONValue { const candidate = this.mapValidators(key, value, (function* (this: PreferenceValidationService): Iterable { for (const option of schema.anyOf) { - yield toValidate => this.validateBySchema(key, toValidate, option); + yield toValidate => this.doValidateBySchema(key, toValidate, option); } }).bind(this)()); if (candidate !== value && (schema.default !== undefined || schema.defaultValue !== undefined)) { @@ -164,7 +171,7 @@ export class PreferenceValidationService { } const valid = []; for (const item of candidate) { - const validated = this.validateBySchema(key, item, schema.items); + const validated = this.doValidateBySchema(key, item, schema.items); if (validated === item) { valid.push(item); } @@ -242,12 +249,12 @@ export class PreferenceValidationService { for (const [fieldKey, fieldValue] of Object.entries(value)) { const fieldLabel = `${key}#${fieldKey}`; if (schema.properties && fieldKey in schema.properties) { - const valid = this.validateBySchema(fieldLabel, fieldValue, schema.properties[fieldKey]); + const valid = this.doValidateBySchema(fieldLabel, fieldValue, schema.properties[fieldKey]); if (valid !== fieldValue) { return false; } } else if (additionalPropertyValidator) { - const valid = this.validateBySchema(fieldLabel, fieldValue, additionalPropertyValidator); + const valid = this.doValidateBySchema(fieldLabel, fieldValue, additionalPropertyValidator); if (valid !== fieldValue) { return false; } diff --git a/packages/editor/src/browser/editor-preferences.ts b/packages/editor/src/browser/editor-preferences.ts index 94206afcd1ff3..f7ab2ef4fab6f 100644 --- a/packages/editor/src/browser/editor-preferences.ts +++ b/packages/editor/src/browser/editor-preferences.ts @@ -22,7 +22,8 @@ import { PreferenceContribution, PreferenceSchema, PreferenceChangeEvent, - PreferenceSchemaProperties + PreferenceSchemaProperties, + PreferenceValidationService } from '@theia/core/lib/browser/preferences'; import { isWindows, isOSX, OS } from '@theia/core/lib/common/os'; import { nls } from '@theia/core/lib/common/nls'; @@ -1582,15 +1583,18 @@ export const EditorPreferenceContribution = Symbol('EditorPreferenceContribution export const EditorPreferences = Symbol('EditorPreferences'); export type EditorPreferences = PreferenceProxy; -export function createEditorPreferences(preferences: PreferenceService, schema: PreferenceSchema = editorPreferenceSchema): EditorPreferences { - return createPreferenceProxy(preferences, schema); +export function createEditorPreferences( + preferences: PreferenceService, schema: PreferenceSchema = editorPreferenceSchema, validator?: PreferenceValidationService +): EditorPreferences { + return createPreferenceProxy(preferences, schema, undefined, validator); } export function bindEditorPreferences(bind: interfaces.Bind): void { bind(EditorPreferences).toDynamicValue(ctx => { const preferences = ctx.container.get(PreferenceService); const contribution = ctx.container.get(EditorPreferenceContribution); - return createEditorPreferences(preferences, contribution.schema); + const validator = ctx.container.get(PreferenceValidationService); + return createEditorPreferences(preferences, contribution.schema, validator); }).inSingletonScope(); bind(EditorPreferenceContribution).toConstantValue({ schema: editorPreferenceSchema }); bind(PreferenceContribution).toService(EditorPreferenceContribution); diff --git a/packages/monaco/src/browser/monaco-editor-provider.ts b/packages/monaco/src/browser/monaco-editor-provider.ts index 5178548d3c8dc..28b825ed5bb4b 100644 --- a/packages/monaco/src/browser/monaco-editor-provider.ts +++ b/packages/monaco/src/browser/monaco-editor-provider.ts @@ -35,7 +35,7 @@ import { MonacoBulkEditService } from './monaco-bulk-edit-service'; import IEditorOverrideServices = monaco.editor.IEditorOverrideServices; import { ApplicationServer } from '@theia/core/lib/common/application-protocol'; import { ContributionProvider } from '@theia/core'; -import { KeybindingRegistry, OpenerService, open, WidgetOpenerOptions, FormatType, PreferenceValidationService } from '@theia/core/lib/browser'; +import { KeybindingRegistry, OpenerService, open, WidgetOpenerOptions, FormatType } from '@theia/core/lib/browser'; import { MonacoResolvedKeybinding } from './monaco-resolved-keybinding'; import { HttpOpenHandlerOptions } from '@theia/core/lib/browser/http-open-handler'; import { MonacoToProtocolConverter } from './monaco-to-protocol-converter'; @@ -74,9 +74,6 @@ export class MonacoEditorProvider { @inject(MonacoQuickInputImplementation) protected readonly quickInputService: MonacoQuickInputImplementation; - @inject(PreferenceValidationService) - protected readonly preferenceValidator: PreferenceValidationService; - protected _current: MonacoEditor | undefined; /** * Returns the last focused MonacoEditor. @@ -274,9 +271,7 @@ export class MonacoEditorProvider { if (event) { const preferenceName = event.preferenceName; const overrideIdentifier = editor.document.languageId; - const newValue = this.preferenceValidator.validateByName(preferenceName, - this.editorPreferences.get({ preferenceName, overrideIdentifier }, undefined, editor.uri.toString()) - ); + const newValue = this.editorPreferences.get({ preferenceName, overrideIdentifier }, undefined, editor.uri.toString()); editor.getControl().updateOptions(this.setOption(preferenceName, newValue, this.preferencePrefixes)); } else { const options = this.createMonacoEditorOptions(editor.document); @@ -305,9 +300,7 @@ export class MonacoEditorProvider { } const overrideIdentifier = editor.document.languageId; const uri = editor.uri.toString(); - const formatOnSave = this.preferenceValidator.validateByName('editor.formatOnSave', - this.editorPreferences.get({ preferenceName: 'editor.formatOnSave', overrideIdentifier }, undefined, uri)! - ); + const formatOnSave = this.editorPreferences.get({ preferenceName: 'editor.formatOnSave', overrideIdentifier }, undefined, uri)!; if (formatOnSave) { const formatOnSaveTimeout = this.editorPreferences.get({ preferenceName: 'editor.formatOnSaveTimeout', overrideIdentifier }, undefined, uri)!; await Promise.race([ @@ -358,9 +351,7 @@ export class MonacoEditorProvider { if (event) { const preferenceName = event.preferenceName; const overrideIdentifier = editor.document.languageId; - const newValue = this.preferenceValidator.validateByName(preferenceName, - this.editorPreferences.get({ preferenceName, overrideIdentifier }, undefined, resourceUri) - ); + const newValue = this.editorPreferences.get({ preferenceName, overrideIdentifier }, undefined, resourceUri); editor.diffEditor.updateOptions(this.setOption(preferenceName, newValue, this.diffPreferencePrefixes)); } else { const options = this.createMonacoDiffEditorOptions(editor.originalModel, editor.modifiedModel); @@ -372,12 +363,10 @@ export class MonacoEditorProvider { protected createOptions(prefixes: string[], uri: string): { [name: string]: any }; protected createOptions(prefixes: string[], uri: string, overrideIdentifier: string): { [name: string]: any }; protected createOptions(prefixes: string[], uri: string, overrideIdentifier?: string): { [name: string]: any } { - const flat: Record = {}; - for (const preferenceName of Object.keys(this.editorPreferences)) { - flat[preferenceName] = (this.editorPreferences).get({ preferenceName, overrideIdentifier }, undefined, uri); - } - const valid = this.preferenceValidator.validateOptions(flat); - return Object.entries(valid).reduce((tree, [preferenceName, value]) => this.setOption(preferenceName, deepClone(value), prefixes, tree), {}); + return Object.keys(this.editorPreferences).reduce((options, preferenceName) => { + const value = (this.editorPreferences).get({ preferenceName, overrideIdentifier }, undefined, uri); + return this.setOption(preferenceName, deepClone(value), prefixes, options); + }, {}); } protected setOption(preferenceName: string, value: any, prefixes: string[], options: { [name: string]: any } = {}): {