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.

Sign up for GitHub

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

Jump to bottom

better error message for related errors #3389

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

better error message for related errors #3389

wants to merge 4 commits into from

Conversation

EmrysMyrddin
Copy link
Collaborator

No description provided.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 26, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 66f8e74

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

This PR includes changesets to release 23 packages
Name Type
graphql-yoga Patch
@graphql-yoga/nestjs Patch
@graphql-yoga/render-graphiql Patch
@graphql-yoga/plugin-apollo-inline-trace Patch
@graphql-yoga/apollo-managed-federation Patch
@graphql-yoga/plugin-apq Patch
@graphql-yoga/plugin-csrf-prevention Patch
@graphql-yoga/plugin-defer-stream Patch
@graphql-yoga/plugin-disable-introspection Patch
@graphql-yoga/plugin-graphql-sse Patch
@graphql-yoga/plugin-jwt Patch
@graphql-yoga/plugin-persisted-operations Patch
@graphql-yoga/plugin-prometheus Patch
@graphql-yoga/plugin-response-cache Patch
@graphql-yoga/plugin-sofa Patch
apollo-federation-gateway-with-yoga Patch
apollo-subgraph-with-yoga Patch
graphql-lambda Patch
cloudflare-advanced Patch
cloudflare Patch
nextjs-app Patch
hello-world-benchmark Patch
@graphql-yoga/nestjs-federation 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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 26, 2024 •
edited
Loading

Apollo Federation Subgraph Compatibility Results

Federation 1 Support Federation 2 Support
_service🟢
@key (single)🟢
@key (multi)🟢
@key (composite)🟢
repeatable @key🟢
@requires🟢
@provides🟢
federated tracing🟢
@link🟢
@shareable🟢
@tag🟢
@override🟢
@inaccessible🟢
@composeDirective🟢
@interfaceObject🟢

Learn more:

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 26, 2024 •
edited
Loading

💻 Website Preview

The latest changes are available as preview in: https://25649a3f.graphql-yoga.pages.dev

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 26, 2024 •
edited
Loading

✅ Benchmark Results

     ✓ no_errors{mode:graphql}
     ✓ expected_result{mode:graphql}
     ✓ no_errors{mode:graphql-jit}
     ✓ expected_result{mode:graphql-jit}
     ✓ no_errors{mode:graphql-response-cache}
     ✓ expected_result{mode:graphql-response-cache}
     ✓ no_errors{mode:graphql-no-parse-validate-cache}
     ✓ expected_result{mode:graphql-no-parse-validate-cache}

     checks.......................................: 100.00% ✓ 411548      ✗ 0     
     data_received................................: 1.7 GB  14 MB/s
     data_sent....................................: 83 MB   693 kB/s
     http_req_blocked.............................: avg=1.53µs   min=982ns    med=1.34µs   max=5.31ms   p(90)=2µs      p(95)=2.2µs   
     http_req_connecting..........................: avg=2ns      min=0s       med=0s       max=135.61µs p(90)=0s       p(95)=0s      
     http_req_duration............................: avg=369.55µs min=220.37µs med=331.13µs max=18.99ms  p(90)=469.05µs p(95)=488.49µs
       { expected_response:true }.................: avg=369.55µs min=220.37µs med=331.13µs max=18.99ms  p(90)=469.05µs p(95)=488.49µs
     ✓ { mode:graphql-jit }.......................: avg=294.75µs min=220.37µs med=272.75µs max=18.99ms  p(90)=302.73µs p(95)=315.79µs
     ✓ { mode:graphql-no-parse-validate-cache }...: avg=494.49µs min=402.2µs  med=464.78µs max=9.49ms   p(90)=505.59µs p(95)=539.72µs
     ✓ { mode:graphql-response-cache }............: avg=348.79µs min=275.33µs med=329.15µs max=8.6ms    p(90)=359.22µs p(95)=370.54µs
     ✓ { mode:graphql }...........................: avg=374.8µs  min=281.36µs med=340.96µs max=14.15ms  p(90)=388.3µs  p(95)=425.43µs
     http_req_failed..............................: 0.00%   ✓ 0           ✗ 205774
     http_req_receiving...........................: avg=34.66µs  min=15.6µs   med=34.31µs  max=3.12ms   p(90)=40.14µs  p(95)=42.31µs 
     http_req_sending.............................: avg=8.4µs    min=6.07µs   med=7.38µs   max=294.66µs p(90)=11µs     p(95)=12.04µs 
     http_req_tls_handshaking.....................: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting.............................: avg=326.48µs min=188.54µs med=288.54µs max=18.84ms  p(90)=425.96µs p(95)=443.03µs
     http_reqs....................................: 205774  1714.757013/s
     iteration_duration...........................: avg=578.18µs min=398.72µs med=536.92µs max=19.56ms  p(90)=680.96µs p(95)=706.09µs
     iterations...................................: 205774  1714.757013/s
     vus..........................................: 1       min=1         max=1   
     vus_max......................................: 2       min=2         max=2

@EmrysMyrddin EmrysMyrddin force-pushed the better-operation-name-errors branch from d660ad3 to 56eff8c Compare July 29, 2024 14:09
EmrysMyrddin
EmrysMyrddin commented Jul 29, 2024
View reviewed changes
packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts
Comment on lines +23 to +35
if (
'operationName' in params &&
typeof params.operationName !== 'string' &&
params.operationName != null
) {
throw createGraphQLError(`Invalid operation name in the request body.`, {
extensions: {
http: {
status: 400,
},
},
});
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This part was missing but required in the http spec. It was caught before by chance...

Copy link
Collaborator

Choose a reason for hiding this comment

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

How can we ensure we don't introduce a regression? How was this cattched by chance?

EmrysMyrddin
EmrysMyrddin commented Jul 29, 2024
View reviewed changes
packages/graphql-yoga/src/plugins/request-validation/use-check-graphql-query-params.ts
Comment on lines +195 to +205
export function isValidOperationName(
operationName: string | undefined,
document: DocumentNode,
): boolean {
try {
checkOperationName(operationName, document);
return true;
} catch {
return false;
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure it's needed, just wanted to implement the same pattern that was used for the check of the params

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this function used?

@EmrysMyrddin EmrysMyrddin requested a review from ardatan July 29, 2024 15:31
n1ru4l
n1ru4l reviewed Aug 3, 2024
View reviewed changes
.changeset/rude-squids-accept.md
'graphql-yoga': patch
---

Improve error messages in case of `operatinName` related errors.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the error message before?
I also don't see any new tests that ensure that the new error message is used and a regression in the future is avoided.

n1ru4l
n1ru4l requested changes Aug 3, 2024
View reviewed changes
Copy link
Collaborator

@n1ru4l n1ru4l left a comment

Choose a reason for hiding this comment

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

see my comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@n1ru4l n1ru4l n1ru4l requested changes

@ardatan ardatan Awaiting requested review from ardatan

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@EmrysMyrddin @n1ru4l