Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Don't reject schemas with issues, but add valid properties. #6341

Merged
merged 1 commit into from
Oct 9, 2019
Merged

[core] Don't reject schemas with issues, but add valid properties. #6341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 2 additions & 37 deletions packages/core/src/browser/preferences/preference-contribution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ export class PreferenceSchemaProvider extends PreferenceProvider {

@inject(ContributionProvider) @named(PreferenceContribution)
protected readonly preferenceContributions: ContributionProvider<PreferenceContribution>;
protected validateFunction: Ajv.ValidateFunction;

@inject(PreferenceConfigurations)
protected readonly configurations: PreferenceConfigurations;
Expand All @@ -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();
}

Expand Down Expand Up @@ -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();
Expand All @@ -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)) {
Expand All @@ -209,6 +204,7 @@ export class PreferenceSchemaProvider extends PreferenceProvider {
}
return changes;
}

protected doSetPreferenceValue(preferenceName: string, newValue: any, { scope, domain }: {
scope: PreferenceScope,
domain?: string[]
Expand Down Expand Up @@ -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<string>();
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;
}
Expand Down
16 changes: 2 additions & 14 deletions packages/core/src/browser/preferences/preference-proxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,15 +86,7 @@ export function createPreferenceProxy<T>(preferences: PreferenceService, schema:
const preferenceName = OverridePreferenceName.is(arg) ?
preferences.overridePreferenceName(arg) :
<string>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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we are fine with runtime errors for statically typed proxies? None of clients is checking for invalid values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's strange to do it partly and VS code is not doing it at all.

};

const ownKeys: () => string[] = () => {
Expand Down Expand Up @@ -163,11 +155,7 @@ export function createPreferenceProxy<T>(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') {
Expand Down
4 changes: 0 additions & 4 deletions packages/core/src/browser/preferences/preference-service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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', () => {
Expand Down
5 changes: 0 additions & 5 deletions packages/core/src/browser/preferences/preference-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ export interface PreferenceService extends Disposable {
overriddenPreferenceName(preferenceName: string): OverridePreferenceName | undefined;

resolve<T>(preferenceName: string, defaultValue?: T, resourceUri?: string): PreferenceResolveResult<T>;
validate(name: string, value: any): boolean;
}

/**
Expand Down Expand Up @@ -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);
}
}
2 changes: 0 additions & 2 deletions packages/editor-preview/src/browser/editor-preview-manager.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down