[
https://issues.apache.org/jira/browse/AVRO-1961?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15712947#comment-15712947
]
Clueless Joe commented on AVRO-1961:
------------------------------------
Hi
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
made.
- 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.
Misc:
@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
checks."
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 :)
cheers
> [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
(v6.3.4#6332)
