Optional Input fields detection

[ehardy]

Hello, I would like to get feedback on the following idea and proposal. There is no out of the box way to easily determine if an optional field of an Input object has been explictly set to null or if it was simply not specified in the mutation request. We have been toying with an idea to provide an easy to use mechanism to address this situation, without forcing service developers to have to handle it themselves. The idea we've had was to create a special type called OptionalField<T>, similar to java.util.Optional<T>, but that could hold a third state, missing. Here's an example of how it could be used:

public class UpdateUserInput {
    private String firstName;
    private String lastName;
    private String email;
    private OptionalField<String> nickName = OptionalField.missing();

    public String getNickName() {
        if (nickName.isMissing()) {
            throw new NoSuchElementException("nickName field not provided");
        }

        return nickName.isPresent() ? nickName.get() : null;
    }

    public void setNickName(String nickName) {
        this.nickName = OptionalField.ofNullable(nickName);
    }

    public boolean isNickNameMissing() {
        return nickName.isMissing();
    }

    //other getters/setters ommitted
}

In the above example, the first three fields are mandatory, and the nickName field is optional. Optional input fields would be set to OptionalField.missing() on instance creation, and only if setters are called during input object mapping, would optional fields be overridden with an explicit null or actual value. OptionalField<T> would provide an elegant mechanism in mutation handling code to determine if a field needs to be set to null or other value, or if the current value should be preserved:

@DgsMutation(field = "updateUser")
public UpdateUserPayload updateUser(@InputArgument("input") UpdateUserInput input) {
    User user = ...//somehow load the user

    if (!input.isNickNameMissing()) {
        user.setNickName(input.getNickName());
    }

    //rest of mutation logic...
}

So all of this works great on paper! But DGS, in the DefaultInputObjectMapper class, only supports setting input object fields directly, and does not call setter methods. I quickly hacked DefaultInputObjectMapper to support calling setter methods to validate if the above idea would work, and it does.

So would it make sense for DefaultInputObjectMapper to support calling setter methods if some exists? I'm thinking there could be different strategies for setting input object fields, the default being the current direct field access. But it would be useful for the above use case if there was a strategy, enabled via some kind of configuration, for calling setters first if they exists, and falling back to direct field access, for example.

If the above makes sense, would be happy to contribute with a PR. Let me know.

[ehardy]

To clarify, the problem with setting the optional field directly in the example above as DGS currently does is that an exception gets thrown because of a type mismatch.

[berngp]

Hi @ehardy, thank you for posting the proposal. I think we have two topics.

1. Support a type that represents an optional field that was explicitly not set.
2. Have the 'DefaultInputObjectMapper' use the setter first, and property access as a fallback.

I think we should implement the second point right away. As of the first point, I like the idea but will like to discuss with the team how this could be supported by codgen.

[ehardy]

Thanks for the quick reply @berngp! You're absolutely right, it is two topics. The second enables the first. Let me know if you'd like that I open a PR for supporting setters or if you'll handle it within the DGS team.

[berngp]

@ehardy please feel free to open the PR. Thanks in advance!

[ehardy]

Hello @berngp , I have created the following PR. As discussed, I have added the ability for setter methods in input objects to be called, falling back on the current behaviour of setting input fields directly. This is my first time writing Kotlin, so feel free to adjust if some things are not idiomatic Kotlin 😁

Some notes about the implementation. I have used Spring reflection capabilities in my implementation, as Spring was already used within DGS. The implementation is wrapped in the new Accessor class, and I have refactored DefaultInputObjectMapper to use this new class. If Spring was not a good choice, it should be possible to refactor Accessor without having to change DefaultInputObjectMapper. I modeled Accessor by reverse engineering the logic of DefaultInputObjectMapper.getFieldType() and created some additional unit tests. All of the existing unit tests are successfully passing.

Let me know if you have any feedback, will be happy to adjust if necessary.

[ehardy]

PR #1094 was reviewed and merged and input object setter methods are now supported. Thus closing this discussion.