-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Default response content type using GraphQL spec #35769
Conversation
Default response content type header should conform to [GraphQL over HTTP spec](https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#serialization-format).
@jmartisk can you have look ? |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing - you need to also update https://github.com/quarkusio/quarkus/blob/3.4.0.CR1/extensions/smallrye-graphql/runtime/src/main/java/io/quarkus/smallrye/graphql/runtime/SmallRyeGraphQLExecutionHandler.java#L299 - please update it to accept both the new (fixed) content-type and the original one, for backward compatibility
@jmartisk this hasn't made any progress and I think it's really something that needs fixing. Could you have a look? Thanks. |
I've added a commit that hopefully completes the fix |
This comment has been minimized.
This comment has been minimized.
@@ -297,7 +297,8 @@ private static String getCharset(String mimeType) { | |||
private boolean isValidAcceptRequest(String mimeType) { | |||
// At this point we only accept two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good to fix the tests but this comment looks outdated. I wonder if we should explain a bit more and put the two valid ones first and the last one with a comment // deprecated content-type supported for compatibility
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's a very good observation, thanks. Fixed
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. |
@gsmet can you please have another look? I would merge it but I can't as long as there's a -1 from you :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sorry, I was busy on other things, let's merge!
This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [flow-bin](https://github.com/flowtype/flow-bin) ([changelog](https://github.com/facebook/flow/blob/master/Changelog.md)) | devDependencies | minor | [`^0.217.0` -> `^0.219.0`](https://renovatebot.com/diffs/npm/flow-bin/0.217.2/0.219.0) | | [io.hypersistence:hypersistence-utils-hibernate-62](https://github.com/vladmihalcea/hypersistence-utils) | compile | minor | `3.5.3` -> `3.6.0` | | [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `3.4.2` -> `3.4.3` | | [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `3.4.2` -> `3.4.3` | --- ### Release Notes <details> <summary>flowtype/flow-bin</summary> ### [`v0.219.0`](flow/flow-bin@1268ec5...c184c5d) [Compare Source](flow/flow-bin@1268ec5...c184c5d) ### [`v0.218.1`](flow/flow-bin@12afce4...1268ec5) [Compare Source](flow/flow-bin@12afce4...1268ec5) ### [`v0.218.0`](flow/flow-bin@dc93913...12afce4) [Compare Source](flow/flow-bin@dc93913...12afce4) </details> <details> <summary>vladmihalcea/hypersistence-utils</summary> ### [`v3.6.0`](https://github.com/vladmihalcea/hypersistence-utils/blob/HEAD/changelog.txt#Version-360---October-12-2023) \================================================================================ Implement QueryStackTraceLogger using StackWalker [#​659](vladmihalcea/hypersistence-utils#659) Add JFR based query logger [#​658](vladmihalcea/hypersistence-utils#658) Adds support for using MonetaryAmount in [@​ElementCollection](https://github.com/ElementCollection) [#​652](vladmihalcea/hypersistence-utils#652) </details> <details> <summary>quarkusio/quarkus</summary> ### [`v3.4.3`](https://github.com/quarkusio/quarkus/releases/tag/3.4.3) [Compare Source](quarkusio/quarkus@3.4.2...3.4.3) ##### Complete changelog - [#​36408](quarkusio/quarkus#36408) - Ensure that SSE builder works in native - [#​36404](quarkusio/quarkus#36404) - Do not exclude properties from recording that are available in sources that should always be included - [#​36403](quarkusio/quarkus#36403) - IBM Db2 - Register resource bundle classes for reflection - [#​36402](quarkusio/quarkus#36402) - Native Picocli build breaks SSE client - [#​36399](quarkusio/quarkus#36399) - quarkus-jdbc-db2: resource bundle missing - [#​36377](quarkusio/quarkus#36377) - Allow `@ClientHeaderParam` to override User-Agent - [#​36371](quarkusio/quarkus#36371) - Fix issue in Java migration in dev-mode - [#​36351](quarkusio/quarkus#36351) - Properly handle invalid response body errors in Reactive REST Client - [#​36329](quarkusio/quarkus#36329) - Custom User-Agent header ignored - [#​36326](quarkusio/quarkus#36326) - Cannot load fixed or default YAML configuration when running native build - [#​36302](quarkusio/quarkus#36302) - Fix headers and preambles in all guides and reintroduce some keywords - [#​36299](quarkusio/quarkus#36299) - quarkus-flyway: Java-Migration does not get picked up on live reload - [#​36290](quarkusio/quarkus#36290) - Fixed URL for configuring JSON support - [#​36257](quarkusio/quarkus#36257) - Rest client call hangs when receiving an invalid chunked response and does not release resources (e.g. Bulkhead semaphore) - [#​36147](quarkusio/quarkus#36147) - Bump org.eclipse.parsson:parsson from 1.1.2 to 1.1.4 - [#​36096](quarkusio/quarkus#36096) - Build cache - Use notCacheableBecause instead of storeEnabled - [#​35929](quarkusio/quarkus#35929) - Do not store build cache for core extensions having config - [#​35927](quarkusio/quarkus#35927) - Build cache - Core extensions containing config shouldn't be cached - [#​35769](quarkusio/quarkus#35769) - Default response content type using GraphQL spec </details> <details> <summary>quarkusio/quarkus-platform</summary> ### [`v3.4.3`](quarkusio/quarkus-platform@3.4.2...3.4.3) [Compare Source](quarkusio/quarkus-platform@3.4.2...3.4.3) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
So the default content type header was not recognized properly by GraphQL gateway. I tried to submit PR but they said it doesn't conform to the GraphQL spec.