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 crash when when editing a file that doesn't belong to a project #3413

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

BPScott
Copy link
Contributor

@BPScott BPScott commented Aug 25, 2023

When loading vscode-graphql it constantly crashes.

Screenshot 2023-09-22 at 09 47 06

In the output there's lots of

[Error - 8:27:20 AM] there was an error loading the project config for this file YDe: File '[SNIPPED OUT FILENAME HERE].json' doesn't match any project
/Users/ben/.vscode/extensions/graphql.vscode-graphql-0.8.18/out/server/index.js:4640
 '${t.documents}'`),this._logger.error(String(r))}}async _cacheAllProjectFiles(t){if(t?.projects)return Promise.all(Object.keys(t.projects).map(async r=>{let n=t.getProject(r);await this._cacheSchemaFilesForProject(n),await this._cacheDocumentFilesforProject(n)}))}_isRelayCompatMode(t){return t.includes("RelayCompat")||t.includes("react-relay/compat")}async _updateFragmentDefinition(t,r){let n=this._graphQLCache.getGraphQLConfig().dirpath;await this._graphQLCache.updateFragmentDefinition(n,t,r)}async _updateSchemaIfChanged(t,r){await Promise.all(this._unwrapProjectSchema(t).map(async n=>{let i=Dx.resolve(t.dirpath,n);uV.URI.parse(r).fsPath===i&&await this._graphQLCache.invalidateSchemaCacheForProject(t)}))}_unwrapProjectSchema(t){let r=t.schema,n=[];if(typeof r=="string")n.push(r);else if(Array.isArray(r))for(let i of r)typeof i=="string"?n.push(i):i&&n.push(...Object.keys(i));else n.push(...Object.keys(r));return n}async _updateObjectTypeDefinition(t,r){let n=this._graphQLCache.getGraphQLConfig().dirpath;await this._graphQLCache.updateObjectTypeDefinition(n,t,r)}_getCachedDocument(t){if(this._textDocumentCache.has(t)){let r=this._textDocumentCache.get(t);if(r)return r}return null}async _invalidateCache(t,r,n){var i;if(this._textDocumentCache.has(r)){let a=this._textDocumentCache.get(r);if(a&&t&&t?.version&&a.version<t.version)return this._textDocumentCache.set(r,{version:t.version,contents:n})}return this._textDocumentCache.set(r,{version:(i=t.version)!==null&&i!==void 0?i:0,contents:n})}};kx.MessageProcessor=QGe;function YGe(e,t,r){let n=t.split(`
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                            ^

TypeError: Cannot read properties of null (reading 'schema')
    at QGe._unwrapProjectSchema (/Users/ben/.vscode/extensions/graphql.vscode-graphql-0.8.18/out/server/index.js:4640:749)
    at QGe._updateSchemaIfChanged (/Users/ben/.vscode/extensions/graphql.vscode-graphql-0.8.18/out/server/index.js:4640:558)
    at /Users/ben/.vscode/extensions/graphql.vscode-graphql-0.8.18/out/server/index.js:4635:5332
    at async Promise.all (index 0)

[Error - 8:27:20 AM] The vscode-graphql server crashed 5 times in the last 3 minutes. The server will not be restarted. See the output for more information.

#3216 Introduced reloading the schema when a change is detected, however if you modify a file that does not belong to a schema then the project can be absent. Passing null/undefined into _updateSchemaIfChanged throws an error and crashes the language-service-server.

This PR wraps the call to await this._updateSchemaIfChanged(project, uri); in a check to ensure that the project exists. This was not caught in the initial PR because the type for getProjectForFile is incorrect - it claims it will always return a project config, but that is not true.

The GraphQLCacheInterface suggests that when a project is absent then the return type of getProjectForFile should be void, but the concrete implementation returns null. I've changed the implementation to match the interface rather than the other way around to ensure the smallest impact area.

I'd argue that changing the interface to getProjectForFile: (uri: string) => GraphQLProjectConfig | null; would make the most sense but that change to the interface is strictly a breaking change and I'd rather avoid that in a bugfix PR.

@changeset-bot
Copy link

changeset-bot bot commented Aug 25, 2023

🦋 Changeset detected

Latest commit: 6f0c1f9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
graphql-language-service-server Patch
graphql-language-service-cli Patch
vscode-graphql Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 25, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: BPScott / name: Ben Scott (6f0c1f9)

@BPScott BPScott force-pushed the fix-crash branch 3 times, most recently from 330b6db to 451fcae Compare August 27, 2023 23:21
@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #3413 (6f0c1f9) into main (2348641) will increase coverage by 0.09%.
Report is 13 commits behind head on main.
The diff coverage is 95.45%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3413      +/-   ##
==========================================
+ Coverage   55.75%   55.84%   +0.09%     
==========================================
  Files         110      110              
  Lines        5243     5257      +14     
  Branches     1426     1433       +7     
==========================================
+ Hits         2923     2936      +13     
  Misses       1897     1897              
- Partials      423      424       +1     
Files Coverage Δ
...raphql-language-service-server/src/GraphQLCache.ts 50.88% <50.00%> (ø)
...ql-language-service-server/src/MessageProcessor.ts 46.11% <50.00%> (-0.11%) ⬇️
...nguage-service/src/utils/getVariablesJSONSchema.ts 94.59% <98.38%> (+0.71%) ⬆️

@BPScott
Copy link
Contributor Author

BPScott commented Sep 22, 2023

Yo @acao would you be able to take a look at this?

At the moment loading up vscode with the graphql plugin crashes as soon as I open any non-graphql file which is very disruptive. This fixes the crashing.

@acao
Copy link
Member

acao commented Sep 22, 2023

@BPScott hey there, yes thanks for this bugfix! I'll take a look this weekend and hopefully we can get it merged for next week!

getProjectForFile used to be nullable, I'm not sure when that changed but it may have been myself

getProjectForFile may return void when there is no project for that
file. Wrap the call to updateSchemaIfChanged with a check to ensure that
a project is present.
Copy link
Member

@acao acao left a comment

Choose a reason for hiding this comment

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

finally got a chance to look, thank you @BPScott !
the one last thing that I would absolutely love to have is a unit test confirming this fix if you can provide it 🤞🏻

I might just ship it anyways because this is a nasty bug!

@acao acao merged commit 530ef47 into graphql:main Sep 26, 2023
12 checks passed
@acao acao mentioned this pull request Sep 26, 2023
@BPScott
Copy link
Contributor Author

BPScott commented Sep 26, 2023

Now that getProjectForFile has the correct typing, type-check fails if you don't add the if (project) check, I was hoping that would be enough rather than having to dig into how integration testing worked.

@acao
Copy link
Member

acao commented Sep 26, 2023

@BPScott yes, excellent point! another reason why i love typescript. fix is released as v0.8.19 for vscode-graphql, 2.11.5 for the LSP, enjoy!

@BPScott BPScott deleted the fix-crash branch September 26, 2023 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants