leangen / graphql-spqr

Build a GraphQL service in seconds
Apache License 2.0
1.09k stars 179 forks source link

Support arguments for batched queries #438

Closed nlippke closed 1 year ago

nlippke commented 1 year ago

Extra arguments to a batched query are always empty. This PR makes them visible. Suppose to have a method like

@Batched
@GraphQLQuery
public List<Education> education(@GraphQLContext List<SimpleUser> users,
                @GraphQLRootContext AtomicBoolean flag,
                @GraphQLArgument(name = "extraArg") String extraArg) {
}

with a query like

query {
  candidates { education(extraArg:"someValue") {startYear} }
}

Without this PR, education() was called, but extraArg was always null. Now it's filled.

kaqqao commented 1 year ago

Hi! And thanks for the interest in contributing. But this isn't semantically valid. Each operation in the batch has its own arguments, that are available from their own DataFetchingEnvironment/ResolutionEnvironment. There isn't a singular global argument value applicable to the whole batch. The first DataFetchingEnvironment is in no way special or applicable to all the resolvers.

nlippke commented 1 year ago

Hmm, but if I define a (static) argument to my graphQL query then wouldn't I expect it to same for the whole key set? What could be a counter example? What would be then the correct way to implement it?

nlippke commented 1 year ago

@kaqqao: I've removed the use of single DataFetchingEnvironment for the whole batch. Instead I'm now grouping the requests by arguments to ensure that the correct resolver is chosen. Afterwards I rejoin the results. I now correctly processes queries like

query {
  candidates { 
      q1:education(extraArg:"someValue") {startYear} 
      q2:education(extraArg:"someOtherValue") {startYear}
  }
}

What do you think?

kaqqao commented 1 year ago

I think this is the right approach! Thanks for looking into it.

I made some changes to your branch (to accommodate the changes in the underlying code base) and squashed and merged into master. So I can close this PR. Thanks for the contribution!

Btw, I also added a custom CacheKeyFunction so that parameterized batch loaders work with caching enabled (in your branch, they'd only work with caching off).