fix: don't treat empty operation name as special #4211
Conversation
previously, if an empty operation name was given and multiple operations were provided in the query, then the first operation would be returned. not entirely sure why this would be the desired behaviour, when an exception would be thrown for any other value that doesn't match an existing operation in the query.
|
@RafeArnold can you please give more context? What do you mean with previously? What is exactly the problem or error you are experiencing? I believe this code has like this forever and we need to have a good reason to change it. |
|
when executing a graphql query that contains multiple operations, the operation to execute must be specified via the operationName parameter. currently, if the provided operation name does not match any of the operations in the query, an error is thrown. however there's an exception to this behaviour when the provided operation name is empty. in that case, the first found operation is returned instead. i dont understand what the motivation for this exception to the rule is. why would an empty operation name imply "just give me the first operation you find in the query"? id expect it to still throw an error explaining that there are no operations with an empty name. |
We kinda agree with this in general, but we are wondering what is your use case? Can you provide more context? This code has been like this for 8+ years. |
|
I kinda agree that its weird that in the case of Its trying to handle this case BUT its also a low level utitlity class. I would love to see examples of how this gets invoked with We could fix it but I want to see the knock on effects. What parts of other higher level code are calling this and can we get tests for that |
|
maybe i was too hasty with my solution here (teaches me not to submit prs as im rushing to leave the office). an alternative solution to an empty operation name retrieving the first operation arbitrarily when multiple operations are supplied is to treat the empty operation name in the exact same way as the null operation name. i.e. throw an exception explaining that an operation name must be supplied if multiple operations are present in the query. that way, the existing functionality remains that allows the library users to supply an empty operation name when there is only a single (named or unnamed) operation in the query, but the unfortunate side effect of the implementation that allows an empty operation name when there are multiple operations is removed. it seems very unlikely that any library users would be relying on this side effect, as it seems more likely to be a bug than a design decision. the fix for this would be to simply add the same would this proposed change be more acceptable? |
if an empty operation name is given and multiple
operations are provided in the query, then the first operation is
returned. not entirely sure why this would be the desired behaviour,
when an exception is thrown for any other value that doesn't
match an existing operation in the query.