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

svenefftinge · 2019-10-07T13:55:56Z

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

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

AlexTugarev · 2019-10-08T11:28:20Z

@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:

theia/packages/core/src/browser/preferences/preference-contribution.ts

Lines 212 to 214 in b4972e3

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

packages/core/src/common/preferences/preference-schema.ts

AlexTugarev · 2019-10-08T15:24:09Z

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 👍

Improves #4902 Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>

AlexTugarev

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.

akosyakov

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

akosyakov · 2019-10-08T17:12:17Z

packages/core/src/browser/preferences/preference-proxy.ts

-        }
-        const values = preferences.inspect(preferenceName, resourceUri);
-        return values && values.defaultValue;
+        return preferences.get(preferenceName, defaultValue, resourceUri || opts.resourceUri);


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

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

AlexTugarev · 2019-10-09T06:20:01Z

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 · 2019-10-09T06:27:04Z

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 requested a review from AlexTugarev October 7, 2019 14:03

akosyakov added preferences issues related to preferences vscode issues related to VSCode compatibility labels Oct 7, 2019

svenefftinge force-pushed the GH-4902 branch from b4972e3 to f65d9cb Compare October 8, 2019 13:29

vince-fugnitto mentioned this pull request Oct 8, 2019

Gitlens Issue #6347

Closed

AlexTugarev reviewed Oct 8, 2019

View reviewed changes

packages/core/src/common/preferences/preference-schema.ts Outdated Show resolved Hide resolved

AlexTugarev reviewed Oct 8, 2019

View reviewed changes

packages/core/src/common/preferences/preference-schema.ts Outdated Show resolved Hide resolved

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

AlexTugarev reviewed Oct 8, 2019

View reviewed changes

packages/core/src/common/preferences/preference-schema.ts Outdated Show resolved Hide resolved

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

6311ef1

Improves #4902 Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>

svenefftinge force-pushed the GH-4902 branch from 3598028 to 6311ef1 Compare October 8, 2019 15:30

AlexTugarev approved these changes Oct 8, 2019

View reviewed changes

akosyakov reviewed Oct 8, 2019

View reviewed changes

svenefftinge merged commit 94706f1 into master Oct 9, 2019

svenefftinge deleted the GH-4902 branch October 9, 2019 06:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

svenefftinge commented Oct 7, 2019 •

edited

Loading

AlexTugarev commented Oct 8, 2019

AlexTugarev commented Oct 8, 2019

AlexTugarev left a comment

akosyakov left a comment

akosyakov Oct 8, 2019

svenefftinge Oct 9, 2019

AlexTugarev commented Oct 9, 2019

svenefftinge commented Oct 9, 2019 •

edited

Loading

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

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

Conversation

svenefftinge commented Oct 7, 2019 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

AlexTugarev commented Oct 8, 2019

AlexTugarev commented Oct 8, 2019

AlexTugarev left a comment

Choose a reason for hiding this comment

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov Oct 8, 2019

Choose a reason for hiding this comment

svenefftinge Oct 9, 2019

Choose a reason for hiding this comment

AlexTugarev commented Oct 9, 2019

svenefftinge commented Oct 9, 2019 • edited Loading

svenefftinge commented Oct 7, 2019 •

edited

Loading

svenefftinge commented Oct 9, 2019 •

edited

Loading