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

Add support for Automated Persisted Queries #601

Conversation

SirensOfTitan
Copy link

Description

Sorry, I didn't quite follow the contributing guidelines for this, as I want to start testing this in our app now.

This introduces APQ to apollo-ios by:

  1. Introducing a APQNetworkTransport class intended to wrap an HTTPNetworkTransport and WebsocketTransport.
  2. Introducing a couple optional flags to send for most queries: includeQuery and extensions, the latter of which is a GraphQLMap.
  3. Introducing CryptoSwift as a dependency for sha256. Happy to switch over to some bridge using CommonCrypto or by only importing some sha256 implementation.

Additionally, I noticed that GETs don't work right now as advertised for any variables payload with nil, primarily around:
https://github.com/apollographql/apollo-ios/blob/master/Sources/Apollo/HTTPNetworkTransport.swift#L178

...because the .jsonObject accessor is called on variables, which encodes nils to NSNull, then the JSONSerializer calls .jsonObject again, leading to a fatal as NSNull is not JSONSerializable.

I fixed that up by just ensuring we don't call .jsonObject for items passed to JSON serializable. I tested this by applying the patch just to apollo-ios trunk and running a couple tests using GET.

re:formatting: there are some places in the code that use 2 spaces and some that use 4, unsure if there's a standard here.

I'm also presenting this PR as a branch off of my fork of apollo-ios. It should merge cleanly regardless of my changes to master, but if not I can rebase against upstream.

Test Plan

I've just started running apollo-ios against a project of mine to get some basic anecdotal signal that the app works as expected. So far so good, although I haven't really validated against websockets yet.

I'm very happy to add tests after the approach here is accepted. I would like to minimally add tests for:

  • Starwars E2E tests for HTTP/Websocket APQ.
  • Basic additional tests for HTTP GET. I'm sort of wondering how we might want to handle long URLs gracefully for GET requests, as servers could have limits on length for GET (I'm unsure if apollo-server does)

@apollo-cla
Copy link

@SirensOfTitan: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@designatednerd
Copy link
Contributor

Heads up that #599 and #602 are making some significant changes to this stack, you may want to wait until those get merged.

@SirensOfTitan
Copy link
Author

@designatednerd: okie dokie! I hacked this up quickly for myself, happy to adjust for upstream changes or help with the other PR that adds this if you all are happier with that direction.

@designatednerd
Copy link
Contributor

Hey @SirensOfTitan it looks like things have moved on quite a bit further with #608 - do you mind if we go ahead with that one and close this one out?

@SirensOfTitan
Copy link
Author

SirensOfTitan commented Jul 11, 2019

@designatednerd Sure! Whatever works best for you! I can also rebase this if you'd like, whatever works! Codebear's diff is great.

Thoughts that are a totally ignore if not helpful or relevant:

  • I like that the other diff uses codegen to pre-calculate the sha256 hashes, as sha256 is a relatively expensive hash to execute compared to its peers and there's very little reason to do this at runtime.
  • I favor my approach in the use of a separate network transport that wraps the terminating HTTP/Websocket transport. The composition model would allow for easier future support of queries over websocket or other transports and offers a looser coupling between persisted queries and the transport by which the client communicates with the server.

@designatednerd
Copy link
Contributor

Cool, thank you for the feedback!

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.

3 participants