Null fields should be serialized as 0 instead of 1

[lcacheux]

In the generated writeToParcel method, null values are serialized as 1, non null as 0 :

if (getValue() == null) {
  dest.writeInt(1);
} else {
  dest.writeInt(0);
  dest.writeString(getValue());
}

This cause issues when this parcelable is exchanged between applications with different versions of the object. For example :

v1 lib have :

@AutoValue
public abstract class MyObject implements Parcelable {
  @Nullable public abstract String getName();
  @Nullable public abstract Integer getValue();
}

At some point we added some new fields to this object to create a v2 version of our lib :

@AutoValue
public abstract class MyObject implements Parcelable {
  @Nullable public abstract String getName();
  @Nullable public abstract Integer getValue();
  @Nullable public abstract String getSurname();
  @Nullable public abstract Integer getNewValue();
}

An application using v1 should still be able to send the object to an application using v2, with null value for the new fields. But with the existing implementation, the generated createFromParcel method will be :

return new AutoValue_MyObject(
  in.readInt() == 0 ? in.readString() : null,
  in.readInt() == 0 ? in.readInt() : null,
  in.readInt() == 0 ? in.readString() : null,
  in.readInt() == 0 ? in.readInt() : null
)

The first two fields are deserialized properly, but since the buffer is empty after this, readInt will always return 0, meaning that the object is not null. The second readString() will return null so the value for the second string is null as expected, but the second Integer value is 0 instead of null (same issue should happen with classes that wrap primitive types).

This behavior would not happen by inverting 0 and 1 for null/non null. Is there a reason why 0 is used for non null instead of null ?

Also changing this behavior now could result in many more backward compatibility breaks for application that used a previous version of auto-value-parcel.

[rharter]

That's a good point. I was checking the Parcel source because I recall this being unintuitive, but based on that, but it looks like Parcel objects use -1 for null, so that couldn't have been it.

This is based on a snippet suggestion in #16, do you recall if there was specific reasoning why we used 1 to mean null instead of 0, @JakeWharton?

The only way I can think of to support a change like this in a backwards compatible way would be to add an AVP version code to the beginning of each generated write/read method to tell us how to decide which functionality to use, but that could be problematic when mixing with other adapters. I'm open to other ideas without breaking this in the same use case by introducing this change.

[JakeWharton]

Can look later, but parcelables in general don't need forward/backward compatibility since the format is not stable and they can't be persisted. The only example where this comes up is two processes with different versions talking to each other. If this change solves that in a semi-reasonable way then we can make it. Beyond that, though, you're still not guaranteed that this will work. You really need to use something like protos where the format and compatibility guarantees are part of the library.

[rharter]

Yeah, I think this is a report of that exact use case. We could fix it for all future multiversion interior cases, but that would break all existing ones.

[lcacheux]

Yes, my use case here is a service used by other applications which can have different versions, and I can't change to something else than parcelables.

The solution I found for now to not break the existing apps is using a custom version of auto-value-parcel, with annotation set on new fields to use 0 for null, but it's not really clean.

Most of all I wanted to know if there was a reason to use 1 for null instead of 0.

[ZacSweers]

What does Kotlin's @Parcelize do? Since this library isn't stable I don't think we're really beholden to backward compatibility. Something we could do is switch it over if there's a more conventional approach, and then add a legacy opt-in via apt flag or something.

[rharter]

I think the lack of 1.0 is a good point, and I'm down for that. Let's change it to match the platform implementation. Unless anyone has objections, I'd vote to skip the opt in, as that would just add some complexity to the implementation.

Given that this library is currently at 0.2.7, the Parcel spec is considered "unstable" in that it doesn't make promises about the implementation details, and the fact that we differ from the platform, which breaks interop with framework implementations, I think it's safe to make the change.

[ZacSweers]

That sounds good to me. I’ll pr a change