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

Extracted query list should include a hash that is compatible with apollo-link-persisted-queries #1117

Closed
abumalick opened this issue Mar 14, 2019 · 7 comments
Labels
🔨 cli related to the CLI itself

Comments

@abumalick
Copy link

abumalick commented Mar 14, 2019

Description of the problem

I am trying to implement persisted queries in our app. To accomplish this I need two things:

  • Generate a list of all queries, with a hash of each query
  • Apollo client should send the hash instead of the query at run time

We can generate the query list with apollo client:extract. And we can use apollo-link-persisted-queries to send the hash with apollo-client.

The problem is that the hash generated by apollo client:extract don't match the hash generated by apollo-link-persisted-queries.

It seems that apollo cli modifies the query before generating the hash (it removes new lines characters and add __typename to every nodes), I think it is the reason why the hashes differs.

Here is a repository that reproduces the problem:
https://github.com/abumalick/persisted-queries-showcase

An overview of the suggested solution

My proposition is to enhance apollo cli to include a hash of the original query (before modification) and add it in the final output. Something like:

{
  "version": 1,
  "operations": [
    {
      "sha256Hash": "14ac5ae57bd5679435b8e80f00e57c054e2ecaf7034b97e45adf6830c731afbb",
      "signature": "638a6e757933d0e452d6bcf2e9745b4fb4082744b54c19ca094d09862c56a593",
      "document": "query getRates{rates(currency:\"\"){__typename currency rate}}",
      "metadata": {
        "engineSignature": "query getRates{rates(currency:\"\"){__typename currency rate}}"
      }
    }
  ]
}

I can try to work on a PR if you are interested by this change.

Is there a workaround ?

I tried hard to find a workaround, and I couldn't find any.

The best way I found is to use the nearly deprecated persistgraphql, and made some modifications that created a hash that matched the hash of apollo-link-persisted-queries. obytes/persistgraphql@3132aae#diff-2f052981d7b286ebc10c6833040e3242R177

The problem with this solution, is that it does not add the __typename to the queries, and apollo-client won't work if __typename is not present in the api response. (edit: I just found out --add_typename option) Also I was disappointed to use a nearly deprecated package.

ref apollographql/apollo-link-persisted-queries#28

@abumalick
Copy link
Author

Any feedback on this issue ? @martijnwalraven @shadaj @jbaxleyiii @abernix @JakeDawkins @evans

It is really blocking us.

I would be very happy to work on this if you don't have time.

@JakeDawkins
Copy link
Contributor

@abumalick I passed this around internally for others to take a look. I think it's something we should address, I just want someone with a little more context to validate the approach!

@JakeDawkins JakeDawkins added the 🔨 cli related to the CLI itself label Mar 18, 2019
@abernix
Copy link
Member

abernix commented Mar 18, 2019

@abumalick Persisted queries (setup guide found here) don't necessitate this list of operations; this operation manifest is intended to be used with the operation registry.

I don't want to suggest that there's no need for what you're saying, but I don't understand the use-case since persisted queries should only require installing the apollo-link-persisted-queries link, per the setup guide I linked above.

@abumalick
Copy link
Author

abumalick commented Mar 18, 2019

Thank you @JakeDawkins !

@abernix We are trying to implement persisted queries, it is not the same than automatic persisted queries. We want to whitelist the operations. I think that automatic persisted queries does not support whitelisting, does it ? Operations are not sent to the server at build time.

The process is explained here: https://blog.apollographql.com/persisted-graphql-queries-with-apollo-client-119fd7e6bba5 but we want to attach the hash to the query list as it is explained here: https://kevinjalbert.com/graphql-persisted-queries-with-http-caching-part-2/ .

We want to:

  • At build time we create a list with all operations, with the hash that will be sent by the app
  • CI sends that list to our proxy
  • the proxy will listen for requests from the frontend, read the hash and attach the corresponding operation/query if the hash is known
  • the proxy will transfer the requests to the backend (if it found an operation for the hash in the request)

We are not currently using Apollo server, I don't know if we can implement an operation registry in our current backend. Our backend use graphene and we are implementing a separate proxy to handle persisted queries (matches the hash with the query).

Is there a better way to implement persisted queries with whitelisting support ?

@abumalick
Copy link
Author

any news on this @abernix @JakeDawkins

I would be very api to work on that if you would accept a PR.

@abernix
Copy link
Member

abernix commented Apr 25, 2019

I really appreciate your offer to help, but persisted queries and safe-listing are, in our minds, currently two separate features: Persisted queries is for performance and safe-listing is for security.

While there might be some overlap in the approaches, we're not certain that combining these is the correct approach and we can see some concrete divergence in terms of implementation of the two, particularly for public versus private APIs but also in terms of management of the operation manifest over several generations of a client's codebase. e.g. Even though version 2 of a mobile app introduces different operations, it doesn't mean that the operations from version 1 should stop working.

The apollo client:extract command is currently designed to work with the Apollo Platform Operation Registry, a feature which is provided currently as part of the Apollo Platform. The manifest which is output by apollo client:extract is actually a command provided mainly for debugging and visibility purposes that produces a manifest identical to the manifest which is uploaded to Apollo Engine by the apollo client:push command (which also accepts additional parameters, like the name of the client, the client version and other identifying information like a Git commit hash). Apollo Engine then takes care of hosting the manifest and making it available to all instances of Apollo Server which are configured with the apollo-server-plugin-operation-registry plugin. (While we're currently focused on the Node.js server, I think other servers could stand to benefit from such a plugin as well.)

Most importantly though, in terms of the technical implementation, the SHA-256 hash (i.e sha256Hash) which is present in the extensions is a runtime hash digest of the operation. Obtaining it via static analysis in the way that you're suggesting is problematic for many use cases. While this may work for your exact use-case, many codebases have patterned themselves around sharing fragments between multiple components (which is very enticing and arguably a great thing to do!) and the on-disk code will likely be in the form of, for example, two files: fragments.js and component.jsx:

// fragments.js
const ImageFragment = gql`
  fragment ImageFragment on Image {
    width
    height
  }
`;

export const MySharedProductFragment = gql`
  fragment MySharedProductFragment on Products {
    name
    image {
      ...ImageFragment
    }
    ${ImageFragment}
  }
`;
// component.jsx
import { MySharedProductFragment } from "../shared/fragments";
// ...
  const query = gql`
    query myQuery {
      products {
        ...MySharedProductFragment
      }
    }

    ${MySharedProductFragment}
  `;

Trying to statically generate the exact same hash for the above myQuery query and make it match that which will arrive at runtime, is not straightforward. Consider a case where the fragments were function calls, or if the fragments were dynamically import-ed or Promises that are only resolved at runtime! There are a lot of static vs runtime considerations to make, particularly as you consider what various build tools and (e.g. Babel) plugins might do to a client bundle between the state it's in while it's on-disk during development and the state it's in at runtime. There are also subtle differences in the operations themselves (e.g. spacing, aliases, etc.) which shouldn't necessarily render an operation un-executable (e.g. Should a minifier's changing whitespace alter an operation's executability?).

For this, and many other reasons, apollo client:extract and apollo client:push use a series of normalizations that allow them to arrive at the same hash as Apollo Server does when it receives an operation from a client. For Apollo Server, when persisted queries are enabled, this means that the exact operation is first obtained via the APQ sha256Hash handshaking (first consulting its cache, then asking the client to send the full operation if it's not found there), then the normalization is performed on that which allows it to arrive at the same signature as was present in the manifest. In the future, this will also allow us to provide observability into safe-listed operations within the Apollo Engine UI and build tooling that allows teams to retire operations from the safe-list when the time comes (e.g. after the 12 legacy versions of a client are no longer sending data!).

I'm afraid that you're ultimately asking to add what is not going to be a stable hash for many cases or doesn't match up with the best-practices we see developing. Right now, if we added this as you're proposing it, I think we'd have a high rate of operations that would fail to execute in production. We don't want to steer implementors wrong, particularly in regards to a security feature, and we think decoupling APQ from the operation registry has benefits.

We will continue to evaluate how to provide this functionality in the future, however for now, there are a number of reasons which I think support not including this. With the above warnings, you're of course welcome to use the community persistedgraphql tooling which you've already found available, particularly if it works for your use-case!

@abernix abernix closed this as completed Apr 25, 2019
@abumalick
Copy link
Author

Thank you a lot for your detailed response @abernix .

I really appreciate the time and informations that you have put into your answer. It actually helped me a lot understanding what is your recommended way of implementing safe-listing and your reasons of doing it this way. Now I understand the benefits of normalizing the operations.

As I wish to use your tooling, I think our options for implementing safe-listing are very clear now:

  • use apollo-server as a proxy for our backend with the operation registry
  • or implement the same normalizations in our backend

Thank you again for your explanation, it was very interesting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 cli related to the CLI itself
Projects
None yet
Development

No branches or pull requests

3 participants