Protobuf: Builder set methods null friendly

[nykolaslima]

Hello guys,
I'm using protobuf 3 beta version(Java) and I'm wondering if it wouldn't be better to make Builder's setter methods null friendly.

In the majority of the systems the protobuf builders will be used in order to transform some domain class into the protobuf class, that will be exposed to other applications.
When we are mapping a class to another, usually we end up with something like this:

AddressResource.newBuilder()
                .setStreet(address.getStreet())
                .setStreetNumber(address.getStreetNumber())
                .build();

The problem is that if we have a null value into street, then AddressResource.Builder will thrown a NullPointerException. In order to avoid it, we need to do:

AddressResource.Builder builder = AddressResource.newBuilder();
if(address.getStreet() != null) {
builder.setStreet(address.getStreet());
}
if(address.getStreetNumber() != null) {
builder.setStreetNumber(address.getStreetNumber());
}
builder.build();

This is much more verbose and make the code looks more complicated than it should be.

Is there any reason to Builder's setters to thrown NullPointerException instead of just ignore the value?

[xfxyjwf]

I think accepting null values will make the API more confusing and error prone. For example, your proposal is one possible behavior of setFoo(null). There can be others like:

if (address.getStreet() != null) {
  bulider.setStreet(address.getStree());
} else {
  builder.clearStreet();
}

It's easy to misunderstand what setFoo(null) actually does and use it incorrectly. That seems to me a good enough reason to not change the current behavior.

[nykolaslima]

Actually, if you have a null and you are setting it to a field, you should assume the field type default value, don't you?

For example, protobuf defines that default value for string is empty string "". If I'm setting null into a string, I assume that the default value will be set.

If you want to use a behaviour different from the default, than you must implement it on your own:

if(address.getStreet() != null) {
  builder.setStreet(address.getStreet());
} else {
  builder.setStreet("not informed");
}

[xfxyjwf]

Current implementation:

Builder setFoo(Foo value) {
  if (value == null) throw exception;
}

Possible alternatives:

// option 1: ignore null
Builder setFoo(Foo value) {
  if (value == null) return;
}

//option 2: set the default value
Builder setFoo(Foo value) {
  if (value == null) { this.foo = defaultValue; return; }
}

You were originally proposing "option 1: ignore null vaules", right?

[nykolaslima]

Sorry, I believe that it would be better to set the default value. IMO, it makes more sense to assume that "none value" will be set as the default value.

[nickwongwong]

I encounter the same issue, the two answers may help.
BTW disappointed for can not use chained build methods
Handling null values protobuffers
why protobuf optional field does not take null

[haberman]

Closing as we have no plans to make any changes to this API.

[m-sasha]

Which is clearer?

return AddressOuterClass.Address.newBuilder()
        .setCountry(address.getCountry())
        .setState(address.getState())
        .setCity(address.getCity())
        .setStreet(address.getStreet())
        .setHouseNumber(address.getHouseNumber())
        .setZipCode(address.getZipCode())
        .setDescription(address.getDescription())
        .build();

or

AddressOuterClass.Address.Builder builder = AddressOuterClass.Address.newBuilder();
if (address.getCountry() != null){
    builder.setCountry(address.getCountry());
}
if (address.getState() != null){
    builder.setState(address.getState());
}
if (address.getCity() != null){
    builder.setCity(address.getCity());
}
if (address.getStreet() != null){
    builder.setStreet(address.getStreet());
}
builder.setHouseNumber(address.getHouseNumber());
if (address.getZipCode() != null){
    builder.setZipCode(address.getZipCode());
}
if (address.getDescription() != null){
    builder.setDescription(address.getDescription());
}

return builder.build();

[jbfbell]

Just seeing this now, it is a readability bummer. Heres an _untested_ workaround that might hopefully make it more readable.

public static <T> void safeSet(T value, Consumer<T> setter) {
  if(value != null) {
    setter.accept(value);
  }
}

SomeClass.Builder builder = SomeClass.newBuilder();
safeSet("foo", builder::setFoo);

[m-sasha]

@jbfbell That doesn't help as much as it could, because you can't chain the calls to safeSet. Here's a better alternative:

public class ProtoBuilder<MessageType extends GeneratedMessageLite<MessageType, BuilderType>,
    BuilderType extends GeneratedMessageLite.Builder<MessageType, BuilderType>>{


    /**
     * The underlying builder actually doing the work
     */
    private final BuilderType builder;


    /**
     * Creates a new {@link ProtoBuilder}.
     */
    private ProtoBuilder(BuilderType builder){
        this.builder = builder;
    }


    /**
     * Creates a new {@link ProtoBuilder} with the given builder to do the actual work.
     */
    public static <MessageType extends GeneratedMessageLite<MessageType, BuilderType>,
        BuilderType extends GeneratedMessageLite.Builder<MessageType, BuilderType>>
        ProtoBuilder<MessageType, BuilderType> protoBuilder(BuilderType builder){
        return new ProtoBuilder<>(builder);
    }


    /**
     * Sets the given value, using the given setter, only if the value is not {@code null}.
     *
     * @return this proto builder, for chaining calls.
     */
    public <T> ProtoBuilder<MessageType, BuilderType> set(Functions.BiFunction<BuilderType,T,BuilderType> setter, T value){
        if (value != null){
            setter.apply(builder, value);
        }

        return this;
    }


    /**
     * Builds and returns the target object.
     */
    public MessageType build(){
        return builder.build();
    }


}

Use like so:

protoBuilder(ProtobufType.newBuilder())
    .set(ProtobufType::setRequiredField, value1)
    .set(ProtobufType::setOptionalField, value2)
    .build()

Of course, it would be better still if the builder allowed null values.

[daxlab]

@m-sasha how can we achieve similar implementation for proto3 ?
Do we need to extend com.google.protobuf.GeneratedMessageV3 ?

[m-sasha]

@daxlab Yes, it seems like that should do the trick, if the structure of builders is the same in the regular Java implementation.

[daxlab]

@m-sasha I tried implementing it, but it the set method isn't able to resolve ProtoBufType::mySetter .

[daxlab]

@m-sasha FYI

import com.google.protobuf.Message;

import java.util.function.BiFunction;
import com.google.protobuf.GeneratedMessageV3;


public class ProtoBuilder<MessageType extends GeneratedMessageV3,
        BuilderType extends GeneratedMessageV3.Builder<BuilderType>> {


    /**
     * The underlying builder actually doing the work
     */
    private final GeneratedMessageV3.Builder<?> builder;


    /**
     * Creates a new {@link ProtoBuilder}.
     */
    private ProtoBuilder(GeneratedMessageV3.Builder<?> builder) {
        this.builder = builder;
    }


    /**
     * Creates a new {@link ProtoBuilder} with the given builder to do the actual work.
     */
    public static <MessageType extends GeneratedMessageV3,
            BuilderType extends GeneratedMessageV3.Builder<BuilderType>>
    ProtoBuilder<MessageType, BuilderType> protoBuilder(GeneratedMessageV3.Builder<?> builder) {
        return new ProtoBuilder<>(builder);
    }


    /**
     * Sets the given value, using the given setter, only if the value is not {@code null}.
     *
     * @return this proto builder, for chaining calls.
     */
    public <T> ProtoBuilder<MessageType, BuilderType> set(BiFunction<GeneratedMessageV3.Builder,T,GeneratedMessageV3.Builder> setter, T value) {
        if (value != null) {
            setter.apply(builder, value);
        }

        return this;
    }


    /**
     * Builds and returns the target object.
     */
    public Message build() {
        return builder.build();
    }
}


// My usage
return ProtoBuilder.protoBuilder(MyOuter.MyPr.newBuilder())
                .set(MyOuter.MyPr::setId, pojo.getId())
                .build();

setId is not resolved.

Any idea ? I guess we are not returning Builder from set function.

ST
```
incompatible types: invalid method reference
[ERROR] method setId in class*.Builder cannot be applied to given types
[ERROR] required: long
[ERROR] found: java.lang.Object,java.lang.Object

[micahstairs]

What if we added "setXxxxxIfPresent" methods? It would take Optional and then only set the field if it is present.

This is both readable and safe!

[hirowan]

Current implementation:

Builder setFoo(Foo value) {
  if (value == null) throw exception;
}

Possible alternatives:

// option 1: ignore null
Builder setFoo(Foo value) {
  if (value == null) return;
}

//option 2: set the default value
Builder setFoo(Foo value) {
  if (value == null) { this.foo = defaultValue; return; }
}

You were originally proposing "option 1: ignore null vaules", right?

option 1 : if (value == null) return this; how to do it? I face this problem too.thank you

[deanorderly]

ouch, more importantly, how do I do the variations of json then? json wire format options

"name": {defaultValue=0 for int} is on the wire
"name": null
{name not exist}

These are all used in the wild as different triggers. If the protobuf binary format can't tell the difference between these 3 things, then it can't truly do json, can it?

[deanorderly]

related to nulls, I just posted this on JSON... https://stackoverflow.com/questions/61541583/does-protobuf-support-json-properly-for-nulls-since-null-support-is-odd

[ryanhoo]

Wouldn't it be nice to have a @NonNull annotation for the builder.setXXX's parameter, so when we try to set the field at least we can get a warning to do null check?