feat: extract document building from OperationBuilder

[Bobrokus]

Why are we making this change?

Related to: #1000

Before this change, the only way (from my research) to serialize queries into their original GraphQL source was to build an Operation and retrieve the query field. The problem is that some queries require variables, which are unnecessary for serialization, and an overhead caused by the unwanted Operation struct.

The serialization process was in place the whole time, it was just inaccessible on its own, covered by the build method.

What effects does this change have?

Moves document building out into a standalone build_executable_document function which can be called to construct a document string without access to a live variables struct

[netlify]

✅ Deploy Preview for cynic-querygen-web canceled.

Name	Link
🔨 Latest commit	304b093
🔍 Latest deploy log	https://app.netlify.com/sites/cynic-querygen-web/deploys/66c64bb5e92f380008648bef

[obmarg]

Hi @Bobrokus. Thanks for the PR, and sorry for the delay getting back to you - I've been on holiday the past couple of weeks.

I'm definitely happy to support this use case, I don't think you're the first person that has wanted it. However, I'm not sure if OperationBuilder is the right place for this to live.

Let me have a think and get back to you in the next day or two.

[Bobrokus]

Hi @Bobrokus. Thanks for the PR, and sorry for the delay getting back to you - I've been on holiday the past couple of weeks.

I'm definitely happy to support this use case, I don't think you're the first person that has wanted it. However, I'm not sure if OperationBuilder is the right place for this to live.

Let me have a think and get back to you in the next day or two.

Hey! I'm thrilled to hear this! I also don't think OperationBuilder is the right place but I was in a hurry and OperationBuilder was good enough for my purposes without having to mess with macros or too low-level stuff.

It's more of a proof of concept. It would be fantastic to see a more mature implementation, but I believe that would require knowledge of macros, which I currently lack.

NOTE: I'm also thinking about splitting the serialization into 2 parts - variables & selection set - for more refined control.

[obmarg]

It would be fantastic to see a more mature implementation, but I believe that would require knowledge of macros, which I currently lack.

I don't think this requires any macro changes tbh. I think adding a free standing function in the queries module would probably do the job. Something like:

fn build_document<Q: QueryFragment>(kind: OperationKind, name: Option<String>, features_enabled: HashSet<String>) -> String

Where the implementation is very similar to what you have in serialize in this PR.

OperationBuilder::build doesn't even need to use this function, since it has slightly different requirements (specifically it needs the list of used variables, and I'm not sure that's something that is worth exposing in this "please give me the string" functionality.

NOTE: I'm also thinking about splitting the serialization into 2 parts - variables & selection set - for more refined control.

I'm not sure exactly what you mean by this - can you elaborate?

[Bobrokus]

I don't think this requires any macro changes tbh. I think adding a free standing function in the queries module would probably do the job. Something like:

fn build_document<Q: QueryFragment>(kind: OperationKind, name: Option<String>, features_enabled: HashSet<String>) -> String

Where the implementation is very similar to what you have in serialize in this PR.

Yea agree that seems reasonable.

I'm not sure exactly what you mean by this - can you elaborate?

I need to serialize the query with variables inline.

Serialized string is made of {vars}{selection_set}. I'm thinking of creating a seperate serialization function for each one so you can serialize them seperately. I haven't checked the source code yet so i might just figure both are already in place.

[obmarg]

I need to serialize the query with variables inline.

Serialized string is made of {vars}{selection_set}. I'm thinking of creating a seperate serialization function for each one so you can serialize them seperately. I haven't checked the source code yet so i might just figure both are already in place.

I am struggling to see a use case for having the selection set separate from the variable definitions - have you got one in mind?

[Bobrokus]

I am struggling to see a use case for having the selection set separate from the variable definitions - have you got one in mind?

Yes. Like I said - inline variables. I need to replace {vars} by a custom string. There might already be a way to do it but I haven't figure it out yet.

[obmarg]

Yes, I've said it. Inline variables. I need to replace {vars} by a custom string. There might already be a way to do it but I haven't figure it out yet.

Why do you want to do that though? Can you give me an example? The variables defined in {vars} are intrinsically tied to the variables used in {selection_set}, so I'm struggling to see why you'd want to change one and not the other...

[obmarg]

Unless of course by inlining you mean taking:

query X(a: Int!, b: Int!) { foo(a: $a, b: $b) }

and converting it to

query X { foo(a: 1, b: 2) }

Is that what you're trying to do? If so, I'd still be curious as to why? I'm not sure that getting access to the raw variable string would be the right way to go about doing that, so any more info on your use case would be great...

[Bobrokus]

Unless of course by inlining you mean taking:

query X(a: Int!, b: Int!) { foo(a: $a, b: b) }

and converting it to

query X { foo(a: 1, b: 2) }

Yep, that's exactly what I meant. Sorry, I thought you could do 'inline variables'

query X(a: Int! = 1, b: Int! =2)

but I realized those are just default values. Your way is much better and compact.

[Bobrokus]

Is there a way to convert this thread into an issue or a discussion?

[obmarg]

Unless of course by inlining you mean taking:

query X(a: Int!, b: Int!) { foo(a: $a, b: b) }

and converting it to

query X { foo(a: 1, b: 2) }

Yep, that's exactly what I meant.

Ok, and you are proposing splitting up the generation of {args} & {selection_set} so that you can achieve this via string replacement...? If so: that seems like quite a poor way to inline variables into arguments, and I'm not sure I'd be happy exposing it as an API...

There might be a better way to get cynic to inline variables but it'd require a bit of API rework to achieve.

Why is it that you can't just provide the variables in the request body? I understand why you might want to serialise a query string without having to provide variables but not why you'd want to avoid variables altogether

[obmarg]

Is there a way to convert this thread into an issue or a discussion?

I don't believe there's a way to convert it. I am fine continuing here, but if you want to create an issue and/or discussion for this new request feel free.

[Bobrokus]

I don't believe there's a way to convert it. I am fine continuing here, but if you want to create an issue and/or discussion for this new request feel free.

Nope, I don't mind if you don't. Just want to point out #1000

[Bobrokus]

@obmarg so what do you think about the inline arguments?

[obmarg]

@obmarg so what do you think about the inline arguments?

I'm still wondering why is it that you can't just provide the variables in the HTTP request body? In #1000 you say:

This query accepts mutation: String, the actual mutation that will be run, and a file with variables that will be passed to each instance of the mutation in Jsonl format.

If there's a file you can pass in (which seems strange but ok) with the variables why don't you do that?

Inlining variables into the query document seems like such a niche use case, given that variables are core to GraphQL and every single server & client supports them. I'd need a very strong motivating use case to consider adding support for it. I've been asking why a lot in order to try and get at that use case, but I don't feel like I've got an answer yet.

[Bobrokus]

Oh! I forgot to mention. In the Shopify GraphQL API, there's also bulkOperationRunQuery which runs a big query e.g. querying all products in the store. It accepts a query: String! argument but not variables so I have to put the variables inside the query itself.

I might be able to pull it off myself by replacing all occurrences of $<variable> by the actual value and removing variables in the operation declaration.

The reason bulkOperationRunMutation accepts variables as a file is that it runs the mutation async for each entry in the Jsonl document. This is great for something like creating products in bulk where there can be over 4000 products.

[obmarg]

Ok, I see - that is... quite an annoying design from shopify. Let me have a quick think about how best to solve this - I have some ideas but I need to mull them over a bit.

[obmarg]

@Bobrokus I've cleaned up this PR into a state where I think it's mergable, and have some ideas for how to tackle the inlining you are after - however it's too late for me to write them out for you now. I'll get back to you about that bit tomorrow.

[Bobrokus]

@Bobrokus I've cleaned up this PR into a state where I think it's mergable, and have some ideas for how to tackle the inlining you are after - however it's too late for me to write them out for you now. I'll get back to you about that bit tomorrow.

Thanks so much! I truly appreciate it. See you tommorow. Btw, why not implement fmt for OperationType instead of as_str?

[obmarg]

Btw, why not implement fmt for OperationType instead of as_str?

as_str seemed more flexible - in that you might sometimes want a &str without the allocations that fmt usually makes. I could have written a fmt impl on top of as_str, but I couldn't be bothered.