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

Persisted operations + GCS integration improvements #13

Merged
merged 10 commits into from
Jan 5, 2024

Conversation

rickbijkerk
Copy link
Collaborator

@rickbijkerk rickbijkerk commented Jan 4, 2024

A MR with small tweaks i had to do to make persisted operations work together with a GCS bucket
changes include:

  • No longer forwarding 'operationName'
  • Minor tweaks in documentation regarding fetching from GCS bucket
  • Properly reading the gcs downloaded files from disk by including the directory

Rick Bijkerk added 6 commits January 3, 2024 11:06
…operationName at hand and sending an empty string towards our upstream results in an error.

For now we just delete until we learn why we need it
…rsisted queries files

Introduce specific parsing of apollo-persisted-query-manifest
@ldebruijn
Copy link
Owner

What is the need for implementing https://www.apollographql.com/docs/react/api/link/persisted-queries/ the Apollo Persisted Queries manifest? It's a proprietary format only supported by the Apollo Router and go-graphql-armor aims to be agnostic to these formats? Any reason why persisted operations cannot be supplied in the format as described in: https://github.com/ldebruijn/go-graphql-armor/blob/main/docs/persisted_operations.md#parsing-structure ?

@rickbijkerk
Copy link
Collaborator Author

rickbijkerk commented Jan 4, 2024

Let me work on the failing tests

@ldebruijn
Copy link
Owner

ldebruijn commented Jan 4, 2024

If you check out the following resources these expect & generate perstisted operations in the already implemented map[string]string format, and not the Apollo PO specification.

https://the-guild.dev/graphql/codegen/plugins/presets/preset-client#persisted-documents
https://github.com/graphile/persisted-operations

@rickbijkerk
Copy link
Collaborator Author

rickbijkerk commented Jan 4, 2024

If you check out the following resources these expect & generate perstisted operations in the already implemented map[string]string format, and not the Apollo PO specification.

https://the-guild.dev/graphql/codegen/plugins/presets/preset-client#persisted-documents https://github.com/graphile/persisted-operations

The problem with that is that the hashes generated by the tooling above dont seem to match the hashes that apollo seemed to generated. Most likely due to new lines added in places and we didnt want to go there to fix it. Which is why we opted for this approach.

Another solution however (and i think more in line with your view) would be to do some pre-processing before pushing it to the bucket so that we have a hash,query pair like you described above, without any formatting leaking into this project. Which might be the nicer option as it also gives us the freedom to use whatever client we want as long as the generated hashes match what a client sends.
This also doesnt attach us too much to apollo, which is a pro :)

Thoughts?

@ldebruijn
Copy link
Owner

Yes I do prefer the simpler non-vendor option of map[string]string and optionally having pre-processing before uploading to the bucket. This removes tying this solution to specific (set of) clients/generators and since the format is extremely simple anyone can use it.

@rickbijkerk rickbijkerk self-assigned this Jan 4, 2024
@ldebruijn
Copy link
Owner

Could you add a description to your MR and update the title as it no longer touches Persisted Ops in that sense. That way it'll show up correctly on the generated changelog

@rickbijkerk
Copy link
Collaborator Author

Could you add a description to your MR and update the title as it no longer touches Persisted Ops in that sense. That way it'll show up correctly on the generated changelog

sure thing, will look into that tomorrow!

@rickbijkerk rickbijkerk changed the title Feature/improvement persisted operations Persisted operations + GCS integration improvements Jan 5, 2024
@ldebruijn
Copy link
Owner

Perfect, thanks for your contribution @rickbijkerk !

@ldebruijn ldebruijn merged commit 5e2e062 into main Jan 5, 2024
2 checks passed
@ldebruijn ldebruijn deleted the feature/improvement_persisted_operations branch January 5, 2024 15:55
ldebruijn pushed a commit that referenced this pull request Jan 23, 2024
A MR with small tweaks i had to do to make persisted operations work
together with a GCS bucket
changes include: 
- No longer forwarding 'operationName'
- Minor tweaks in documentation regarding fetching from GCS bucket
- Properly reading the gcs downloaded files from disk by including the
directory

---------

Co-authored-by: Rick Bijkerk <[email protected]>
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.

2 participants