From 94706f19dc6e3e6b92cd9620a05a20d94d8da88c Mon Sep 17 00:00:00 2001 From: Sven Efftinge Date: Tue, 8 Oct 2019 15:27:05 +0000 Subject: [PATCH] [core] Don't reject schemas with issues, but add valid properties. Improves eclipse-theia/theia#4902 Signed-off-by: Sven Efftinge --- .../preferences/preference-contribution.ts | 39 +------------------ .../browser/preferences/preference-proxy.ts | 16 +------- .../preferences/preference-service.spec.ts | 4 -- .../browser/preferences/preference-service.ts | 5 --- .../browser/editor-preview-manager.spec.ts | 2 - 5 files changed, 4 insertions(+), 62 deletions(-) diff --git a/packages/core/src/browser/preferences/preference-contribution.ts b/packages/core/src/browser/preferences/preference-contribution.ts index 4a0f8850f5dcc..e09040d5a4cf4 100644 --- a/packages/core/src/browser/preferences/preference-contribution.ts +++ b/packages/core/src/browser/preferences/preference-contribution.ts @@ -77,7 +77,6 @@ export class PreferenceSchemaProvider extends PreferenceProvider { @inject(ContributionProvider) @named(PreferenceContribution) protected readonly preferenceContributions: ContributionProvider; - protected validateFunction: Ajv.ValidateFunction; @inject(PreferenceConfigurations) protected readonly configurations: PreferenceConfigurations; @@ -94,8 +93,6 @@ export class PreferenceSchemaProvider extends PreferenceProvider { this.doSetSchema(contrib.schema); }); this.combinedSchema.additionalProperties = false; - this.updateValidate(); - this.onDidPreferencesChanged(() => this.updateValidate()); this._ready.resolve(); } @@ -173,8 +170,7 @@ export class PreferenceSchemaProvider extends PreferenceProvider { const valid = ajv.validateSchema(schema); if (!valid) { const errors = !!ajv.errors ? ajv.errorsText(ajv.errors) : 'unknown validation error'; - console.error('An invalid preference schema was rejected: ' + errors); - return []; + console.warn('A contributed preference schema has validation issues : ' + errors); } const scope = PreferenceScope.Default; const domain = this.getDomain(); @@ -193,7 +189,6 @@ export class PreferenceSchemaProvider extends PreferenceProvider { this.overridePatternProperties.properties[preferenceName] = schemaProps; } this.combinedSchema.properties[preferenceName] = schemaProps; - this.unsupportedPreferences.delete(preferenceName); const value = schemaProps.defaultValue = this.getDefaultValue(schemaProps, preferenceName); if (this.testOverrideValue(preferenceName, value)) { @@ -209,6 +204,7 @@ export class PreferenceSchemaProvider extends PreferenceProvider { } return changes; } + protected doSetPreferenceValue(preferenceName: string, newValue: any, { scope, domain }: { scope: PreferenceScope, domain?: string[] @@ -250,37 +246,6 @@ export class PreferenceSchemaProvider extends PreferenceProvider { return null; } - protected updateValidate(): void { - const schema = { - ...this.combinedSchema, - properties: { - ...this.combinedSchema.properties - } - }; - for (const sectionName of this.configurations.getSectionNames()) { - delete schema.properties[sectionName]; - } - this.validateFunction = new Ajv().compile(schema); - } - - protected readonly unsupportedPreferences = new Set(); - validate(name: string, value: any): boolean { - if (this.configurations.isSectionName(name)) { - return true; - } - const overridden = this.overriddenPreferenceName(name); - const preferenceName = overridden && overridden.preferenceName || name; - const result = this.validateFunction({ [preferenceName]: value }) as boolean; - if (!result && !(name in this.combinedSchema.properties)) { - // in order to avoid reporting it on each change - if (!this.unsupportedPreferences.has(name)) { - this.unsupportedPreferences.add(name); - console.warn(`"${name}" preference is not supported`); - } - } - return result; - } - getCombinedSchema(): PreferenceDataSchema { return this.combinedSchema; } diff --git a/packages/core/src/browser/preferences/preference-proxy.ts b/packages/core/src/browser/preferences/preference-proxy.ts index 34526feaeddf7..dac89517a687a 100644 --- a/packages/core/src/browser/preferences/preference-proxy.ts +++ b/packages/core/src/browser/preferences/preference-proxy.ts @@ -86,15 +86,7 @@ export function createPreferenceProxy(preferences: PreferenceService, schema: const preferenceName = OverridePreferenceName.is(arg) ? preferences.overridePreferenceName(arg) : arg; - const value = preferences.get(preferenceName, defaultValue, resourceUri || opts.resourceUri); - if (preferences.validate(preferenceName, value)) { - return value; - } - if (defaultValue !== undefined) { - return defaultValue; - } - const values = preferences.inspect(preferenceName, resourceUri); - return values && values.defaultValue; + return preferences.get(preferenceName, defaultValue, resourceUri || opts.resourceUri); }; const ownKeys: () => string[] = () => { @@ -163,11 +155,7 @@ export function createPreferenceProxy(preferences: PreferenceService, schema: if (value === undefined) { value = preferences.get(fullProperty, undefined, opts.resourceUri); } - if (preferences.validate(fullProperty, value)) { - return value; - } - const values = preferences.inspect(fullProperty, opts.resourceUri); - return values && values.defaultValue; + return value; } } if (property === 'onPreferenceChanged') { diff --git a/packages/core/src/browser/preferences/preference-service.spec.ts b/packages/core/src/browser/preferences/preference-service.spec.ts index f57cc9dc66b6e..4f47ecec3a563 100644 --- a/packages/core/src/browser/preferences/preference-service.spec.ts +++ b/packages/core/src/browser/preferences/preference-service.spec.ts @@ -220,8 +220,6 @@ describe('Preference Service', () => { })), 'events before'); assert.strictEqual(prefService.get('editor.insertSpaces'), true, 'get before'); assert.strictEqual(prefService.get('[go].editor.insertSpaces'), false, 'get before overridden'); - assert.strictEqual(prefSchema.validate('editor.insertSpaces', false), true, 'validate before'); - assert.strictEqual(prefSchema.validate('[go].editor.insertSpaces', true), true, 'validate before overridden'); events.length = 0; toUnset.dispose(); @@ -241,8 +239,6 @@ describe('Preference Service', () => { })), 'events after'); assert.strictEqual(prefService.get('editor.insertSpaces'), undefined, 'get after'); assert.strictEqual(prefService.get('[go].editor.insertSpaces'), undefined, 'get after overridden'); - assert.strictEqual(prefSchema.validate('editor.insertSpaces', true), false, 'validate after'); - assert.strictEqual(prefSchema.validate('[go].editor.insertSpaces', true), false, 'validate after overridden'); }); describe('overridden preferences', () => { diff --git a/packages/core/src/browser/preferences/preference-service.ts b/packages/core/src/browser/preferences/preference-service.ts index dc848193811fb..cbaf9e683d570 100644 --- a/packages/core/src/browser/preferences/preference-service.ts +++ b/packages/core/src/browser/preferences/preference-service.ts @@ -88,7 +88,6 @@ export interface PreferenceService extends Disposable { overriddenPreferenceName(preferenceName: string): OverridePreferenceName | undefined; resolve(preferenceName: string, defaultValue?: T, resourceUri?: string): PreferenceResolveResult; - validate(name: string, value: any): boolean; } /** @@ -358,8 +357,4 @@ export class PreferenceServiceImpl implements PreferenceService { value: result.value !== undefined ? deepFreeze(result.value) : defaultValue }; } - - validate(name: string, value: any): boolean { - return this.schema.validate(name, value); - } } diff --git a/packages/editor-preview/src/browser/editor-preview-manager.spec.ts b/packages/editor-preview/src/browser/editor-preview-manager.spec.ts index 715bd9f9cae26..196018ec375a6 100644 --- a/packages/editor-preview/src/browser/editor-preview-manager.spec.ts +++ b/packages/editor-preview/src/browser/editor-preview-manager.spec.ts @@ -87,12 +87,10 @@ describe('editor-preview-manager', () => { it('should handle preview requests if editor.enablePreview enabled', async () => { (mockPreference.get as sinon.SinonStub).returns(true); - (mockPreference.validate as sinon.SinonStub).returns(true); expect(await previewManager.canHandle(new URI(), { preview: true })).to.be.greaterThan(0); }); it('should not handle preview requests if editor.enablePreview disabled', async () => { (mockPreference.get as sinon.SinonStub).returns(false); - (mockPreference.validate as sinon.SinonStub).returns(true); expect(await previewManager.canHandle(new URI(), { preview: true })).to.equal(0); }); it('should not handle requests that are not preview or currently being previewed', async () => {