Allow persisting to also include the query text for safe migration #3917

tomgasson · 2022-05-19T05:16:59Z

Hi folks.

We're attempting to start using persisted queries. However, we're not 100% confident in our ability to switch over to them in one big step safely.

Fetching the persisted documents is a new capability for our GraphQL server. If something goes wrong fetching the document, or if it introduces unexpected latency to our existing production queries we want to be able to fallback to the query text until we're more confident in its maturity.

Relay currently supports outputting the query text, or outputting the persisted document id. This PR introduces a middle state, where the text and id are both output. When both are sent with the GraphQL request from the client to the server, the server is able to control the rollout of persisted queries via a feature flag.

This PR introduces a safeMigration bool to the remote persist config, which when set will also output the query text in the operation params as well as the id

persistConfig: {
  url: ...,
  params: ...,
  includeQueryText: true
}

The benefits of persisted queries (saving the network bytes) won't come until we've progressed past the migration stage, but it makes it possible to do the move in the first place.

maraisr · 2022-05-19T05:21:47Z

Love it @tomgasson good moment this.

Summary: We noticed that some of our query text contains a significant amount of unnecessary whitespace. Unlike server->client requests, client->server requests generally aren't compressed and would require a js compression library to do so. This PR adds a feature flag to compact the query text. I saw the pre-existing `compact` mode wasn't wired up to anything, so I've co-opted it. As an example of the benefit, we have one query which this shrinks from `84kb` to `31kb` We're slowly [making our way to persisted queries](#3917), but this is an immediate improvement and I imagine we're not the only ones who will benefit from smaller network payloads Happy to make this a config rather than a feature flag, if you don't think this should be the default output Pull Request resolved: #3983 Test Plan: Imported from GitHub, without a `Test Plan:` line. Static Docs Site previews have moved into the custom phabricator field "Static Docs", and will no longer modify test plans after 5th October 2022. Reviewed By: captbaritone Differential Revision: D39891556 Pulled By: captbaritone fbshipit-source-id: c11e0914beafe80069e170b9b5800ef507356ada

captbaritone

Thanks for your PR and for your patience! Seems like a reasonable idea! I left some feedback.

compiler/crates/relay-compiler/src/artifact_content/content.rs

compiler/crates/relay-config/src/project_config.rs

compiler/crates/relay-codegen/src/build_ast.rs

captbaritone · 2023-03-22T23:03:22Z

compiler/crates/relay-codegen/src/build_ast.rs

+
+        match request_parameters.request_format {
+            Some(
+                RequestFormat::Text(Some(ref text)) | RequestFormat::Migration(_, Some(ref text)),


See above: Can RequestFormat have a method text() to get the text from the underlying variant?

captbaritone · 2023-03-22T23:04:57Z

compiler/crates/relay-codegen/src/build_ast.rs

+        params_object.push(id_prop);
+        params_object.push(metadata_prop);
+        params_object.push(name_prop);
+        params_object.push(operation_kind_prop);


Could we unconditionally initialize the params object with the ones we know we are going to include and then just have one push for the request_format case?

I've done it like this in order to maintain the existing artifact ordering. If I change this, it will invalidate all existing artifacts (even without this feature on). If that's okay to for you at Meta, I can do this - but wanted to reduce your churn

captbaritone · 2023-03-22T23:05:20Z

compiler/Cargo.lock

@@ -1580,6 +1580,7 @@ dependencies = [
 "graphql-test-helpers",
 "intern",
 "lazy_static",
+ "relay-config",


Is this intentional?

captbaritone · 2023-03-22T23:05:44Z

compiler/crates/relay-codegen/src/ast.rs

+pub enum RequestFormat {
+    ID(QueryID),
+    Text(Option<String>),
+    Migration(QueryID, Option<String>),


Can we rename this to something more concrete? TextAndID maybe?

I've simplified the structure to no longer need this additional struct

Lalitha-Iyer · 2023-05-09T02:31:12Z

@tomgasson and @captbaritone Thanks a ton for seeing this PR through. Do you know when can we expect this to be merged and available ?

facebook-github-bot · 2023-05-22T17:08:59Z

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

captbaritone

I've imported the diff and it uncovered some Flow errors. I also left some requests to avoid the tuple matches and replace with nested if/else statements

packages/relay-runtime/util/RelayConcreteNode.js

compiler/crates/relay-config/src/project_config.rs

compiler/crates/relay-compiler/src/artifact_content/content.rs

compiler/crates/relay-codegen/src/build_ast.rs

facebook-github-bot · 2023-05-30T21:44:02Z

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-06-02T17:40:40Z

@captbaritone merged this pull request in 95c54b4.

captbaritone · 2023-06-02T18:28:53Z

This has landed. Folks can try it out by using the @main NPM tag. Make sure you upgrade all of relay-runtime, relay-compiler and react-relay.

reckter · 2023-12-08T10:51:30Z

FYI, if you use local persist query you have to use the field name in snake_case, not camelCase.
it has to be include_query_text: true.

facebook-github-bot added the CLA Signed label May 19, 2022

tomgasson mentioned this pull request Jun 21, 2022

Compact GraphQL query text #3983

Closed

tomgasson force-pushed the persist-migration-helper branch 2 times, most recently from e8836f5 to 688714f Compare September 26, 2022 03:03

tomgasson force-pushed the persist-migration-helper branch 2 times, most recently from 108abba to 7b80335 Compare October 29, 2022 05:26

soneymathew mentioned this pull request Dec 6, 2022

Add configuration option to keep both ID and query text in artifacts #4129

Closed

captbaritone requested changes Mar 22, 2023

View reviewed changes

tomgasson force-pushed the persist-migration-helper branch 3 times, most recently from d6a1e23 to ad7853f Compare April 11, 2023 23:35

tomgasson requested a review from captbaritone April 11, 2023 23:35

captbaritone requested changes May 22, 2023

View reviewed changes

Allow persisting to also include the query text

9af4ff3

tomgasson force-pushed the persist-migration-helper branch from ad7853f to 4c3cf3d Compare May 24, 2023 08:06

Apply suggested changes

50bbdb3

tomgasson force-pushed the persist-migration-helper branch from 4c3cf3d to 50bbdb3 Compare May 24, 2023 08:08

facebook-github-bot closed this in 95c54b4 Jun 2, 2023

facebook-github-bot added the Merged label Jun 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow persisting to also include the query text for safe migration #3917

Allow persisting to also include the query text for safe migration #3917

tomgasson commented May 19, 2022 •

edited

Loading

maraisr commented May 19, 2022

captbaritone left a comment

captbaritone Mar 22, 2023

captbaritone Mar 22, 2023

tomgasson Apr 11, 2023

captbaritone Mar 22, 2023

captbaritone Mar 22, 2023

tomgasson Apr 11, 2023

Lalitha-Iyer commented May 9, 2023 •

edited

Loading

facebook-github-bot commented May 22, 2023

captbaritone left a comment

facebook-github-bot commented May 30, 2023

facebook-github-bot commented Jun 2, 2023

captbaritone commented Jun 2, 2023 •

edited

Loading

reckter commented Dec 8, 2023

Allow persisting to also include the query text for safe migration #3917

Allow persisting to also include the query text for safe migration #3917

Conversation

tomgasson commented May 19, 2022 • edited Loading

maraisr commented May 19, 2022

captbaritone left a comment

Choose a reason for hiding this comment

captbaritone Mar 22, 2023

Choose a reason for hiding this comment

captbaritone Mar 22, 2023

Choose a reason for hiding this comment

tomgasson Apr 11, 2023

Choose a reason for hiding this comment

captbaritone Mar 22, 2023

Choose a reason for hiding this comment

captbaritone Mar 22, 2023

Choose a reason for hiding this comment

tomgasson Apr 11, 2023

Choose a reason for hiding this comment

Lalitha-Iyer commented May 9, 2023 • edited Loading

facebook-github-bot commented May 22, 2023

captbaritone left a comment

Choose a reason for hiding this comment

facebook-github-bot commented May 30, 2023

facebook-github-bot commented Jun 2, 2023

captbaritone commented Jun 2, 2023 • edited Loading

reckter commented Dec 8, 2023

tomgasson commented May 19, 2022 •

edited

Loading

Lalitha-Iyer commented May 9, 2023 •

edited

Loading

captbaritone commented Jun 2, 2023 •

edited

Loading