Add support for Optional

[panfngsugar]

AutoValue now supports Optional properties. Can we also add support for Optional Properties?

[acdebaca]

Would a pull request be considered for this feature if I provided one?

The current issue is the null guard in the generated code. Input:

@AutoValue
abstract class Offer {
    abstract String plainString();
    abstract Optional<String> optionalString();

    static TypeAdapter<Offer> typeAdapter(final Gson gson) {
        return new AutoValue_Offer.GsonTypeAdapter(gson);
    }
}

Output, read method only:

    @Override
    public Offer read(JsonReader jsonReader) throws IOException {
      if (jsonReader.peek() == JsonToken.NULL) {
        jsonReader.nextNull();
        return null;
      }
      jsonReader.beginObject();
      String plainString = null;
      Optional<String> optionalString = null; // This would be Optional.empty()
      while (jsonReader.hasNext()) {
        String _name = jsonReader.nextName();
        if (jsonReader.peek() == JsonToken.NULL) { // Null would propagate to optional adapter
          jsonReader.nextNull();
          continue;
        }
        switch (_name) {
          case "plainString": {
            plainString = plainStringAdapter.read(jsonReader);
            break;
          }
          case "optionalString": {
            optionalString = optionalStringAdapter.read(jsonReader);
            break;
          }
          default: {
            jsonReader.skipValue();
          }
        }
      }
      jsonReader.endObject();
      return new AutoValue_Offer(plainString, optionalString); // optionalString = null
    }

As a result, optionalString is never processed, and the parent $AutoValue_Offer constructor throws a validation exception because the argument is null.

Note: I wrote my own TypeAdapterFactory implementation that works with java.util.Optional. It would be trivial to add another for Guava Optional as well.

[rharter]

Yeah, a PR would be appreciated for this.

[acdebaca]

@rharter Happy to do it. Before proceeding, I'd like your guidance on one issue. auto-value-gson would have to include a TypeAdapter (and/or corresponding factory) for Java and Guava Optional.

1. Are you comfortable with furnishing TypeAdapter implementations as part of the library? These classes need to be present at runtime. I don't think that there is precedent for that in this project. If so, I assume they would go in the com.ryanharter.auto.value.gson package, but in which artifact?

2. Users that are not using @GsonTypeAdapterFactory would have to register the adapter/factory manually. For users that are using @GsonTypeAdapterFactory, should we automatically include it in the factory generated with @GsonTypeAdapterFactory if we detect an Optional property?

Thanks. Looking forward to contributing.

[acdebaca]

We could always generate the source for the Optional TypeAdapter classes as part of the output. The classes are very small.

[JakeWharton]

I would skip the type adapter. Get the delegate for the parameterized type like you would if it was nullable and then emit code which uses Optional.ofNullable before storing the value in the local.

On Fri, Sep 29, 2017 at 12:08 PM Alejandro J. C De Baca < notifications@github.com> wrote:

We could always generate the source for the Optional TypeAdapter classes as part of the output. The classes are very small.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rharter/auto-value-gson/issues/123#issuecomment-333169058, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEfMUMbtpTMaU-JOKRC6zkE94Asloks5snRYQgaJpZM4MkfCX .

[acdebaca]

I have something working, and I can submit the PR at this point. However, I had to make a few calls that I wanted to review ahead of the PR.

1. Properties with both @Nullable and Optional default to Optional.empty(), and not to null. During deserialization, if a JSON property is null or not present, the field will be Optional.empty().

2. If a collection type is used as the type parameter to an Optional, the default value is still Optional.empty(), regardless of defaultCollectionsToEmpty

3. Default setters - the default fields are initialized to Optional.empty(), and the setters take the wrapped type (or corresponding primitive type in the case of Optional{Double,Int,Long}. So:

   abstract Optional<Integer> optInt();
   abstract Optional<String> optStr();
   abstract OptionalInt optIntPrim();

   ...causes the following default setters to be generated:

   
   public GsonTypeAdapter setDefaultOptInt(Integer defaultOptInt) {
   this.defaultOptInt = Optional.ofNullable(defaultOptInt);
   return this;
   }
   public GsonTypeAdapter setDefaultOptStr(String defaultOptStr) {
   this.defaultOptStr = Optional.ofNullable(defaultOptStr);
   return this;
   }
   public GsonTypeAdapter setDefaultIntPrim(int defaultIntPrim) {
   this.defaultIntPrim = OptionalInt.of(defaultIntPrim);
   return this;
   }

4. There is a case in the code (which I don't think is covered by the regression tests) in which constructor parameters are added to the `GsonTypeAdapter` nested class for default values:
```java
if (defaultSetters && !prop.shouldDeserialize() && !prop.nullable()) {
// ...

I included non-optional properties to the check:

if (defaultSetters && !prop.shouldDeserialize() && !prop.nullable()
    && !prop.optionalSpec.isPresent()) {
//...

As a side note, shouldn't this condition be !defaultSetters instead of defaultSetters? If you have a setter method, why is it necessary to require the default value as a constructor param?

Anyway, if you agree with these decisions, I'll submit the PR. If not, happy to make changes based on your recommendations. Thank you.

[rharter]

These sounds mostly reasonable to me. My only question would be in regards to the email collections, but I'd need to see it.

Best thing to do it just open​ the PR and put these comments in the code where they apply. The discussion in the PR makes this kind of thing much easier as we can see the code.

On Sat, Sep 30, 2017, 3:02 AM Alejandro J. C De Baca < notifications@github.com> wrote:

I have something working, and I can submit the PR at this point. However, I had to make a few calls that I wanted to review ahead of the PR.

1.

Properties with both @Nullable and Optional default to Optional.empty(), and not to null. During deserialization, if a JSON property is null or not present, the field will be Optional.empty(). 2.

If a collection type is used as the type parameter to an Optional, the default value is still Optional.empty(), regardless of defaultCollectionsToEmpty 3.

Default setters - the default fields are initialized to Optional.empty(), and the setters take the wrapped type (or corresponding primitive type in the case of Optional{Double,Int,Long}. So:

abstract Optional optInt(); abstract Optional optStr(); abstract OptionalInt optIntPrim();

...causes the following default setters to be generated:

public GsonTypeAdapter setDefaultOptInt(Integer defaultOptInt) { this.defaultOptInt = Optional.ofNullable(defaultOptInt); return this; }public GsonTypeAdapter setDefaultOptStr(String defaultOptStr) { this.defaultOptStr = Optional.ofNullable(defaultOptStr); return this; }public GsonTypeAdapter setDefaultIntPrim(int defaultIntPrim) { this.defaultIntPrim = OptionalInt.of(defaultIntPrim); return this; }

1. There is a case in the code (which I don't think is covered by the regression tests) in which constructor parameters are added to the GsonTypeAdapter nested class for default values:

if (defaultSetters && !prop.shouldDeserialize() && !prop.nullable()) {// ...

I included non-optional properties to the check:

if (defaultSetters && !prop.shouldDeserialize() && !prop.nullable() && !prop.optionalSpec.isPresent()) {//...

As a side note, shouldn't this condition be !defaultSetters instead of defaultSetters? If you have a setter method, why is it necessary to require the default value as a constructor param?

Anyway, if you agree with these decisions, I'll submit the PR. If not, happy to make changes based on your recommendations. Thank you.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/rharter/auto-value-gson/issues/123#issuecomment-333291575, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPJblAZUA7d4FIzrN1HrKvr7Jz_mdiHks5snfM7gaJpZM4MkfCX .

[JakeWharton]

Why would @Nullable be represented with an Optional? That limits availability to Android API 24+ or the use of Guava–neither of which is acceptable.

[acdebaca]

@JakeWharton I was referring to cases like the following:

  @Nullable abstract Optional<String> a();
  @Nullable abstract OptionalLong b();

I can't think of a reason to do something like this, but AutoValue doesn't have a problem with it, and because there are two reasonable defaults (null or Optional.empty()), we have to choose which takes precedence, or make it configurable.

@rharter I'll comment the code and submit the PR.

Thank you!

[JakeWharton]

Ah, got it. It's weird that's allowed.

On Sat, Sep 30, 2017, 2:00 PM Alejandro J. C De Baca < notifications@github.com> wrote:

@JakeWharton https://github.com/jakewharton I was referring to cases like the following:

@Nullable abstract Optional a(); @Nullable abstract OptionalLong b();

I can't think of a reason to do something like this, but AutoValue doesn't have a problem with it, and because there are two reasonable defaults (null or Optional.empty()), we have to choose which takes precedence, or make it configurable.

@rharter https://github.com/rharter I'll comment the code and submit the PR.

Thank you!

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/rharter/auto-value-gson/issues/123#issuecomment-333325365, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEQMzZbJKByVpdTOb59MkVaDTpNExks5snoGzgaJpZM4MkfCX .

[acdebaca]

I omitted updating the documentation and examples, so please let me know if you'd like those in the PR as well. Thanks.

[acdebaca]

@rharter and @JakeWharton - just making sure you aren't waiting on me for anything. This is my first open-source PR, so apologies if I'm unfamiliar with the process/etiquette. Thanks.