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

Fix handling malformed recentworkspace.json #10711

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

alxflam
Copy link
Contributor

@alxflam alxflam commented Feb 3, 2022

What it does

Fixes #10250.
In case a recentworkspace.json contains any non string element in its recentRoots array, the JSON is not considered valid.

How to test

Run the provided tests.
Adapt your recentworkspace.json file as specified in issue #10250.

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added the workspace issues related to the workspace label Feb 3, 2022
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution 👍

The test failures on Windows will need to be looked at as they currently fail:

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

Note that CI is failing. Unless there is a reason to change the way the paths look, please make sure the previous tests for the DefaultWorkspaceServer still pass.

@alxflam
Copy link
Contributor Author

alxflam commented Feb 5, 2022

@vince-fugnitto @paul-marechal thanks for the review and sorry for the CI noise - i should have known this will fail on windows, i've set up a win VM to check locally and fix the tests properly and then get back.

@alxflam
Copy link
Contributor Author

alxflam commented Feb 6, 2022

@vince-fugnitto @paul-marechal adapted the tests and resolved your comments. The tests are now running on all platforms.

@vince-fugnitto
Copy link
Member

@colin-grant-work do you mind reviewing the pull-request, I believe you opened the original issue #10250. At least for me on master the application seems to be robust to the point where it will gracefully fall on it's feet if the recentworkspaces.json is malformed or inexistent folders are referenced.

@alxflam alxflam force-pushed the fixRecentWorkspaces branch 2 times, most recently from 3fb6061 to d0d8e08 Compare February 11, 2022 14:53
In case a recentworkspace.json contains any non string element in its recentRoots array, the JSON is not considered valid.

Signed-off-by: Alexander Flammer <alex.flammer.dev@outlook.com>

fix test on windows

Signed-off-by: Alexander Flammer <alex.flammer.dev@outlook.com>

one more try to fix test

Signed-off-by: Alexander Flammer <alex.flammer.dev@outlook.com>

use uri for windows to try to make test pass

Signed-off-by: Alexander Flammer <alex.flammer.dev@outlook.com>

ignore non-string array entries

Signed-off-by: Alexander Flammer <alex.flammer.dev@outlook.com>

Replace check for undefined with null

Signed-off-by: Alexander Flammer <alex.flammer.dev@outlook.com>
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

@colin-grant-work colin-grant-work merged commit 40227dc into eclipse-theia:master Feb 14, 2022
@alxflam alxflam deleted the fixRecentWorkspaces branch February 15, 2022 22:35
thegecko pushed a commit to ARMmbed/theia that referenced this pull request Feb 17, 2022
Ensures that the value in the file is an array, and filters out and non-string entries.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workspace issues related to the workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Malformed recentworkspaces.json can crash startup
4 participants