Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,24 @@ make generate-jooq
./gradlew :generated:jooqCodegen
```

The generator writes JSpecify nullability annotations into the generated sources, so NullAway reads the real nullness of every generated column accessor instead of guessing.
Routine return values stay unannotated, so `Routines.shobjDescription` still counts as non-null even though PostgreSQL returns `NULL` for an object without a comment.
Three consequences are worth knowing:

- jOOQ does not officially support the `TYPE_USE` positioning that JSpecify requires yet ([jOOQ/jOOQ#10759](https://github.com/jOOQ/jOOQ/issues/10759)).
The positioning is correct for every scalar column, because the generated code uses no generics, collections, maps or forced types with inner
classes.
Keep the `includes` list free of such objects, or review the annotation positions after a regeneration.
- The positioning is wrong for the six array columns.
Java applies a `TYPE_USE` annotation written before an array type to the element type, and NullAway drops array-dimension annotations in `JSpecifyMode`.
So `@Nullable String[] getNspacl()` reads as a non-null array of nullable strings, while the column is a nullable array of non-null strings.
The six accessors are `getNspacl`, `getDatacl`, `getRelacl`, `getReloptions`, `getSetconfig` and `getDefaclacl`.
No operator code calls them today, because the operator uses the table field constants and `Routines.aclexplode` instead.
A declaration annotation for `nullableAnnotationType` would fix the positioning, because such an annotation always applies to the method. That is a design change.
- Every generated class carries `@SuppressWarnings({"all", ...})`, which Error Prone honours.
That suppression, and nothing else, is why the generated sources are exempt from the checks in `errorprone.args`, including `RequireExplicitNullMarking`.
Do not add `NullAway:TreatGeneratedAsUnannotated`: it would make NullAway discard the nullability annotations that this generator now writes.

### Docker Environment

See [Docker Environment](docs/docker-environment.md) for setting up a local development environment using Quarkus Dev Services.
Expand Down
5 changes: 5 additions & 0 deletions errorprone.args
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@

# Nullness
-Xep:NullAway:ERROR
-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true
-XepOpt:NullAway:AnnotatedPackages=it.aboutbits.postgresql
-XepOpt:NullAway:CheckOptionalEmptiness=true
# ExhaustiveOverride is safe only while MissingOverride runs at ERROR, because it assumes that every overriding method carries the Override annotation.
-XepOpt:NullAway:ExhaustiveOverride=true
-XepOpt:NullAway:HandleTestAssertionLibraries=true
-XepOpt:NullAway:JSpecifyMode=true
-Xep:AnnotationPosition:ERROR
-Xep:EqualsMissingNullable:ERROR
Expand Down
17 changes: 17 additions & 0 deletions generated/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ dependencies {
*/
api(libs.jooq)
compileOnly(libs.jooqMeta)
compileOnlyApi(libs.jspecify)
// PostgreSQL JDBC Driver for jOOQ generation
jooqCodegen(libs.postgresql)
}
Expand Down Expand Up @@ -50,6 +51,22 @@ jooq {
fluentSetters = true
generatedAnnotation = true
pojos = false
nonnullAnnotation = true

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The annotations only land on record getters/setters and table methods. Field constants stay unannotated (TableField<PgAuthidRecord, String> ROLPASSWORD), and the operator only uses those plus fetchOneInto/fetchSingle, never the record getters.

So NullAway gains almost nothing today, while we take on the caveats documented in the readme. Maybe I'm wrong, so feel free to correct me on this.

Is it worth keeping this part, or should the PR stick to the NullAway options?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The annotations only land on record getters/setters and table methods. Field constants stay unannotated (TableField<PgAuthidRecord, String> ROLPASSWORD), and the operator only uses those plus fetchOneInto/fetchSingle, never the record getters.

So NullAway gains almost nothing today, while we take on the caveats documented in the readme.

You are right about today: the field constants stay unannotated, and no operator code calls a record getter. I would still like to keep it, for three reasons.

1. Without the annotations, NullAway trusts every getter as non-null.
The generated package it.aboutbits.postgresql.core.infrastructure.persistence is inside -XepOpt:NullAway:AnnotatedPackages=it.aboutbits.postgresql. So NullAway treats the jOOQ code as annotated code, and an unannotated return type there means non-null. For a nullable column, that is wrong, and NullAway stays silent.

I checked this with a small probe class that dereferences PgAuthidRecord.getRolpassword():

return authid.getRolpassword().length();
  • On this branch: error: [NullAway] dereferenced expression authid.getRolpassword() is @Nullable
  • On the base branch, without the annotations: BUILD SUCCESSFUL

So the annotations do not only add information, they remove a false guarantee. The first time someone uses a record getter, NullAway checks it correctly. I first thought AcknowledgeRestrictiveAnnotations was the reason, but that option only applies to unannotated third-party code. The generated package is inside AnnotatedPackages, so NullAway reads it as annotated code.

2. It is a real-world example for the open jOOQ issue.
jOOQ does not support TYPE_USE positioning yet (jOOQ/jOOQ#10759, still open). The six array accessors in this module show the problem: @Nullable String[] getNspacl() reads as a non-null array of nullable strings. I would like to show this to Lukas Eder as a concrete case, possibly as the base for a reproducer.

3. It is a good case for feedback to NullAway, too.
The usual way to handle generated code, NullAway:TreatGeneratedAsUnannotated, would discard exactly these annotations. RestrictiveAnnotationHandler ignores annotations in generated code "no matter what", even with AcknowledgeRestrictiveAnnotations=true, which JSpecify mode turns on anyway. NullAway's own source names the gap in CodeAnnotationInfo: "In the future, we might want finer grain controls to distinguish code that is generated with nullability info and without." This module is exactly that case: generated code that carries nullability info.

The caveats are documented in the README and in generated/build.gradle.kts. The only wrong positions are the six array accessors, and no operator code calls them. If one of them is needed later, we can switch nullableAnnotationType to a declaration annotation or wait for the jOOQ fix.

nullableAnnotation = true
// We use JSpecify annotations already even though jOOQ does not officially support JSpecify's TYPE_USE positioning yet.
// See https://github.com/jOOQ/jOOQ/issues/10759
// The positioning is correct for every scalar column, as our generated code is not using any generics,
// collections, maps or forced types with inner classes.
// The positioning is wrong for the six array columns. Java applies a TYPE_USE annotation written before
// an array type to the element type, and NullAway drops array-dimension annotations in JSpecifyMode.
// So "@Nullable String[] getNspacl()" reads as a non-null array of nullable strings, while the column is
// a nullable array of non-null strings.
// The six accessors are getNspacl, getDatacl, getRelacl, getReloptions, getSetconfig and getDefaclacl.
// No operator code calls them today, as the operator uses the table field constants and Routines.aclexplode.
// A declaration annotation for nullableAnnotationType would fix the positioning, as such an annotation
// always applies to the method. That is a design change, not a comment fix.
nonnullAnnotationType = "org.jspecify.annotations.NonNull"
nullableAnnotationType = "org.jspecify.annotations.Nullable"
}
target {
packageName = "it.aboutbits.postgresql.core.infrastructure.persistence"
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading