avro-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Clueless Joe (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (AVRO-1961) [JAVA] Generate getters that return an Optional
Date Thu, 01 Dec 2016 20:12:58 GMT

    [ 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)

Mime
View raw message