AVRO-3641: Adds support for nullSafeAnnotations to java SpecificCompiler #2142

peknu · 2023-03-13T09:37:48Z

What is the purpose of the change

*This pull request adds support for JetBrains @Nullable and @NotNull annotations based on schema.isNullable(), fixing AVRO-3641.

Verifying this change

This change added tests and can be verified as follows:

Run TestSpecificCompilerTool.compileSchemaWithNullSafeAnnotationsFields() to fully verify the generated java code

Documentation

This pull request introduces a new feature for null safety and is documented using JavaDoc in SpecificCompiler class.
Adding configuration option <createNullSafeAnnotations>true<createNullSafeAnnotations/> enables this feature.
It is disabled by default to conform to the default behavior.

# Conflicts: # lang/java/pom.xml # lang/java/tools/src/test/java/org/apache/avro/tool/TestSpecificCompilerTool.java

artheus · 2023-03-13T13:53:58Z

👍🏻

...ls/src/test/compiler/output-string/avro/examples/baseball/NullSafeAnnotationsFieldsTest.java

opwvhk · 2023-03-14T09:01:53Z

Given that this package main use case are the command line tools / build plugins, I'm fine with adding the jetbrains dependency.

phile314 · 2023-03-14T12:30:32Z

I was just now looking for this feature, great to see that this is already worked on :-)

As there are many nullable annotations around, would it make sense to make it configurable?

opwvhk · 2023-03-16T09:16:34Z

lang/java/ipc/pom.xml

+    <dependency>
+      <groupId>org.jetbrains</groupId>
+      <artifactId>annotations</artifactId>
+    </dependency>


Is this dependency actually used here?

opwvhk · 2023-03-16T09:17:25Z

lang/java/compiler/pom.xml

+    <dependency>
+      <groupId>org.jetbrains</groupId>
+      <artifactId>annotations</artifactId>
+    </dependency>


I only see the dependency being used in the test code (which is perfect IMHO). Can we please reduce the scope to test?

opwvhk · 2023-03-16T09:19:25Z

As there are many nullable annotations around, would it make sense to make it configurable?

I think it does. Also, your question triggered me to take a closer look at the code & dependencies. It would actually be a good change, and relatively easy to do as well.

opwvhk

I've left a few comments wrt use of dependencies.

I think a configurable approach, where the test configuration explicitly names the org.jetbrains.annotations.Nullable and org.jetbrains.annotations.NotNull annotations.

…e it is only used in the produced jar

karlll · 2023-03-25T14:25:08Z

👍

peknu · 2023-05-10T14:42:25Z

The JSR to develop standard annotations (such as @nonnull) for Java is dead since August 2006: https://jcp.org/en/jsr/detail?id=305

Maybe this PR can be merged so that the most commonly used @nullable and @NotNull annotations can be used. This would be especially useful for Kotlin based projects, since it would add compile time safety when using Avro.

This reverts commit f99cf16.

opwvhk

In general, this code looks good (nice, simple & clean).

As per the comments (also left by others), I think it would be better to prepare the template for using different annotation classes (i.e., remove the import statement). Actually implementing for other @Nullable/@NoptNull annotations is a lot easier then.

...ompiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm

opwvhk

In general, this code looks good (nice, simple & clean).

As per the comments (also left by others), I think it would be better to prepare the template for using different annotation classes (i.e., remove the import statement). Actually implementing for other @Nullable/@NoptNull annotations is a lot easier then.

…mports

…Annotations

peknu · 2023-08-16T12:10:46Z

Is there anything I can do to get this PR merged? It has been ready to merge for quite some time now?

opwvhk · 2023-08-16T14:25:52Z

lang/java/maven-plugin/src/main/java/org/apache/avro/mojo/AbstractAvroMojo.java

+  /**
+   * The createNullSafeAnnotations parameters adds @Nullable and @NotNull
+   * annotations for fhe fields of the record. The default is to not include
+   * annotations.


Can we please mention here that the annotations are JetBrains annotations?

Then it'll automatically be documented by the plugin help. Ideally, it also lists the dependency coordinates / package-url (pkg:maven/org.jetbrains/[email protected]).

At this point in time, there are no other places on the website that document the maven plugin, or code generation options. It's only mentioned in passing on the "Getting started" page for Java.

peknu · 2023-10-05T08:44:48Z

Any chance to get this merged?

m-kay · 2024-01-18T10:10:40Z

@opwvhk Is there any planned version where this will be released? This would really help us a lot when working with Avro in Kotlin projects.

opwvhk · 2024-01-23T19:00:24Z

@m-kay It will be in the next release, version 1.12.0. The most authoritative source on it is the dev mailing list.

…ler (apache#2142) * AVRO-3641: Adds support for nullSafeAnnotations to java SpecificCompiler * AVRO-3641: Removes maven dependency to org.jetbrains:annotations since it is only used in the produced jar * AVRO-3641: Removes the unused property for jetbrains-annotations version * AVRO-3641: Removes empty lines from expected output file * Revert "AVRO-3641: Removes empty lines from expected output file" This reverts commit f99cf16. * AVRO-3641: Use full class name for null safe annotations instead of imports * AVRO-3641: Adds avro-maven-plugin config parameter for createNullSafeAnnotations * AVRO-3641: Clarifies the source of the nullability annotations * AVRO-3641: Minor javadoc formatting. * AVRO-3641: Fixes spotless formatting * AVRO-3641: Adds license exclude for NullSafeAnnotationsFieldsTest.java --------- Co-authored-by: pknu <[email protected]>

AVRO-3641: Adds support for nullSafeAnnotations to java SpecificCompiler

e660842

github-actions bot added build Java Pull Requests for Java binding labels Mar 13, 2023

Merge branch 'master' into AVRO-3641

b4b8593

# Conflicts: # lang/java/pom.xml # lang/java/tools/src/test/java/org/apache/avro/tool/TestSpecificCompilerTool.java

opwvhk reviewed Mar 14, 2023

View reviewed changes

...ls/src/test/compiler/output-string/avro/examples/baseball/NullSafeAnnotationsFieldsTest.java Show resolved Hide resolved

opwvhk approved these changes Mar 14, 2023

View reviewed changes

opwvhk reviewed Mar 16, 2023

View reviewed changes

opwvhk requested changes Mar 16, 2023

View reviewed changes

AVRO-3641: Removes maven dependency to org.jetbrains:annotations sinc…

dcd68f8

…e it is only used in the produced jar

AVRO-3641: Removes the unused property for jetbrains-annotations version

5a4b481

github-actions bot removed the build label May 10, 2023

peknu added 2 commits May 10, 2023 17:31

AVRO-3641: Removes empty lines from expected output file

f99cf16

Revert "AVRO-3641: Removes empty lines from expected output file"

5533e8c

This reverts commit f99cf16.

peknu requested a review from opwvhk May 30, 2023 05:55

opwvhk requested changes Jun 1, 2023

View reviewed changes

...ompiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm Outdated Show resolved Hide resolved

opwvhk requested changes Jun 1, 2023

View reviewed changes

peknu added 2 commits June 2, 2023 17:03

AVRO-3641: Use full class name for null safe annotations instead of i…

d6d33d7

…mports

AVRO-3641: Adds avro-maven-plugin config parameter for createNullSafe…

f747ffd

…Annotations

peknu requested a review from opwvhk June 2, 2023 15:08

opwvhk reviewed Aug 16, 2023

View reviewed changes

opwvhk approved these changes Aug 16, 2023

View reviewed changes

AVRO-3641: Clarifies the source of the nullability annotations

39d1bed

pknu-sbab added 4 commits September 1, 2023 10:00

AVRO-3641: Minor javadoc formatting.

5238426

AVRO-3641: Fixes spotless formatting

592b8ee

AVRO-3641: Merge with master

e513365

AVRO-3641: Adds license exclude for NullSafeAnnotationsFieldsTest.java

af2d04f

github-actions bot added the build label Sep 25, 2023

opwvhk merged commit 2b19559 into apache:main Oct 23, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AVRO-3641: Adds support for nullSafeAnnotations to java SpecificCompiler #2142

AVRO-3641: Adds support for nullSafeAnnotations to java SpecificCompiler #2142

peknu commented Mar 13, 2023 •

edited

Loading

artheus commented Mar 13, 2023

opwvhk commented Mar 14, 2023

phile314 commented Mar 14, 2023

opwvhk Mar 16, 2023

opwvhk Mar 16, 2023

opwvhk commented Mar 16, 2023

opwvhk left a comment

karlll commented Mar 25, 2023

peknu commented May 10, 2023

opwvhk left a comment

opwvhk left a comment

peknu commented Aug 16, 2023

opwvhk Aug 16, 2023

opwvhk Aug 16, 2023

peknu commented Oct 5, 2023

m-kay commented Jan 18, 2024

opwvhk commented Jan 23, 2024

AVRO-3641: Adds support for nullSafeAnnotations to java SpecificCompiler #2142

AVRO-3641: Adds support for nullSafeAnnotations to java SpecificCompiler #2142

Conversation

peknu commented Mar 13, 2023 • edited Loading

What is the purpose of the change

Verifying this change

Documentation

artheus commented Mar 13, 2023

opwvhk commented Mar 14, 2023

phile314 commented Mar 14, 2023

opwvhk Mar 16, 2023

Choose a reason for hiding this comment

opwvhk Mar 16, 2023

Choose a reason for hiding this comment

opwvhk commented Mar 16, 2023

opwvhk left a comment

Choose a reason for hiding this comment

karlll commented Mar 25, 2023

peknu commented May 10, 2023

opwvhk left a comment

Choose a reason for hiding this comment

opwvhk left a comment

Choose a reason for hiding this comment

peknu commented Aug 16, 2023

opwvhk Aug 16, 2023

Choose a reason for hiding this comment

opwvhk Aug 16, 2023

Choose a reason for hiding this comment

peknu commented Oct 5, 2023

m-kay commented Jan 18, 2024

opwvhk commented Jan 23, 2024

peknu commented Mar 13, 2023 •

edited

Loading