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

Parse extend schema correctly when root operations list is absent #3670

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

trevor-scheer
Copy link
Contributor

@trevor-scheer trevor-scheer commented Jul 31, 2024

The online parser currently expects to parse a schema extension the same way it parses a schema definition, however the rules vary slightly in that the list of root operations can be omitted.

This change treats them as two distinct rules, allowing the list to be optional in extend case.

Copy link

changeset-bot bot commented Jul 31, 2024

🦋 Changeset detected

Latest commit: b0942f9

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

This PR includes changesets to release 2 packages
Name Type
graphql-language-service Patch
codemirror-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

@trevor-scheer trevor-scheer changed the title Add tests for parsing schema extensions Reproduction for extend schema parsing issue Jul 31, 2024
Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.35%. Comparing base (9f725b8) to head (b0942f9).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3670      +/-   ##
==========================================
+ Coverage   65.32%   65.35%   +0.02%     
==========================================
  Files         122      122              
  Lines        7003     7003              
  Branches     2260     2265       +5     
==========================================
+ Hits         4575     4577       +2     
+ Misses       2411     2409       -2     
  Partials       17       17              
Files with missing lines Coverage Δ
...kages/graphql-language-service/src/parser/Rules.ts 97.27% <ø> (+1.81%) ⬆️

@trevor-scheer trevor-scheer changed the title Reproduction for extend schema parsing issue Parse extend schema correctly when root operations list is absent Sep 30, 2024
@trevor-scheer
Copy link
Contributor Author

@acao @yaacovCR would either of you be the right person to ping to get eyes on this? I've just updated this PR from a reproduction to a fix and would love to get this reviewed. Thanks in advance!

@yaacovCR
Copy link
Contributor

yaacovCR commented Oct 2, 2024

Sorry @trevor-scheer — I am not familiar with the code base here. Just have contributed what I could to get incremental delivery moving…

@trevor-scheer
Copy link
Contributor Author

Hey @acao, is there anything I can do to help move this along?

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

This looks correct to me, the tests look sensible.

However; it looks like the same issue applies to object, interface, enum and input object extensions?

Actually... for those, it looks like the {...} should be optional in all cases (extension and original definition), though they appear to be required in this parser definition? It's not immediately obvious to me why schema doesn't follow that same pattern, e.g. if you can have

input Foo @oneof
scalar Bar
extend input Foo {
  int: Int
  bar: Bar
}

why could you not also have:

schema @blah
type Query {
  int: Int
}
extend schema {
  query: Query
}

Perhaps this warrants a spec tweak. That's out of scope for this PR though!

I'm unfamiliar with the parser, so I defer to someone more familiar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants