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.

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

Conversation

svenefftinge
Copy link
Contributor

@svenefftinge svenefftinge commented Oct 7, 2019

What it does

Allows configuration schemas to have issues.
Some vs code extensions have schema validation issues in their configuration section.
Instead of rejecting the full contribution, which will effectively break the extension, this PR just logs the validation message as a warning. Moreover the PR removes the runtime validation of actual preference values and leaves this to the user and the preference consumer.

How to test

Install gitlens.

Review checklist

Reminder for reviewers

@akosyakov akosyakov added preferences issues related to preferences vscode issues related to VSCode compatibility labels Oct 7, 2019
@AlexTugarev
Copy link
Contributor

@svenefftinge this appears to work with the invalid pref schema of the gitlense extension 🎉

root WARN A preference schema with validation issues was registered: data.properties['gitlens.remotes'].items.properties['description'] should be object,boolean, data.properties['gitlens.remotes'].items should be array, data.properties['gitlens.remotes'].items should match some schema in anyOf
root WARN Removing invalid property description

But the broken schema of the rust extension is not cleaned up.

root WARN A preference schema with validation issues was registered: data.properties['rust-client.channel'].type should be equal to one of the allowed values, data.properties['rust-client.channel'].type[1] should be equal to one of the allowed values, data.properties['rust-client.channel'].type should match some schema in anyOf
root ERROR Error: schema is invalid: data.properties['rust-client.channel'].type should be equal to one of the allowed values, data.properties['rust-client.channel'].type[1] should be equal to one of the allowed values, data.properties['rust-client.channel'].type should match some schema in anyOf
    at Ajv.validateSchema (https://3000-b9392769-00a5-411d-8e84-e3f994899c45.ws-eu0.gitpod.io/bundle.js:26918:16)
    at Ajv._addSchema (https://3000-b9392769-00a5-411d-8e84-e3f994899c45.ws-eu0.gitpod.io/bundle.js:27047:10)
    at Ajv.compile (https://3000-b9392769-00a5-411d-8e84-e3f994899c45.ws-eu0.gitpod.io/bundle.js:26847:24)
    at PreferenceSchemaProvider.../../packages/core/lib/browser/preferences/preference-contribution.js.PreferenceSchemaProvider.updateValidate (https://3000-b9392769-00a5-411d-8e84-e3f994899c45.ws-eu0.gitpod.io/bundle.js:95439:43)
    at https://3000-b9392769-00a5-411d-8e84-e3f994899c45.ws-eu0.gitpod.io/bundle.js:95221:65
    at https://3000-b9392769-00a5-411d-8e84-e3f994899c45.ws-eu0.gitpod.io/bundle.js:113679:33
    at CallbackList.../../packages/core/lib/common/event.js.CallbackList.invoke (https://3000-b9392769-00a5-411d-8e84-e3f994899c45.ws-eu0.gitpod.io/bundle.js:113694:39)
    at Emitter.../../packages/core/lib/common/event.js.Emitter.fire (https://3000-b9392769-00a5-411d-8e84-e3f994899c45.ws-eu0.gitpod.io/bundle.js:113782:29)
    at PreferenceSchemaProvider.../../packages/core/lib/browser/preferences/preference-provider.js.PreferenceProvider.emitPreferencesChangedEvent (https://3000-b9392769-00a5-411d-8e84-e3f994899c45.ws-eu0.gitpod.io/bundle.js:95720:49)
    at PreferenceSchemaProvider.../../packages/core/lib/browser/preferences/preference-contribution.js.PreferenceSchemaProvider.setSchema (https://3000-b9392769-00a5-411d-8e84-e3f994899c45.ws-eu0.gitpod.io/bundle.js:95468:14)

Looking at the code responsible to filter out invalid properties, I don't understand, how invalid ones are detected here:

if (!PreferenceSchemaProperties.is(property)) {
console.warn(`Removing invalid property ${preferenceName}`);
delete properties[preferenceName];

@svenefftinge svenefftinge force-pushed the GH-4902 branch 3 times, most recently from 419fcac to 3598028 Compare October 8, 2019 14:51
@AlexTugarev
Copy link
Contributor

strings.ts:31 Uncaught TypeError: Cannot read property 'replace' of null
    at Object.escapeInvisibleChars (strings.ts:31)
    at preferences-menu-factory.ts:39
    at Array.forEach (<anonymous>)
    at PreferencesMenuFactory.push.../../packages/preferences/lib/browser/preferences-menu-factory.js.PreferencesMenuFactory.createPreferenceContextMenu (preferences-menu-factory.ts:34)
    at PreferencesTreeWidget.push.../../packages/preferences/lib/browser/preferences-tree-widget.js.PreferencesTreeWidget.openContextMenu (preferences-tree-widget.ts:524)
    at PreferencesTreeWidget.push.../../packages/preferences/lib/browser/preferences-tree-widget.js.PreferencesTreeWidget.handleClickEvent (preferences-tree-widget.ts:504)
    at onClick (tree-widget.tsx:644)
    at HTMLUnknownElement.callCallback (react-dom.development.js:100)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:138)
    at Object.invokeGuardedCallback (react-dom.development.js:187)

this is raised when trying to use the (tree based) preference editor for invalid pref items like rust-client.channel.

I don't think this is critical here; now one can use at least the valid parts 👍

Copy link
Contributor

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

This works nicely for me. I tried with vscode extensions like gitlense and rust, and it let's you use the valid preference items 👍

The preference editor currently doesn't give any feedback on invalid schema items; we might think about it in follow-ups.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

@AlexTugarev have you tried with not vscode extensions, but with native extensions like filesystem?

}
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.

@svenefftinge svenefftinge merged commit 94706f1 into master Oct 9, 2019
@AlexTugarev
Copy link
Contributor

have you tried with not vscode extensions, but with native extensions like filesystem?

@akosyakov now I got your point! "files.encoding": "FOOBAR" is passed read as "FOOBAR" in the filesystem extension, but without this change it's the default value (something like "uft8"). This break with the usual behavior indeed.

@svenefftinge
Copy link
Contributor Author

svenefftinge commented Oct 9, 2019

I didn't find any preferences that would cause bad side effects with bad values myself during a quick try. We can add guards to those places but we had now multiple VS code extensions with bad or incomplete preference schema so being strict on this level doesn't seem right.
An alternative would be to hand-roll a type check where we used to use the ajv validation.

@svenefftinge svenefftinge deleted the GH-4902 branch October 9, 2019 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences issues related to preferences vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants