Differentiating explicit null from default values

[Frederick888]

Is your feature request related to a problem? Please describe.

We'd like to use the generated models for a Merge Patch API.

It's similar to https://tools.ietf.org/html/rfc7396, where missing fields in the request payload are left untouched, and explicit null fields are unset.

At the moment there doesn't seem to be a place to track the explicitly null'ed fields and by default they are missing from request payloads.

Describe the solution you'd like

Add a magic map field (e.g. Set<?> changedFields) or a magic boolean field for each actual field (e.g. for someField it can have boolean isSomeFieldChanged) to track changed fields via field mutators, and include these fields in serialisation result even if they are null.

Describe alternatives you've considered

Copy out the generated models and manually do what I described above.

[kobylynskyi]

@Frederick888 thanks for your feature request.
This seems doable by slightly changing serialization logic by:

• Removing a null-check in GraphQLRequestSerializer#buildQuery
• Changing a generated Builder class in QueryRequest classes (.ftl) to populate only fields which were explicitly set by the client.

[Frederick888]

@kobylynskyi Thanks for your quick response!

I'm not very familiar this project so please correct me if I'm wrong, but if we are going to track explicitly set fields in the Builders, will it also track regular mutator calls such as xyz.setSomeField(foo)? And I'm curious about how this info is going to be stored and passed to serialisers?

(Am I actually talking about the same Builder here? I think you meant Builders in models generated from those templates? For instance below.)

public static class Builder {

    private String $alias;
    private UpdatePersonInputTO input;

    public Builder() {
    }

    public Builder alias(String alias) {
        this.$alias = alias;
        return this;
    }

    public Builder setInput(UpdatePersonInputTO input) {
        this.input = input;
        return this;
    }


    public UpdatePersonMutationRequest build() {
        UpdatePersonMutationRequest obj = new UpdatePersonMutationRequest($alias);
        obj.setInput(input);
        return obj;
    }

}

And inspired by @robbertnoordzij's comment, what about using null for defaults, Optional.empty() for explicitly set nulls and Optional.of(...) for other regular values?

[Frederick888]

I tried out the Optional<> solution and it seems to be roughly ok: Frederick888@50d7ecd

@kobylynskyi What do you think?

[robbertnoordzij]

Seems plausible, I would only want it for nullable fields and as an option.

[Frederick888]

@robbertnoordzij Thanks for the comment, that makes sense.

Second try: Frederick888@c449cb2

A few questions:

1. Not sure if we should mark both jackson-databind and jackson-datatype-jdk8 as providedCompile? I'm not terribly familiar with Java's dependency resolution, and I marked the latter as providedCompile simply because without it I got

   Execution default of goal io.github.kobylynskyi:graphql-codegen-maven-plugin:5.1.0:generate failed: A required class was missing while executing io.github.kobylynskyi:graphql-codegen-maven-plugin:5.1.0:generate: com/fasterxml/jackson/datatype/jdk8/Jdk8Module
   

   ...in my project.
2. I'm not entirely certain about https://github.com/Frederick888/graphql-java-codegen/blob/c449cb2956a0eb1560f278103090e9634aee8d82/src/main/resources/templates/java-lang/javaClassGraphqlType.ftl#L97-L103. The return type comes from c8df485, not sure if my change plays well with it.

[Frederick888]

By the way I just realised that this seems to be in GraphQL specs: graphql/graphql-spec#83

Related graphql-java PR: graphql-java/graphql-java#156

[robbertnoordzij]

From the specification it is clear, however I do not see how they solved in graphql Java at this moment. I would expect an optional in such case for inputs and projection. Meaning:

null not provided
Optional.empty() provided but empty
Optional.of() provided not empty

But the provided but empty in the spec is also null so that might be even more confusing.

[Frederick888]

@robbertnoordzij You were right. The PR closed graphql-java/graphql-java#106 but only solved the issue partially (IIUC it omits null fields which haven't got default values from the final result, but provides no way to explicitly set null value).

graphql-java/graphql-java#452 is the one that actually addressed this issue.

But the provided but empty in the spec is also null so that might be even more confusing.

I agree. They were probably just trying to stick to JSON terminology though.

[Frederick888]

@robbertnoordzij By the way, what were your thoughts on my second try?

[robbertnoordzij]

I am sorry, I totally missed the update on this issue. I am like you second approach with the Optional.ofNullable. However, I would suggest to this only for Nullable fields, since a non nullable field should always have a value and can only be null if it was not requested by the client.

[Frederick888]

@robbertnoordzij Hmmm... which part are you referring to? In my second try it's not using Optional for mandatory fields, unless I overlooked something?

For example, in the following generated model, tenantId is optional and name is mandatory:

import com.kobylynskyi.graphql.codegen.model.graphql.GraphQLRequestSerializer;
import java.util.StringJoiner;
import java.util.Optional;

import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonInclude.Include;

@javax.annotation.processing.Generated(
    value = "com.kobylynskyi.graphql.codegen.GraphQLCodegen",
    date = "2021-05-26T17:12:05+1000"
)
@JsonInclude(Include.NON_NULL)
public class PersonTO implements java.io.Serializable, IClientTO {
    private Optional<String> tenantId;
    @javax.validation.constraints.NotNull
    private String name;

    public PersonTO() {
    }

    public PersonTO(String tenantId, String name) {
        this.tenantId = Optional.ofNullable(tenantId);
        this.name = name;
    }

    public PersonTO(Optional<String> tenantId, String name) {
        this.tenantId = tenantId;
        this.name = name;
    }

    public String getTenantId() {
        return (tenantId == null || tenantId.isEmpty()) ? null : tenantId.get();
    }
    public void setTenantId(String tenantId) {
        this.tenantId = Optional.ofNullable(tenantId);
    }

    public String getName() {
        return name;
    }
    public void setName(String name) {
        this.name = name;
    }

    @Override
    public String toString() {
        StringJoiner joiner = new StringJoiner(", ", "{ ", " }");
        if (tenantId != null && tenantId.isPresent()) {
            joiner.add("tenantId: " + GraphQLRequestSerializer.getEntry(tenantId));
        }
        if (name != null) {
            joiner.add("name: " + GraphQLRequestSerializer.getEntry(name));
        }
        return joiner.toString();
    }

    public static PersonTO.Builder builder() {
        return new PersonTO.Builder();
    }

    public static class Builder {

        private Optional<String> tenantId;
        private String name;

        public Builder() {
        }

        public Builder setTenantId(String tenantId) {
            this.tenantId = Optional.ofNullable(tenantId);
            return this;
        }

        public Builder setName(String name) {
            this.name = name;
            return this;
        }

        public PersonTO build() {
            return new PersonTO(tenantId, name);
        }

    }
}

And by the way, I've also reverted types like Integer to primitive int for mandatory fields in my second try.

[kliakos]

For example, in the following generated model, tenantId is optional and name is mandatory:

Shouldn't there be an additional method like the following to be able to know if value has been explicitly passed as null or was absent?

public boolean isTenantIdDefined() {
  return tenantId != null
}