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

feature: Adds support for GraphQL over HTTP media type #558

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

Conversation

calvincestari
Copy link
Member

@calvincestari calvincestari commented Dec 16, 2024

Closes apollographql/apollo-ios#3485.

  1. Make sure we include the new type in the 'accept' request header.
  2. Update tests for new content type.
  3. There were no changes needed for error handling because we do not throw when the response status code falls outside the 200..<300 range.
  4. Our non-multipart response parser does not require that the content type actually be application/json. Instead it simply attempts to deserialize anything as if it is JSON. It's only in the multipart parsers that we do a content type check for the chunks. @AnthonyMDev - this hasn't been an issue for now but do you think we should add this check? I'm inclined to leave the standard parser as it is.

@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Dec 16, 2024

✅ Docs Preview Ready

No new or changed pages found.

Copy link

netlify bot commented Dec 16, 2024

Deploy Preview for apollo-ios-docc canceled.

Name Link
🔨 Latest commit 78085ef
🔍 Latest deploy log https://app.netlify.com/sites/apollo-ios-docc/deploys/6760c10fadb456000869bbf6

Copy link

netlify bot commented Dec 16, 2024

Deploy Preview for eclectic-pie-88a2ba ready!

Name Link
🔨 Latest commit 78085ef
🔍 Latest deploy log https://app.netlify.com/sites/eclectic-pie-88a2ba/deploys/6760c10fc341750008c8c058
😎 Deploy Preview https://deploy-preview-558--eclectic-pie-88a2ba.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@calvincestari
Copy link
Member Author

calvincestari commented Dec 16, 2024

I'm going to add a couple tests of our request chain to ensure that the assumption we make on content type holds true.

Done.


// This test is odd because you might assume it would fail but there is no content-type checking on standard
// GraphQL response parsing. So this test is here to ensure that existing behaviour does not change.
func test__response__givenUnknownContentType_shouldNotFail() throws {
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added tests to keep the existing behaviour of standard GraphQL response parsing including this odd one.

Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

Tests all look good. I just thing that we should clean the repeated string constants up with a helper method.

@@ -59,7 +59,7 @@ struct MultipartResponseDeferParser: MultipartResponseSpecificationParser {
for dataLine in chunk.components(separatedBy: Self.dataLineSeparator.description) {
switch DataLine(dataLine.trimmingCharacters(in: .newlines)) {
case let .contentHeader(type):
guard type == "application/json" else {
guard type == "application/graphql-response+json" || type == "application/json" else {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should create a helper method for this. We're doing string comparisons against strings that are really constants. And we're doing the same comparison in two places. This is prone to future bugs.

We should store the constants as StaticStrings and have a helper method to check the content header.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll put this PR back into draft and make that change when I get back.

@calvincestari calvincestari marked this pull request as draft December 21, 2024 04:10
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.

feature: Implement GraphQL response type
3 participants