router

router copied to clipboard

Describe the bug Our customers define variables: "{key1: value1}" using an object inside a string but the router doesn't parse it. We have to use an object directly like this variables: {key1: value1}, and this creates an issue for us. We are not sure why the router doesn't parse objects from a string, and it will also be really difficult to ask the customers to change their behaviour.

Expected behavior router should be able to parse the object from variables: "{key1: value1}".

why are the variables defined that way, while the expected behaviour is to have them passed as an object? (cf https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md )

I'd be wary of changing the router's default behaviour for this, but that can probably be adapted with a plugin

Looked at it a bit more, and it would not be doable with a plugin, because the request object is parsed before it gets to plugins. It could be solved by having a proxy in front of the router normalizing queries, that should help with other format issues you would get from customers, is it a solution you looked at?

@xsoufiane:

1. Are these GET requests or POST requests?
2. What is the content-type of these HTTP requests?
3. Can you share an example of a GraphQL server which does support what you're suggesting?
4. Why did your customers choose to use that approach in the first place? (Your examples within the strings aren't even valid, parsable JSON. Is there a different encoding being used?)

Thank you for feedback @Geal and @abernix

What is the content-type of these HTTP requests?

I did some reading of https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md, and I think we have this because we are also supporting text content-type not only json (as raised in https://github.com/apollographql/router/issues/1298).

a proxy in front of the router normalizing queries

Adding another component before the router would add overhead, which we would like to avoid. right? @yanns

Are these GET requests or POST requests?

The ones that I investigated are POST.

Why did your customers choose to use that approach in the first place? (Your examples within the strings aren't even valid, parsable JSON. Is there a different encoding being used?)

I think this is because we were allowing a different content-type, and it may be also that customers are using tech that doesn't allow to send json objects (https://stackoverflow.com/questions/36861125/pass-json-object-vs-json-string-in-http-post).

Can you share an example of a GraphQL server which does support what you're suggesting?

I don't have knowledge about other servers, but we are using sangria library https://sangria-graphql.github.io/learn/#result-marshalling-and-input-unmarshalling as you can read it deserialization is open and it depends on the chosen unmarshaller. In our server, we are using https://github.com/json4s/json4s, this is why we are able to parse json objects from strings.

If I wasn't clear about something, I should look deeper into a subject, have any other questions or remarks please feel free to tell me

Also I might be wrong but I believe if the router doesn't allow to deserialise any content-type to json, it goes against the spec because it enforces application/json content-type implicitly.

Also I might be wrong but I believe if the router doesn't allow to deserialise any content-type to json, it goes against the spec because it enforces application/json content-type implicitly.

More precisely, the server MUST support requests with the application/json and application/graphql+json content type header, but MAY accept other types, like text/plain. So the router is following the spec here, but as I said in #1298, we'll look into it, because accepting text/plain is not too complex and the CSRF plugin already handles that case.

In any case, if we accept text/plain as content type, we would still expect valid JSON in the body, we don't support other serialization formats. And we have to follow the expected schema for the request object, which uses an object for the variables field.

I am not sure there would be a lot of overhead in having a proxy normalize data before sending it to the router, that should be measured. The router spends its time serializing and deserializing JSON and it amounts to a few microseconds per query

isn't there a way to abstract deserialisation, so that we can customise it when there is a need to? like the approach followed in sangria 🤔 because adding a proxy for us will mean an additional component, ownership, potential network hop, overhead, and complexity, which we would really like to avoid.

In a similar spirit to my comment on https://github.com/apollographql/router/issues/1298, I'm going to close this as "working as intended" (and unlikely to be supported natively in the Router, since that is not a standardised way for serializing objects in the way that JSON is) but reference this in the issue that aims to abstract away transport-related details like this to something that can be configured in user-land plugins.

I hope that issue ends up being a suitable answer to your desire to have an abstraction that allows changes to the serialization format that might be more suitable for your own cases!

Separate from the above, personally, I think it would do good service to the GraphQL ecosystem at large if you help sort out how your customers ended up sending data in that format and help them understand how invalid it is. I cannot imagine any format in which the format that you're suggesting should be supported should actually be supported or how it could be desired. I would encourage doing anything possible to make that go away to ensure compatibility with GraphQL tooling across the stack. The GraphQL HTTP transport specification is relatively well-adopted by popular frameworks today, but both this issue and your #1298 would be doing quite opposite of what that specification suggests and the specification working group seems to have every intention of becoming more opinionated about what it does specify over time.

We agree with you. Our issue is mainly a misunderstanding of the specification. The new customers are not allowed to use this special format anymore. And we are in communication with the existing ones to help them doing the move. https://docs.commercetools.com/api/releases/2022-10-04-announced-removal-of-non-standard-graphql-variables-format