Skip to content
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

AutoValue.Builder doesn't handle OptionalLong correctly. #1198

Closed
toadzky opened this issue Nov 8, 2021 · 8 comments
Closed

AutoValue.Builder doesn't handle OptionalLong correctly. #1198

toadzky opened this issue Nov 8, 2021 · 8 comments
Assignees

Comments

@toadzky
Copy link

toadzky commented Nov 8, 2021

It seems to treat it like it's just a regular Optional, but OptionalLong doesn't have the same methods.

If I have a builder class with

    OptionalLong getLongField();
....
    Builder setLongField(long value);

the generated code works fine:

    @Override
    public Builder setLongField(long value) {
      this.longField = OptionalLong.of(value);
      return this;
    }

This caused a problem with jackson because when it tries to parse an object with the field set as null, jackson fills in a 0 for the value and it turns into OptionalLong.of(0) - which is not the same as Optional.empty(). So I switched it to an object wrapper and marked it as @Nullable:

    Builder setLongField(@Nullable Long value);

but now it generates invalid java:

    public Builder setLongField(@Nullable Long value) {
      this.longField = OptionalLong.fromNullable(value);
      return this;
    }

OptionalLong doesn't have a fromNullable static constructor. This is broken up through at least 1.8.2

@eamonnmcmanus
Copy link
Member

Thanks, that does look like a bug. The fix is not completely straightforward, since I think we want it to generate this:

    public Builder setLongField(@Nullable Long value) {
      this.longField = (value == null) ? OptionalLong.empty() : OptionalLong.of(value);
      return this;
    }

The current code structure doesn't make that easy.

@eamonnmcmanus
Copy link
Member

I'm not able to reproduce this problem. Could you give complete code that produces it?

@toadzky
Copy link
Author

toadzky commented Nov 17, 2021

i misread the version in my project. i had 1.6.2, not 1.8.2. upgrading to 1.8.2 (which i thought i did in my testing, probably forgot to refresh intellij's maven stuff) gives me different problems. the primitive one still works but still has the same problem with jackson, but the object wrapper won't compile at all.

java: [AutoValueGetVsSet] Parameter type java.lang.Long of setter method should be java.util.OptionalLong to match property method Parent.getValue()

if i switch to a builder method that takes an OptionalLong, it works but the class api isn't as nice - it requires anyone using the builder in code to explicitly create and pass in an OptionalLong instead of a @Nullable Long.

@interface Nullable {}

interface Parent {
    OptionalLong getValue();
}

@AutoValue
@JsonDeserialize(builder = AutoValue_PrimitiveBuilderField.Builder.class)
abstract class PrimitiveBuilderField implements Parent {
    @AutoValue.Builder
    abstract static class PrimitiveBuilder {
        @JsonProperty("value")
        abstract PrimitiveBuilder value(long value);
        abstract PrimitiveBuilderField build();
    }
}

@AutoValue
//@JsonDeserialize(builder = AutoValue_ObjectWrapperBuilderField.Builder.class)
abstract class ObjectWrapperBuilderField implements Parent {

//    @AutoValue.Builder
    abstract static class Builder {
        @JsonProperty("value")
        abstract Builder value(@Nullable Long value);
        abstract ObjectWrapperBuilderField build();
    }
}

@AutoValue
@JsonDeserialize(builder = AutoValue_OptionalFieldBuilderField.Builder.class)
abstract class OptionalFieldBuilderField implements Parent {

    @AutoValue.Builder
    abstract static class Builder {
        @JsonProperty("value")
        abstract Builder value(OptionalLong value);
        abstract OptionalFieldBuilderField build();
    }
}
public class Main {

    public static void main(String[] args) throws Exception {
        final ObjectMapper objectMapper = new ObjectMapper().registerModule(new Jdk8Module());

        final PrimitiveBuilderField primitiveBuilderField = objectMapper.readValue("{\"value\": null}", PrimitiveBuilderField.class);
        System.out.println(primitiveBuilderField); // output: PrimitiveBuilderField{value=OptionalLong[0]}

        final OptionalFieldBuilderField objectWrapperBuilderField = objectMapper.readValue("{\"value\": null}", OptionalFieldBuilderField.class);
        System.out.println(objectWrapperBuilderField); // output: OptionalFieldBuilderField{value=OptionalLong.empty}
    }
}

Here's the full maven project
auto-value-optional-long-demo.zip

@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Nov 17, 2021

On the one hand, a @Nullable Long setter would work if the property was Optional<Long> rather than OptionalLong, so it's obvious what the behaviour should be if we supported this. On the other hand, OptionalLong itself doesn't have an ofNullable(Long) method, though it obviously could. That makes implementing this a little bit trickier and could also be used as a feeble rationale for not supporting it.

It's fairly straightforward to achieve the API you want with overloading:

@AutoValue
abstract class OptionalFieldBuilderField implements Parent {
  abstract Builder value(OptionalLong value);
  Builder value(@Nullable Long value) {
    return value(value == null ? OptionalLong.empty() : OptionalLong.of(value));
  }
  abstract OptionalFieldBuilderField build();
}

I don't know if that would do what you want with Jackson, though.

@eamonnmcmanus eamonnmcmanus added type=enhancement Make an existing feature better and removed type=defect Bug, not working as expected labels Nov 17, 2021
@toadzky
Copy link
Author

toadzky commented Nov 19, 2021

yeah, that was basically how i ended up working around it already. it's not ideal, since you get sonar and IDE warnings about having an optional as a parameter, but it is a viable workaround.

@ratheeshcn
Copy link

@eamonnmcmanus I am having an issue on Builder method, it generates the json with all the fields instead of the values we set on builder method.

these are my dependencies

const val autoValue = "com.google.auto.value:auto-value:1.10"
const val autoParcel = "com.ryanharter.auto.value:auto-value-parcel:0.2.9"
const val autoGson = "com.ryanharter.auto.value:auto-value-gson:1.3.1"

here is my data class


@AutoValue
public abstract class CreateBuyerBody implements Parcelable{


    @SerializedName("first_name")
    @Nullable
    public abstract  String firstName();

    @SerializedName("last_name")
    @Nullable
    public abstract  String lastName();

    @SerializedName("phone_numbers")
    @Nullable
    public abstract 
    List<PhoneNumber> phoneNumbers();

    @SerializedName("email_addresses")
    @Nullable
    public abstract  List<EmailAddress> emailAddresses();


    public static @NonNull TypeAdapter<CreateBuyerBody> typeAdapter(@NonNull Gson gson) {
        return new $AutoValue_CreateBuyerBody.GsonTypeAdapter(gson);
    }

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

    @AutoValue.Builder
    public static abstract class Builder {


        public abstract Builder firstName(@Nullable String firstName);
        public abstract Builder lastName(@Nullable String lastName);
        public abstract Builder phoneNumbers(@Nullable List<PhoneNumber> phoneNumbers);
        public abstract Builder emailAddresses(@Nullable List<EmailAddress> emailAddresses);
         public abstract CreateBuyerBody build();
    }
}

while accessing the builder method

CreateBuyerBody.Builder builder = CreateBuyerBody.builder();
builder.firstName("John");

it generates this

{"first_name":"John","last_name":null,"phone_numbers":null,"email_addresses":null}

instead of this

{"first_name":"John"}

Can you help me where i get wrong on this ? thanks in advance.

@eamonnmcmanus
Copy link
Member

@ratheeshcn I think this is an issue with auto-value-gson rather than AutoValue itself. It would make sense for auto-value-gson to omit null values from the serialized JSON string unless Gson.serializeNulls() is true. However that's not what it actually does. You might consider creating a bug or enhancement request there. There might be compatibility considerations, though.

@eamonnmcmanus
Copy link
Member

I'm going to close the original issue on the grounds that OptionalLong itself is null-hostile: you can write neither OptionalLong.ofNullable(x) nor optionalLong.orElse(null).

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

No branches or pull requests

3 participants