Skip to content

Best practices for handling optional arguments? #388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alexgenco opened this issue Apr 9, 2020 · 9 comments · Fixed by #404
Closed

Best practices for handling optional arguments? #388

alexgenco opened this issue Apr 9, 2020 · 9 comments · Fixed by #404

Comments

@alexgenco
Copy link

We just updated graphql-java-tools from 5.4.1 and the biggest change is around nullable input fields (I think caused by #315).

Previously, we were defining our resolver methods with Optional<> arguments for those that are nullable in the schema, e.g. first in

customers(input: CustomerSearchInput!, first: Int): [Customer!]

... mapped to the following resolver method:

public List<Customer> getCustomers(CustomerSearchInput input, Optional<Integer> first) { ... }

Previously, the first argument was Optional.empty() regardless of whether the client passed an explicit first: null or didn't pass the argument at all. However, now we are getting null when the client doesn't pass the argument, which requires us to handle both null and Optional.empty in all resolver methods with optional arguments.

Is there a way to opt-out of this new behavior? Or if not, a recommended way to unify handling of null and absence? e.g. in the object mapper configurer or something?

@vojtapol
Copy link
Member

vojtapol commented Apr 9, 2020

This was done so that it could be detected whether the client passed in a parameter or not. If you are not interested in the distinction, can you just use Integer first and do a simple null check on it?

@alexgenco
Copy link
Author

@vojtapol this is what we've started doing (updating the arguments to the unwrapped type, and doing an Optional.ofNullable on it inside the method body), but it will be duplicated in a lot of places, almost all of which the distinction between null and absent isn't meaningful.

@alexgenco
Copy link
Author

alexgenco commented Apr 9, 2020

Another question... we already have a custom class that we use to differentiate null vs. absent called Omissible, which is basically a wrapper around Optional<Optional<T>>, so Omissible.absent == Optional.empty, Omissible.null == Optional.of(Optional.empty), and Omissible.of(value) == Optional.of(Optional.of(value)). Is there a way to hook up the schema object mapper to deserialize nullable arguments into Omissible?

Edit: also want to mention that we have been using a custom deserializer for our omissible class on the schema object mapper for a while now, and it works for deserializing input object fields into omissibles, but we're seeing now that it doesn't work for top level field arguments. It seems like it should work the same for both?

@vojtapol
Copy link
Member

vojtapol commented Apr 12, 2020

@vojtapol this is what we've started doing (updating the arguments to the unwrapped type, and doing an Optional.ofNullable on it inside the method body), but it will be duplicated in a lot of places, almost all of which the distinction between null and absent isn't meaningful.

I think the right way to solve this would be to add a configuration option that changes how Optional behaves. The current behavior would be the default but you could switch it to the old behavior.

also want to mention that we have been using a custom deserializer for our omissible class on the schema object mapper for a while now, and it works for deserializing input object fields into omissibles, but we're seeing now that it doesn't work for top level field arguments. It seems like it should work the same for both?

Can you show me how are you passing the custom deserializer? I don't need to see its implementation, just how it's provided to GraphQL Java Tools.

@alexgenco
Copy link
Author

I think the right way to solve this would be to add a configuration option that changes how Optional behaves.

👍 to this, we definitely don't want to have to null check top level arguments, when input sub-fields just work.

Can you show me how are you passing the custom deserializer?

GraphQLSchema graphQLSchema = SchemaParser.newParser()
  ...
  .options(SchemaParserOptions.newOptions()
    .objectMapperConfigurer((mapper, context) -> {
      mapper.registerModule(new OmissibleModule());
    })
    .build())
.build()
.makeExecutableSchema();

OmissibleModule is basically

public class OmissibleModule extends SimpleModule {
  public OmissibleModule() {
    addDeserializer(Omissible.class, new OmissibleDeserializer());
  }
}

@alexgenco
Copy link
Author

Any thoughts on this? This is basically blocking us from upgrading to the latest version

@vojtapol
Copy link
Member

vojtapol commented May 21, 2020

#404 addresses the first half of this issue. Let me know if it would fix your issue.

Another question... we already have a custom class that we use to differentiate null vs. absent called Omissible, which is basically a wrapper around Optional<Optional>, so Omissible.absent == Optional.empty, Omissible.null == Optional.of(Optional.empty), and Omissible.of(value) == Optional.of(Optional.of(value)). Is there a way to hook up the schema object mapper to deserialize nullable arguments into Omissible?

Edit: also want to mention that we have been using a custom deserializer for our omissible class on the schema object mapper for a while now, and it works for deserializing input object fields into omissibles, but we're seeing now that it doesn't work for top level field arguments. It seems like it should work the same for both?

This doesn't work because types that are a GraphQL scalars bypass the object mapper. We might want to look into changing that.

@alexgenco
Copy link
Author

@vojtapol any chance for a patch release in the near future?

@vojtapol
Copy link
Member

Yeah. There is a chance for that. We are planning to make the input argument mapping a lot more flexible for users that need more customization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants