Add OptionalInput type to allow explicit null value detection

[koenpunt]

In graphql there's a difference between a null passed as input, and not providing the input value at all.

Inspired by the type that graphql-kotlin provided, I've introduced an OptionalInput wrapper, which can be used to distinguish between explicit null values and absent values.

Builds upon #139

[koenpunt]

DataBinder.bind doesn't pass null values to the conversion service, so this setter can receive null.
The condition here is more like a quickfix, and I hope someone knows a better way around this.

[bclozel]

Hey @koenpunt , I'm still processing #139 with some refactoring and discussing with the broader team.

I don't understand OptionalInput and its relationship to the existing Java Optional support or the required annotation arguments already in place. This would be a quite unusual arrangement in a Spring project (we don't have similar types in other Spring web frameworks).

I'll review these changes after #139, but please note that for such improvements discussing things in an issue is a good first step if we want to avoid working on contributions that won't be integrated.

[koenpunt]

I don't understand OptionalInput and its relationship to the existing Java Optional support or the required annotation arguments already in place.

See here for details on the concept of the OptionalInput class; https://opensource.expediagroup.com/graphql-kotlin/docs/schema-generator/execution/optional-undefined-arguments/#using-optionalinput-wrapper

The Java Optional type only distinguishes between null and non-null (afaik), so there's no state where the value is not provided. And with the required annotation I assume you mean the @Argument(required = false)? That annotation can only be applied to function paramaters.

Although maybe a better, more Spring-like solution would be to introduce an additional annotation.

[rstoyanchev]

I think for an Optional-like type in Java, by now most are conditioned to expect methods like isDefined, ifDefined(Consumer<T>) and similar vs relying on instanceof checks.

That said, like Brian, my concern is that such a type feels more broadly applicable. In a web application you can have a query parameter present with a value, present without a value, or not present at all.

What did you have in mind with the additional annotation? Also I'm wondering is it really necessary to differentiate vs expecting a more explicit modeling of "null" input?

[koenpunt]

I think for an Optional-like type in Java, by now most are conditioned to expect methods like isDefined, ifDefined(Consumer<T>) and similar vs relying on instanceof checks.

I'm not sure I understand what you mean by this.

What did you have in mind with the additional annotation?

Didn't have anything in mind yet, but since a lot of things in Spring are handled with annotations it might be more fitting.

Also I'm wondering is it really necessary to differentiate vs expecting a more explicit modeling of "null" input?

What do you mean by this?

[koenpunt]

I think for an Optional-like type in Java, by now most are conditioned to expect methods like isDefined, ifDefined(Consumer<T>) and similar vs relying on instanceof checks.

Oh I think you refer to the different nested classes for Undefined/Defined; that's mainly because that how this was implemented in graphql-kotlin, and Kotlin supports them in switch statements with pattern matching, allowing to use it like;

val value = when (myParameter) {
  is Defined -> myParameter.value
  else -> null
}

[koenpunt]

Don't know if this would work or This works but not sure if it makes sense, but when the property is null, it's undefined, when it's an Optional, it's defined, but can still be null.

static class BookInput {
    @Nullable
    Optional<String> notes = null;
    
    @Nullable
    public Optional<String> getNotes() {
	    return this.notes;
    }
    
    public void setNotes(@Nullable Optional<String> notes) {
	    this.notes = notes;
    }
}

[rstoyanchev]

Oh I think you refer to the different nested classes for Undefined/Defined

Yes.

This works but not sure if it makes sense, but when the property is null, it's undefined, when it's an Optional, it's defined, but can still be null.

I did not mean to suggest using java.util.Optional. I was merely observing that OptionalInput is related to java.util.Optional, as a concept and through its name, but the API and usage styles are so different. Maybe in Kotlin, as you said, it is idiomatic but in Java, I was surprised to see no instance methods and only static factory methods with nested classes, which implies if-else with instanceof checks.

This is just an aside but it does relate to a broader question. I'm hesitant to introduce a type that is positioned so closely as an alternative to java.util.Optional. Judging by our experience with java.util.Optional, it can have far-reaching impact over time to be supported as input or output in framework APIs and wherever the framework invokes application classes, along with specific support in areas such as type conversion, encoding/decoding, and last but not least, in combination with other wrapper types like Mono<Optional<T>> and CompletableFuture<Optional<T>>. It's not too hard to see how such a type or similar concept could be requested in more places.

Currently one can inject @Argument Map<String, ?> args and perform presence checks. Not super convenient, but I'm wondering how common the case for checking presence vs null is, or is it somewhat of a niche case.

I would be more in favor of some wrapper type like ArgumentValue<T> which could be used without @Argument, or with if the name needs to be specified explicitly. This type would expose the concept of present vs not present, value vs null, applying validation, etc. all through a functional, declarative API, like java.util.Optional. That said again, unless the need is common, I'd rather hold off on it for now.

[koenpunt]

Currently one can inject @Argument Map<String, ?> args

That is far from convenient, especially when having more complex/nested input types. I do think it's quite common, but maybe that's only from my perspective.

A rudimentary example; having a mutation to update 2 properties of a resource, for example name and description. But it should be possible to update only one of the two, by completely omitting a value. Where we would still want validation on the value if it is provided, so that explicit null values can be rejected.

type Mutation {
  updateResource(input: UpdateResourceInput!): UpdateResourcePayload!
}

input UpdateResourceInput {
  name: String
  description: String
}

In the meantime, would a PR be accepted to have the possibility to "intercept" the conversion/instantiation proces, so custom types like this can be implemented in user space?

[koenpunt]

For more flexibility with argument resolving, maybe a specific constructor or method could indicate to the argument resolver that it should be used to instantiate the object, instead of the built in data binder. Then it wouldn't be specific for optional inputs, but any arbitrary pre processing logic could be added for any argument.

[rstoyanchev]

@koenpunt apologies for the delay. We had a team discussion last week and I've been meaning to comment.

The required flag on @Argument, which has now been removed in #150, only applies to top level arguments, and I thought you meant the same here, but your #140 (comment) made me realize that it's intended to work nested on fields within input types. This paints a different picture and you do bring up a good point about this ability in GraphQL to specify partial input at any level.

We're now wondering about approaches. Perhaps a programmatic API to navigate the Map for an input argument with conversion/binding support could be useful. I see your proposal under #151 along similar lines, although I wonder how or if that's intended to work on more deeply nested fields? Other ideas could be JSONPath to check for presence/absence, or projection interfaces that mirror the input types to navigate to sub-parts and check for presence/absence.

Before we go farther, I would like to ask for some clarification. In your solution, do you mean for OptionalInput to be nested literally at any level, and if so what do you actually do with it thereafter? I don't imagine any easy ways to persist such an Object structure, not with JPA at least. Maybe with Mongo where you can check for keys and update documents, but even then it requires manual handling at different levels. Or do you tend to use this mostly for top-level fields within input types that you can inspect manually and then treat the data below those fields as aggregates that can be persisted?

[jurriaan]

It would be nice if we could differentiate between absence of fields and explicit nulls.

One use-case for this is supporting partial update mutations with nullable values. In those cases you need a tri-state input field like this example:

firstName	result
"John"	the firstName database field is updated with the value "John"
null	the firstName database field is updated with the value NULL
(undefined)	the firstName database field is not updated

This way the client can use a single mutation to update multiple fields of an entity without the need to specify all values in the input type or accidentally setting values to NULL by not specifying them.

[rstoyanchev]

Thanks for commenting @jurriaan. Yes the case for partial update is clear. As per my questions to @koenpunt in #140 (comment), I would also be interested in your feedback. How do you work with and persist such input? Obviously without a dedicated type, it's impossible to differentiate null from undefined, but a dedicated type would also be a challenge for JPA and others. Or do you manually check a few top-level fields and then treat their values as aggregates without making such checks at lower levels? The general question is how to expose this Map of data in a way that is convenient for the way it will actually be used.

[jurriaan]

We do have a layer separating the GraphQL and JPA layers where we do some sanitizing / extra checks anyway, and these checks are done on multiple layers of the input objects (i.e. we have nested input objects containing fields of OptionalInput<T> currently), this sanitized data is then persisted through a service that updates the JPA entities for example.

If this is too far fetched to implement in this codebase it would help if there is some kind of interface where we could add our own mappers to map the GraphQL input to some kind of Optional input type ourselves.

[koenpunt]

FYI @jurriaan and I are working on the same team.

Before we go farther, I would like to ask for some clarification. In your solution, do you mean for OptionalInput to be nested literally at any level, and if so what do you actually do with it thereafter? I don't imagine any easy ways to persist such an Object structure, not with JPA at least. Maybe with Mongo where you can check for keys and update documents, but even then it requires manual handling at different levels. Or do you tend to use this mostly for top-level fields within input types that you can inspect manually and then treat the data below those fields as aggregates that can be persisted?

To answer your questions;

do you mean for OptionalInput to be nested literally at any level

I indeed do think it should be possible to use this at any level.

what do you actually do with it thereafter

To give an example of our implementation;

We have a mutation with an input type;

type Mutation {
  productUpdate(productId: ID!, input: ProductInput!): ProductPayload!
}

input ProductInput {
  "The description of the product."
  description: String
  "The name of the product."
  name: String
  "The price of the product."
  price: Decimal
}

The implementation looks a bit like this;

fun productUpdate(productId: ID, input: ProductInput): ProductPayload {
    val name: Option<String> = when (input.name) {
        is OptionalInput.Defined -> {
            val value = input.name.value ?: throw IllegalArgumentException("Name can not be set to null")
            Some(value)
        }
        is OptionalInput.Undefined -> none()
    }
    // ...
}

Where you can see we validate, and transform the input type to an arrow.core.Option, a Kotlin optional container.

Later this optional is passed to an update method in our service class, which looks something in the lines of;

fun updateEntity(product: Product, name: Option<String>, description: Option<String?>, price: Option<BigDecimal?>): Product {
    name.map { product.name = it }
    description.map { product.description = it }
    // ...

Where Product is a JPA entity, and as you can see we only assign the values for which the Option has a defined value.

So no, we don't have a direct mapping with our JPA entities, we have custom logic in place for that.

---

As you see we're not necessarily looking for an OptionalInput type, since we do convert it to a different type internally anyway. But since graphql-kotlin did provide a type like that, and we are migrating from graphql-kotlin to spring-graphql, the name stayed.

So to echo @jurriaan's suggestion; having an interface to provide custom input mappers would probably be the most flexible.

NB. Data validation as is shown in the example above should probably be moved to a schema annotation, having the input be rejected at query parse time by using a custom directive.

[rstoyanchev]

Thanks for the extra details. I think we could allow the ConversionService on the DataBinder to be set and that should allow you to register a Converter and take control of conversion for specific inputs and target types, but I'll double check that.

[koenpunt]

I think we could allow the ConversionService on the DataBinder to be set and that should allow you to register a Converter and take control of conversion for specific inputs and target types

I noticed that the conversion service doesn't receive null values (see the implementation of the notes setter in the test), so AFAIK those can't be converted. But I have only limited knowledge of the spring frameworks, so maybe there's a way around which I don't know.

[koenpunt]

A quick update:

The conversion service added in 6975e6c covers a lot of the use cases I wanted to fix with this PR.

By defining various static methods like of(value: String) in my application's OptionalInput all simple types can be converted.

However properties with nested non-simple types can't, because those are passed as a map.

I could add an additional of(value: Map) method, but that would mean I have to reimplement part of the instantiator in application code.

This wouldn't be necessary once #155 gets merged, but then there's the problem that it tries to instantiate the OptionalInput class, with the properties of the wrapped type, instead of an instance of the wrapped type, which doesn't work.

I didn't yet figure out how to fix this, but maybe support for customization of the instantiator would allow wrapping and unwrapping custom types.

[koenpunt]

Some time has passed, and we still have hacks in place to support an "optional input" type, and thus I would still like to see a proper solution for this.

I did notice that it's now possible to assign a conversion service to the databinder, using AnnotatedControllerConfigurer.setDataBinderInitializer. However, this doesn't solve the issue with null values, because the conversion service is not called for null values.
That said, I'm not too familiar with the conversion service and PropertyEditors, so maybe I'm just missing some knowledge to make this work.

[rstoyanchev]

@koenpunt and @jurriaan, this has now been addressed as part of #518. Please, have a look there. Your feedback would be much appreciated!