Add a JSON file query persister strategy to relay-compiler

[vincentriemer]

NOTE: This is essentially the first time I've developed in Rust so feel free to eviscerate it because...

Summary

Previously in the new rust relay-compiler your only option for persisting queries is to use the remote persister which makes POST requests to a given endpoint with the operation text to be persisted. This PR adds the option to instead persist your queries to a local JSON file in a very similar way to how the JS compiler originally did it.

If you had a basic remote persister config like this:

persistConfig: {
  url: "http://localhost:3000",
  params: {}
}

changing it to a local JSON file persister means updating the config to:

persistConfig: {
  filePath: "./queries.json"
}

When this is enabled and a new query is persisted the compiler will first attempt to read the local file if it exists as a hash map (creating a new one if there is no existing data), hashes the operation text, adds the operation text to the hashmap keyed by the hashed version of the operation, and then serializes & writes the resulting map to the configured path.

Test Plan

Leveraging my own simple repo I've been using for reproducing issues with the new relay compiler, installed the relay-compiler to a version including this PR's changes, and updated the relay.config.js file as such:

module.exports = {
  "src": "./src",
  "schema": "./schema.graphql",
  "artifactDirectory": "./src/__generated__",
  "language": "typescript",
  "persistConfig": {
-    "url": "http://localhost:3000",
-    "params": {}
+    filePath: "./queries.json"
  }
};

Then after running the relay compiler I verified that it created a queries.json file and it contained the expected persisted query json output:

{
  "00dc69b2cfc4c1da3528e9aaf65a05091740fc71": "query AppQuery {\n  viewer {\n    submission(name: \"t3_qvyl2j\") {\n      ...Post_submission\n      id\n    }\n  }\n}\n\nfragment Post_submission on Submission {\n  title\n  author\n  selftext {\n    html\n  }\n}\n"
}

[alunyov]

Wow! This is great @vincentriemer! As for the first time rust contribution you very much know what you're doing!

I think we also need to test the case with --watch mode. I think we may be adding more queries to this file, essentially for every intermediate, but valid change in the watch mode these queries would get persisted to this file.

Maybe we also need to pass CLI flags to FilePersister - and error if --watch flag is provided (as a simple workaround)?

[alunyov]

Fill free to import this to fbsource, I think there also maybe some internal formatting issues that needs to be fixed.

But this looks great!

[alunyov]

I like this serde feature, the only problem with it: obscure error messages if both/mixed values are provided.

[alunyov]

Nice!

[josephsavona]

I just quickly skimmed the code but what is the strategy for handling the fact that queries are persisted concurrently?

[alunyov]

Also, alternative approach is to implement this persisting the way you did here: https://github.com/vincentriemer/new-relay-compiler-issues-repro/blob/main/mockQueryPersistServer.js

But just make this mockQueryPersistServer store the map in memory, and flush it to the file system (periodically, maybe)?

[vincentriemer]

I just quickly skimmed the code but what is the strategy for handling the fact that queries are persisted concurrently?

I'm sorry I'm a JS dev what does "concurrently" mean \s

But just make this mockQueryPersistServer store the map in memory, and flush it to the file system (periodically, maybe)?

But more seriously: I think this is the right strategy and I'll update the PR to do this — make it so that it reads the existing file once at initialization time and keeps the persisted query map in memory, flushing back to the file only when a new query is added.

[facebook-github-bot]

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

[wyattjoh]

It seems as though the hashing mechanism here is locked to SHA1:

relay/compiler/crates/relay-compiler/src/file_persister.rs

Lines 35 to 39 in c955577

	fn hash_operation(&self, operation_text: String) -> String {
	let mut hash = Sha1::new();
	hash.input(&operation_text);
	hex::encode(hash.result())
	}

Our team has been looking to utilize the generated ID's to use with Apollo Server's Automatic Persisted Queries (or APQ), and they stipulate a required SHA256 hash.

As it seems y'all are already changing this from MD5 to SHA1, is it possible to either:

1. Change this to SHA256 to support APQ directly
2. Support changing the hashing algorithm directly using this new FilePersister config

[janicduplessis]

Tested this and works well! Needed to make some changes to fix conflicts (janicduplessis@ae1f783).

This would help a lot migrating from the js compiler. There is actually a lot of complexity that goes into running an http server just to generate a queries json file, especially if you want to abstract away starting the server in background (start server in bg, wait for it to start before running compiler, close server once compiler done). Also making the server somewhat performant by batching writes.

[tbezman]

@vincentriemer My team could definitely benefit from this. Are you going to continue work or should I drive this to completion?

[alunyov]

@tbezman @vincentriemer yes, we will include these changes to one of the upcoming releases