Sort fragments during operation registry operation normalization #1027

trevor-scheer · 2019-02-15T22:22:32Z

Preface

Note: this PR will likely make the most sense if reviewed in commit order.

Now that we've migrated apollo-graphql into this repo (and made some decisions about the state of op. normalization), I'm unblocked from making these changes.

Previous discussion and work for reference:
apollographql/apollo-server#2282

With the expectation that future work will drastically simplify the way we normalize operations for the registry, we came to the consensus to solve an immediate problem now, and make larger changes later on as operation registry v2 becomes ready for implementation.

Purpose

The main focus of this PR is to sort fragments w.r.t operations within a document as one of the steps in sortAST. The important snippet is literally just this:

Document(node: DocumentNode) {
      return {
        ...node,
        // Use sortBy because 'definitions' is not optional.
        definitions: sortBy(node.definitions, "kind", "name.value")
      };
    },

Miscellaneous

Some other changes in this PR that felt appropriate:

DRY up some shared code between client:extract and client:push
Adopt defaultEngineReportingSignature as the only way to do normalization within the CLI

...as opposed to composing the various transformations it encapsulates in an arbitrary way

Export an additional function from apollo-graphql, in order to be prescriptive about how the operation signature hash is generated (hashForOperationSignature)
Adding and modifying tests in order to have some certainty around the effect of this change

TODO

Update apollo-server-operation-registry-plugin to use both:
1. defaultEngineReportingSignature
2. hashForOperationSignature
Update backend to support v2 payload

TODO:

Update CHANGELOG.md* with your change (include reference to issue & this PR)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass

abernix

Real talk, thanks for the incredibly clear PR body and the semantic commits.

This looks pretty great to me. I've left some comments which I think are worth considering within. I think we're pretty clear on applying these same changes to apollo-server-plugin-operation-registry as a PR too. I suspect that any follow-up changes to finalize it (e.g. changing the import-ed function name or matching version) would be be relatively straight forward.

package-lock.json

packages/apollo-graphql/src/signature.ts

abernix · 2019-02-19T19:47:19Z

packages/apollo-graphql/src/signature.ts

@@ -66,3 +66,9 @@ export function defaultEngineReportingSignature(
    )
  );
 }
+
+export function hashForOperationSignature(operationSignature: string): string {


I think we should keep the word signature out of this even if it's already been a pattern. I think documentHash or operationHash might be appropriate. While it was at one time more of a signature (in that it was lossless and not hashed), the signature in the manifest today is actually a hash and I suspect it will remain that way based on the recent discussions we've had.

I realize this suggestion also breaks the name of the file since we're inside signature.ts right now. That said, I don't think we have anything that relies on the current path-based import and this is relatively new so I think we could still rename this file to something else if we wanted. Straw-person: id? (But definitely open to something else).

Resolved in 9dfc76e

I'm for this name change. IMO, id for the filename doesn't give enough information. I went with operationId which still doesn't feel right.

In my mind, id -> identifier. I think what we want to express is that this module is for the verb identification.

operationIdentification? Something else? I'm open to suggestion.

I'd be fine with operationId or operationIdentification, the latter being great if it seems to be better fit in your mind!

packages/apollo-graphql/src/transforms.ts

packages/apollo/src/commands/client/extract.ts

abernix · 2019-02-19T20:09:26Z

packages/apollo/src/commands/client/extract.ts

+          ctx.filename = filename;
+          writeFileSync(
+            filename,
+            JSON.stringify({ version: 1, operations: ctx.operations }, null, 2)


It's possible we should bump this version to 2, even with the current changes.

Reason being: currently, the apollo-server-plugin-operation-registry will throw if the manifest it tries consuming is version !== 1, so this should allow us to ensure that the new apollo-server-plugin-operation-registry which starts consuming this will use the same pre-hash normalization algorithm against incoming operations.

If we do bump this, I definitely think we should change the signature attribute on the individual documents in the operations[] to be sha256Hash as @glasser has proposed.

Going to go forward with the version bump, but for now, changing the shape of operations[] does require a change to the backend (see: RegisteredOperationInput type).

My intuition says to defer this until 2.0 work in order to keep the scope of this work within reason, but I'm open to being convinced otherwise if you believe this is important enough.

Resolved in f7915e1

Deferring the change of signature seems fine. We also need to update the backend (not in open-source land!) to support a different version other than 1.

Added to overall TODOs and will leave this issue unresolved until I address it on the backend 👍

glasser

Overall looking good!

packages/apollo-graphql/src/transforms.ts

packages/apollo/src/utils/getOperationManifestFromProject.ts

glasser · 2019-02-28T00:15:00Z

packages/apollo-graphql/src/operationId.ts

+// should not be depended on in the future, as it's likely we will choose not
+// to hide them at all. The rationale being, if queries are being shipped to
+// a client bundle, exposing PII via this signature is a very small concern,
+// relatively speaking.
 export function defaultEngineReportingSignature(


Can we just not change defaultEngineReportingSignature at all in this PR, especially since it's an interim quick fix?

Resolved in 7714ee1

trevor-scheer · 2019-03-04T19:58:19Z

@abernix the comment about isNodeLike moving to shared logic was easily lost (by me) as a comment on a commit, so I'm adding it here to the PR. I've incorporated the one line change and related fn/file rename.

What do you propose for moving the logic to one place? Would it be appropriate to open a PR against AS to use isNodeLike from apollo-env? I know there was some discussion around the *-env style packages, and I'm not sure if any decisions were made or if alignment was reached there. Any information w.r.t. best path forward would be appreciated!

glasser

The parts of this that I've been paying attention to look good.

chrisbradleydev · 2019-03-12T13:32:53Z

@trevor-scheer any updates on Apollo Operation Registry?

trevor-scheer · 2019-03-13T14:23:29Z

@ctbradley: as we speak, @abernix and I are fully focused on landing this fix!

…. (#1112) * Cutover to apollo-graphql * Add apollo-graphql as a dependency (and project reference) * Remove apollo-engine-reporting as a dependency * Update sortAST to normalize order of fragments w.r.t operations * Tests are failing expectedly at this point * Update tests This update is exactly as expected. All fragments will now move to the beginning of a normalization result. * Use snapshot tests This is a valid use case for snapshots. Even better would be inline snapshots - something we can get to later. * Centralize operation hashing function These two operations will likely be used in tandem, and we want this to be consistent across consumers. * Incorporate rename suggestions * Add explicit comment about sort order * Transform the operation in a less aggressive manner - don't remove aliases and be less strict about removing literals. * Update incorrect reference to renamed module, update related snapshots. * Revert changes to defaultEngineReportingSignature. Apply changes to new function, defaultOperationRegistrySignature. This new function is the effective interim fix, and the current existing function is now left alone. * Add CHANGELOG.md entry for #1112.

1) Remove duplication from client:push and client:extract. 2) Create a test to verify upcoming changes for this PR.

* Add apollo-graphql as a dependency (and project reference) * Remove apollo-engine-reporting as a dependency * Update sortAST to normalize order of fragments w.r.t operations * Tests are failing expectedly at this point

These two operations will likely be used in tandem, and we want this to be consistent across consumers. Incorporate rename suggestions Update version for extracted manifest output. Use empty string and add comment about unused metadata field. Revert changes to defaultEngineReportingSignature. Apply changes to new function, defaultOperationRegistrySignature. This new function is the effective interim fix, and the current existing function is now left alone.

abernix

LG(reat)TM! 🎉

This is a breaking change that will necessitate users of the apollo-server-plugin-operation-registry plugin bump their version of that plugin to a version that matches this normalization technique. We expect that to be apollo-server-plugin-operation-registry@0.1.x-(something) but the exact version is yet to be determined.

Since this is a breaking change this PR intentionally targets the release-apollo-3.x branch where it will live (and we'll open a PR soon to track those changes more specifically.)

Great work, @trevor-scheer!

This includes the work from #1027, #1115 and #1118, which first surfaced in an `apollo@next` CLI version which was released in order to provide a migration path for paying customers who utilize the Apollo Operation Registry through the CLI's `apollo client:push` features. Those customers were notified and advised to either pin their `apollo` version prior to this being released, so the hope is that we'll be able to released this under the `apollo@2` cover without incurring breaking changes on anyone else. The summary of the relevant commit messages is below: 1) Remove duplication from client:push and client:extract. 2) Create a test to verify upcoming changes for this PR. Cutover to apollo-graphql * Add apollo-graphql as a dependency (and project reference) * Remove apollo-engine-reporting as a dependency * Update sortAST to normalize order of fragments w.r.t operations * Tests are failing expectedly at this point Centralize operation hashing function These two operations will likely be used in tandem, and we want this to be consistent across consumers. Incorporate rename suggestions Update version for extracted manifest output. Use empty string and add comment about unused metadata field. Revert changes to defaultEngineReportingSignature. Apply changes to new function, defaultOperationRegistrySignature. This new function is the effective interim fix, and the current existing function is now left alone. Pass operation name along to the operation registry signature function Leverage updated registerOperations API Now that registerOperations supports version as an argument to the mutation, we can leverage this for v2 oeprations manifest work.

This includes the work from #1027, #1115 and #1118, which first surfaced in an `apollo@next` CLI version which was released in order to provide a migration path for paying customers who utilize the Apollo Operation Registry through the CLI's `apollo client:push` features. Those customers were notified and advised to either pin their `apollo` version prior to this being released, so the hope is that we'll be able to released this under the `apollo@2` cover without incurring breaking changes on anyone else. For more information on the operation registry, see: https://www.apollographql.com/docs/platform/operation-registry.html And if you encounter any problems, please contact our customer support via Intercom from within your Engine UI. --- The summary of the relevant commit messages is below: 1) Remove duplication from client:push and client:extract. 2) Create a test to verify upcoming changes for this PR. Cutover to apollo-graphql * Add apollo-graphql as a dependency (and project reference) * Remove apollo-engine-reporting as a dependency * Update sortAST to normalize order of fragments w.r.t operations * Tests are failing expectedly at this point Centralize operation hashing function These two operations will likely be used in tandem, and we want this to be consistent across consumers. Incorporate rename suggestions Update version for extracted manifest output. Use empty string and add comment about unused metadata field. Revert changes to defaultEngineReportingSignature. Apply changes to new function, defaultOperationRegistrySignature. This new function is the effective interim fix, and the current existing function is now left alone. Pass operation name along to the operation registry signature function Leverage updated registerOperations API Now that registerOperations supports version as an argument to the mutation, we can leverage this for v2 oeprations manifest work.

Until the work in apollographql/apollo-tooling#1027 is completed, this will ensure that we can publish this package with the current setup, which I'd rather do for the first time _soon_ rather than _whenever_.

trevor-scheer requested a review from jbaxleyiii as a code owner February 15, 2019 22:22

trevor-scheer force-pushed the trevor/operation-normalization branch 6 times, most recently from 3296fd0 to 9e2fa7d Compare February 15, 2019 22:57

trevor-scheer requested review from abernix, martijnwalraven, glasser and justinanastos February 15, 2019 23:03

abernix requested changes Feb 19, 2019

View reviewed changes

glasser reviewed Feb 19, 2019

View reviewed changes

trevor-scheer force-pushed the trevor/operation-normalization branch 3 times, most recently from be6231f to 6137504 Compare February 27, 2019 21:38

trevor-scheer changed the title ~~[WIP] Interim Operation Normalization Fix~~ Interim Operation Normalization Fix Feb 27, 2019

glasser reviewed Feb 28, 2019

View reviewed changes

trevor-scheer force-pushed the trevor/operation-normalization branch 2 times, most recently from 0f9d90e to 7714ee1 Compare March 4, 2019 19:49

glasser approved these changes Mar 4, 2019

View reviewed changes

trevor-scheer mentioned this pull request Mar 5, 2019

Signature normalization, ordering fragments apollographql/apollo-server#2282

Closed

4 tasks

abernix changed the base branch from master to release-apollo-3.x March 13, 2019 13:42

trevor-scheer mentioned this pull request Mar 13, 2019

Add environment-aware createHash function to apollo-env #1110

Merged

4 tasks

abernix mentioned this pull request Mar 13, 2019

Sort fragments during operation normalization (De-composed from #1027). #1112

Merged

trevor-scheer force-pushed the trevor/operation-normalization branch from 7714ee1 to 131bc40 Compare March 13, 2019 14:54

abernix changed the base branch from release-apollo-3.x to master March 13, 2019 14:56

trevor-scheer added 3 commits March 13, 2019 17:07

Groundwork for updating operation signature normalization

c8de7b4

1) Remove duplication from client:push and client:extract. 2) Create a test to verify upcoming changes for this PR.

Cutover to apollo-graphql

5d11f79

* Add apollo-graphql as a dependency (and project reference) * Remove apollo-engine-reporting as a dependency * Update sortAST to normalize order of fragments w.r.t operations * Tests are failing expectedly at this point

abernix force-pushed the trevor/operation-normalization branch from 131bc40 to ad47aa1 Compare March 13, 2019 15:13

abernix changed the base branch from master to release-apollo-3.x March 13, 2019 15:13

abernix approved these changes Mar 13, 2019

View reviewed changes

trevor-scheer merged commit 1fac342 into release-apollo-3.x Mar 13, 2019

trevor-scheer deleted the trevor/operation-normalization branch March 13, 2019 15:22

abernix mentioned this pull request Mar 13, 2019

Release apollo@3 (alpha). #1113

Closed

abernix changed the title ~~Interim Operation Normalization Fix~~ Sort fragments during operation registry operation normalization. Mar 19, 2019

abernix changed the title ~~Sort fragments during operation registry operation normalization.~~ Sort fragments during operation registry operation normalization Mar 19, 2019

abernix mentioned this pull request Mar 19, 2019

client:push operation is re-ordering document definitions #986

Closed

abernix mentioned this pull request Apr 1, 2019

Update operation normalization to deterministically sort fragments. #1158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort fragments during operation registry operation normalization #1027

Sort fragments during operation registry operation normalization #1027

trevor-scheer commented Feb 15, 2019 •

edited

Loading

abernix left a comment •

edited

Loading

abernix Feb 19, 2019

trevor-scheer Feb 27, 2019

abernix Mar 4, 2019

abernix Feb 19, 2019

trevor-scheer Feb 27, 2019

trevor-scheer Feb 27, 2019

abernix Mar 4, 2019 •

edited

Loading

trevor-scheer Mar 4, 2019

glasser left a comment

glasser Feb 28, 2019

abernix Mar 4, 2019

trevor-scheer Mar 4, 2019

trevor-scheer commented Mar 4, 2019

glasser left a comment

chrisbradleydev commented Mar 12, 2019

trevor-scheer commented Mar 13, 2019

abernix left a comment

Sort fragments during operation registry operation normalization #1027

Sort fragments during operation registry operation normalization #1027

Conversation

trevor-scheer commented Feb 15, 2019 • edited Loading

Preface

Purpose

Miscellaneous

TODO

abernix left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abernix Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevor-scheer commented Mar 4, 2019

glasser left a comment

Choose a reason for hiding this comment

chrisbradleydev commented Mar 12, 2019

trevor-scheer commented Mar 13, 2019

abernix left a comment

Choose a reason for hiding this comment

trevor-scheer commented Feb 15, 2019 •

edited

Loading

abernix left a comment •

edited

Loading

abernix Mar 4, 2019 •

edited

Loading