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

Update field collection to allow inline fragment without type #3384

Merged
merged 12 commits into from
Nov 22, 2024

Conversation

phughes-scwx
Copy link
Contributor

@phughes-scwx phughes-scwx commented Nov 21, 2024

Updates the algorithm for field collection to hew more closely to the version in the spec described in https://spec.graphql.org/June2018/#sec-Field-Collection. Specifically, it:

  • only checks if the fragment type (for inline fragments) applies to the list of satisfying types if it is defined;
  • handles the use of @include and @skip directives only after type application is performed (for inline and spread fragments).

This issue was identified in relation to the use of @defer on an inline fragment without a type condition, as described in #1972 . Because @defer is obscure and this was hard to recreate / test, this PR also adds an _example server that can handles deferred responses correctly.

I am not sure if there should be any documentation updates related to this.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@phughes-scwx phughes-scwx changed the title Draft: Updates field collection algorithm to allow fragments Draft: Updates field collection to allow inline fragment without type Nov 21, 2024
@phughes-scwx phughes-scwx changed the title Draft: Updates field collection to allow inline fragment without type Draft: Update field collection to allow inline fragment without type Nov 21, 2024
@phughes-scwx phughes-scwx marked this pull request as draft November 21, 2024 23:52
@coveralls
Copy link

coveralls commented Nov 22, 2024

Coverage Status

coverage: 59.754% (+0.2%) from 59.601%
when pulling 37acabd on phughes-scwx:issue-1972
into 1040b92 on 99designs:master.

@StevenACoffman
Copy link
Collaborator

Thanks! I'm extremely excited about the _example server addition, as it has been tricky to figure out how @defer is interacting with some other edge cases.

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Nov 22, 2024

Odd that the Go 1.22 Integration / federation check failed, but the Go 1.23 did not. I'm assuming it's just flaky and not related to these changes, so that's annoying. (If anyone sees anything obvious, I would appreciate you telling me!)

@phughes-scwx
Copy link
Contributor Author

Hm, yeah, odd. I'll continue to look. Are there any changes / upates you'd like me to make to the example or any docs? Or any additional tests for collection? If not, I'll remove the draft description after trying to pinpoint the 1.22 failures.

@phughes-scwx
Copy link
Contributor Author

Looks like a similar issue to #2060 / #2409

@phughes-scwx
Copy link
Contributor Author

phughes-scwx commented Nov 22, 2024

@StevenACoffman after a few more passes and some tests, I think this is just flaky due to a slow startup, associated with the calls out to sleep in the integration test:

We aren't really pausing for some meaningful signal, so might just get out of sync if the system is running slowly. (I could not replicate it.)

Are there any changes / updates you'd like me to make to the example or any docs? Or any additional tests for field collection? If not, I'll remove the draft description and hopefully the tests all pass when we trigger them again? 🤷

@StevenACoffman StevenACoffman marked this pull request as ready for review November 22, 2024 21:46
@StevenACoffman
Copy link
Collaborator

Thanks for looking at those flaky tests. It's annoying, but not this PR's concern.

Currently, for @defer the executable_schema.go has no test coverage for line 99 or lines 189-192 which means we cannot verify whether other changes accidentally break the correct behavior.

I'm not going to hold up merging this PR for that now, as it's wasn't your changes that caused those to be uncovered, but I would gladly merge any later PR that added more test coverage for them if you get a chance.

@StevenACoffman StevenACoffman changed the title Draft: Update field collection to allow inline fragment without type Update field collection to allow inline fragment without type Nov 22, 2024
@StevenACoffman StevenACoffman merged commit be20ed5 into 99designs:master Nov 22, 2024
16 of 17 checks passed
@phughes-scwx
Copy link
Contributor Author

Yeah sure I'll do a follow up next week.

@phughes-scwx phughes-scwx deleted the issue-1972 branch November 25, 2024 21:06
@StevenACoffman StevenACoffman added the defer-or-stream Related to GraphQL @defer or @stream directives label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defer-or-stream Related to GraphQL @defer or @stream directives
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants