Clueless Joe commented on AVRO-1961:


In the last few days I had the time to clarify why I was willing to go for 
optional: currently there's no easy way for an Avro pojo consumer to know 
whether a field is nullable or not.

This leads to many nasty NPE when consuming messages. And in early stage of 
development, if a field becomes nullable, this is invisible to all the 
consumers, even if at the time they made sure to check nullability. The code 
goes on compiling... and then may break anytime.

How could we avoid this?

Optional<Foo> getFoo() for nullable fields and Foo getFoo() would non nullable 
fields would make sure everyone must handle nullabity when present, because 
nullabity would be expressed in a type safe way. No more looking around to 
check whether the field is nullable or not. And if a field becomes nullable (or 
non nullable), the consumer's code doesn't compile anymore.

Regarding setter/builder/constructor: considering the initial goal, ie making 
obvious if a field is nullable or not, and what was achieved with getter 
through typesafety, I would like the same level of warranty. Which means having 
setter/builder/constructor with Optional<Foo> for nullable fields.

Once again, nullability would be ensured in a typesafe way. If it changes in 
the schema, then the code using the generated pojo doesn't compile anymore.

And we would gain extra readability: now a setter/builder/constructor would 
tell explicitly whether a field is required or not.

Actually, on the setter/builder/constructor, we could even go a step further: a 
required field with a default value could be with Optional<Foo> as well. The 
matching getter would be without Optional. It would make the concept of default 
value type safe just as well. Sweet isn't it? 

And, in setter for a non nullable value, we could add some null check to make 
sure nulls don't make it through.

In the end, the name of the settings on the specific compiler, currently 
gettersReturnOptionalOnNullableFields, isn't appropriate anymore. It should be 
something like "pojoWithOptional" (suggestions welcome!).

Do we agree ?

Technically speaking:

1. Java 8 isn't required for Avro project for this to work

All the "magic" happen uniquely through the template: the code could still 
compile with Java 6, but anyone using the "pojoWithOptional" flag should 
consume the generated pojo with Java 8 or backport Optional.

2. Only 2 news methods required in the Schema class

There is a need for isNullable and hasDefault methods on the Schema class, to 
use in the template. In case this "pojoWithOptional" proposal isn't accepted, 
could these 2 methods still be added?
3. Not test for generated pojo in Avro's project code base AFAIK

I haven't seen any test of the generation outcome. If I'm wrong, please tell me.

If not, then I would add something along these lines:
- defaultPojoGenerationSafetyNetTest: generating a pojo with default settings 
from a moderatly complex schema and then inspecting the generated .java class 
to make sure of its content. I would use this a "safety net" for the changes 
- pojoWithOptionalGenerationTest: generating a pojo with "pojoWithOptional" 
from the same schema as above and then inspecting it to make sure the above 
rules are respected.

4. No arg constructor limit

No arg constructors are needed for Avro's generated builders. Yet they may 
break the wanted type safety: an instance created with the no arg constructor 
may have non nullable fields being nulls. To limit this issue, I'm thinking of 
adding a warning in the javadoc when pojoWithOptional is set. Something along 
the lines: used internally by Avro, do not use.

5. Current pull request change imports

My current pull request, https://github.com/apache/avro/pull/165, changes the 
imports order. This is bad, I'll fix it.


@Gabor Szadovszky:
Actually, for the setters/builders, having both setFoo(Foo foo) and 
setFoo(Optional<Foo> foo) would be cumbersome when passing null, which then 
would have to be cast explicitly.

Regarding the getter, I agree with you :)

@Tom White
Having Optional<Foo> getFoo() instead of Foo getFoo() is made through a 
dedicated setting. It won't break backward compatibility unless you set the 
parameter to true.

About this quote "I think routinely using it as a return value for getters 
would definitely be over-used", it all depends of the context and the intent. 
For me the goal is to get rid of NullPointerException because the message's 
consumer didn't know/care which fields are nullable. These NPEs are really 
annoying and currently require extensive unit testing to (try to) prevent. 
Having type safety there would make nullability obvious.

@Niels Basjes 
In your comment of the 23/Nov/16 11:12, you said :
"The idea is that the downstream application code becomes more readable when 
retrieving a field that is buried deep in a nested structure.
With Optionals you can rely on the fact that there will always be a value 
returned and this removes the need for having many if (foo != null) type 

With the above proposal, you would be sure a getFoo would always return a 
value, so chaining call would also be straigthforward.

Any comment welcome, all this pretty long text is just a suggestion :)


> [JAVA] Generate getters that return an Optional
> -----------------------------------------------
>                 Key: AVRO-1961
>                 URL: https://issues.apache.org/jira/browse/AVRO-1961
>             Project: Avro
>          Issue Type: New Feature
>            Reporter: Niels Basjes
>            Assignee: Niels Basjes
>            Priority: Minor
> A colleague of mine indicated that having getters that return an Optional 
> (java 8 thingy) would be very useful for him.

This message was sent by Atlassian JIRA

Reply via email to