Emit raw query text in artifact for @relay_test_operation operations

[jakobkhansen]

Hello!

In Microsoft we have a use case where we would like to parse Relay operations being executed with graphql-js. In our case this is specifically for testing with Storybook, where we want to use Graphitation payload generator to generate mock data, which takes graphql-js operations as input.

Today, parsing operations is somewhat limited because the text field in Relay operation artifacts is heavily transformed and for example does not contain client extensions. We would like to expose the raw, untransformed query text in operation artifacts, hidden behind the @relay_test_operation directive. We believe this would be useful in other testing scenarios as well, because it enables Relay operations to be easily converted between formats and used with other GraphQL tooling.

This PR implements that by adding a rawText field in OperationDefinition and a new transform, the RawTextTransform. The transform populates this field, only on operations with @relay_test_operation directive. The transform is specifically added as the first transform to be executed, since it relies on generating the original, "raw" query text, before any other transforms are applied.

Here's an example on how rawText would be exposed in an artifact:

where you can see that viewData, which is a client-extension, is preserved.

I've added this field also to Flow types, and if you think it makes sense to mention this in docs, I can add that as well.

Would love to hear if you have any thoughts on adding this feature :)

[facebook-github-bot]

Hi @jakobkhansen!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[jakobkhansen]

TODO: Write a test where rawText is not None if possible.

[tyao1]

I'm curious how is the text used for test operations?

text field in Relay operation artifacts is heavily transformed and for example does not contain client extensions. We would like to expose the raw, untransformed query text in operation artifacts, hidden behind the @relay_test_operation directive

Apart from client extensions, is there anything else that is missing in text and requires raw_text? If the goal is to include client extensions in the query text, I'm wondering if we can add a flag to not exclude client extensions in the query text output.

Since the raw text is intended for @relay_test_operation only, the recommended approach is to create a directive to store that information, and extract it in build_ast. So it doesn't change existing data structures

[jakobkhansen]

@tyao1 our main use case right now is to include the client extensions, yes. What type of flag were you thinking here? Since this would need to be on a per operation basis, would it be a directive similar to @relay_test_operation which would simply include client extensions in the text field? I think that would work in our testing scenario, but it would mean that the operation could no longer be used as a real server query since AFAIK the text field is what is sent to the server, right? So it shouldn't contain the client extensions. That is not problematic in our case though since an operation would be for testing only if it has the directive.

To answer the question on how text is used when we test, since Graphitation payload generator uses graphql-js operations as input, we parse the text field to get the correct type format , but that does not contain client extensions, so we are losing that information there.

Good tip on the directive to store the data, if we don't go the flag route I will look into that change.

[tyao1]

I think that would work in our testing scenario, but it would mean that the operation could no longer be used as a real server query since AFAIK the text field is what is sent to the server, right?

That's correct.

skip_client_extensions is the transform that removes client extensions from the query text. I think it can read a new argument on @relay_test_operation or a new directive to skip the transform.

[jakobkhansen]

I've been trying to do the flag route and skip the client extensions transform, and I have some questions.

We cannot skip the transform entirely, since of course we have multiple operations in a program and some may not have @relay_test_operation, but we can just Transformed::Keep in transform_operation based on the directive flag.

But then comes the issue that the transform also deletes fragments that are on ClientType. We need to keep those around since the operations can spread those now that we're not removing the client extensions there. I see the options here of either having some mapping of which fragments all the @relay_test_operation operations refer to and keeping only those, or just keeping all of them and removing the transform_fragment function in skip_client_extensions entirely. This means that we are keeping some client fragments around that are perhaps never used in test operations, but personally I think that is fine? It seems to me that at some later step the text field has filtered out the fragment definitions that are not used by the operation anyways. Would there be a performance concern with keeping these fragments around?

What do you think @tyao1?

[jakobkhansen]

Ok, so I attempted the flag to not do the skip_client_extensions transform and I don't think it's a viable path. Mainly since multiple operations can share the same fragment, and one operation may want to remove the client extensions while another wants to keep them (in the case of test operation with flag set). For example this:

query TestOperation @relay_test_operation(includeClientExtensions: true) {
  ...ServerType
}

query ServerOperation {
  ...ServerType
}

fragment ServerType {
  serverField
  clientExtension
}

Since the test operation uses the ServerType fragment, we can't remove the clientExtension field there, but if we don't then the ServerOperation would end up emitting clientExtension in the text field, which we obviously can't.

The only thing we could do would be to duplicate the fragment somehow and have a separate version which keeps the client extensions, but I think this would be a bit complicated and probably we don't want to do that.

I think the best option is to as you suggested, have an argument on @relay_test_operation which stores the raw_text in a metadata directive, and then we set text field to the raw_text metadata if it's populated. This is a way smaller change now and has no interface changes in artifact, which I think is best.

I pushed this change now, which adds the emitRawText: Boolean argument to @relay_test_operation.

@tyao1 , please have a look and see if this approach is viable :)

[tyao1]

Since the test operation uses the ServerType fragment, we can't remove the clientExtension field there, but if we don't then the ServerOperation would end up emitting clientExtension in the text field, which we obviously can't.

Makes sense. Thanks for trying out this approach

[tyao1]

Looks very clean! Requesting changes to check EMIT_RAW_TEXT_ARG before attaching the raw text

[jakobkhansen]

Thanks for the review @tyao1, let me know if there is anything else to fix now :)

[tyao1]

Looks good to me! It would be nice to have a snapshot test for the raw text generation under relay-compiler/tests/compile_relay_artifacts/fixtures, but it is up to you.

[facebook-github-bot]

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

[jakobkhansen]

@tyao1 I added a test and hopefully fixed the lint issue that was failing on CI.

[facebook-github-bot]

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

[jakobkhansen]

@tyao1 sorry for the back and forth, I had manually updated the fixture tests before as I was unaware of ./scripts/update_fixtures.sh. I've ran and committed that now. Hopefully that was the last thing failing internally for you.

[jakobkhansen]

@tyao1 ping on this one :)

[facebook-github-bot]

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

[tyao1]

@jakobkhansen I brought the PR up to the team, and we have more questions. Using raw query text has the downside that some client only directives like @required will no longer work. Could you provide more detail on how the generated payload is consumed?

[jakobkhansen]

Sure, we are planning on using it exactly here. The idea is that we have a Storybook decorator which should work to generate mock-data for both Apollo and Relay based applications, and uses the Graphitation payload-generator. What we do for Relay operations is take the text field and parse that with graphql-js in order to input it into our payload generator. At that point the problem with missing client extensions appears, because we lose those in the translation, since the text field does not contain it.

If there are concerns with this breaking the text field, my suggestion is that we could go back to the previous approach of supplying rawText field in the artifact in addition to text when @relay_test_operation is present. That way we can avoid adding the new directive, and the original text field will be left unchanged. I think we would actually prefer this, to avoid adding the emitRawText directive to every test operation. How do you feel about that @tyao1 ?

[tomgasson]

Chiming in to support the request for features like this.

At Atlassian, we also had to tweak the compiler to output the query text (a bit of a hack - into the extra artifacts dir) so that our tests can look that up and mock it. We've been carrying that patch across upstream updates for a while now.
For our tests / storybook setup, we replace the network layer, find the queries' text for the operation's persisted id and then run that query (with graphqljs in the browser) and have those resolvers read from mock data.

We'd additionally like to shift from this setup to instead actually hit a local server. Sending 1k+ queries over the network and the resolvers for storybook takes a while to bundle since GraphQL is a natural "barrelling" point. But even that would then have the same trouble: the query text for these operations doesn't even exist in our repo (they're persisted to a service), so any mechanism to get the query text out is valuable: Even something as simple as allowing the persist: {...} to do both the local and remote based persisting at the same time (with the ID coming from the remote) would solve our problem.
Because testing is local, we actually worked out we don't even need to do this by ID. The operationName's are unique, there's no versioning issues to contend with for testing, so being able to lookup Record<OperationName, QueryText> would be a nicer and cleaner way to consume this (the requestParams has the name)

[Markionium]

@tyao1 I'm not sure I understand why @required would no longer work?

Am I misunderstanding that this would only change the query string that is in the params.text field of the document but not the actual runtime artifact?

Perhaps because these fields would now returned in the payload from the mock server even though they're actually client extensions? But I think @required works for both?

[facebook-github-bot]

@tyao1 merged this pull request in 3320532.