jOOQ / jOOQ

jOOQ is the best way to write SQL in Java
https://www.jooq.org
Other
6.17k stars 1.21k forks source link

Add nullability annotations to jOOQ API for better Kotlin interop #6244

Closed lukaseder closed 4 years ago

lukaseder commented 7 years ago

While generating JSR-305 annotations is not technically correct (https://github.com/jOOQ/jOOQ/issues/4748) as a non-null column may be null "in transit" (e.g. when the constraint is deferred, or for a while in Java code, prior to storing the record, or when fetching a record from an outer join, which removes not null constraints) it might certainly make sense to annotate most jOOQ API with @javax.annotation.Nonnull and @javax.annotation.Nullable. This would greatly improve the interoperability experience, especially for Kotlin / Ceylon users.

Alternatives are possible, e.g. using the JetBrains annotations.

When annotating the API, the following things can be done automatically:

Some API will be left out of this improvement. This includes:

Incompatible change

For Kotlin users, this will be an incompatible change as they will get occasional compilation errors, e.g. when using fetchOne()

lukaseder commented 7 years ago

See licensing concerns here: https://github.com/jOOQ/jOOQ/issues/4748#issuecomment-303380405

lukaseder commented 7 years ago

Spring has moved forward with JSR 305 adoption through the TypeQualifierNickname backdoor, which allows for defining custom @Nullable annotations that "correspond" to the JSR 305 annotations of equivalent semantics, without exposing them to clients. Background info here:

Let's wait how Spring fares with this strategy. I feel that we're really on a very wrong path as an ecosystem. If we cannot agree on something silly and stupid as a @Nullable annotation as part of the JRE, it's better to wait and see what happens. Let's not jump on this part of the Kotlin hype.

GuiSim commented 6 years ago

Kotlin added support for @TypeQualifierNickname in version 1.1.50.

While I agree that this isn't the perfect solution, I think it would be great to have jOOQ annotate nullable and non-nullable types, even if it's with its own custom annotation.

lukaseder commented 6 years ago

I rest my case (for now):

I feel that we're really on a very wrong path as an ecosystem. If we cannot agree on something silly and stupid as a @Nullable annotation as part of the JRE, it's better to wait and see what happens. Let's not jump on this part of the Kotlin hype.

fkowal commented 5 years ago
  1. Thank you for the great library

A lot of time has passed since this topic was first discussed. Perhaps it's worth reconsidering?

Support @Nullable annotations would be great. It could be an optin feature, and the user would decide what package he would use if any.

generate {
   nullable: "javax.annotations.Nullable"
}
lukaseder commented 5 years ago

Thanks for your comments, @fkowal. The usage of JSR 305 is crystal clear. The reservations mentioned in this issue and elsewhere aren't purely technical. JSR 305 still creates a legal mess. Is it LGPL licensed like FindBugs? Or BSD licensed as claimed on some websites? Or ASL 2.0 licensed as mentioned in the maven artifact com.google.code.findbugs:jsr305:3.0.2? What does the groupId com.google.code.findbugs even mean, who owns it? Were they allowed to re-license the library, or did they just not care and do it anyway? Will Oracle allow the continued use of the javax namespace (they didn't in the case of Jakarta!)

Nothing seems to have changed since this issue was first created. Everyone is just hacking around, and I'm really reluctant to spend a lot of time annotating the entire jOOQ API with something that is so unclean - without any clear and obvious benefits, too! The Kotlin integration could profit from this, yes, but only to a limited extent, as Kotlin has made good choices to integrate with Java APIs by introducing T! types, i.e. types that might be nullable, but are not type checked as strongly as T? types, which are known to be nullable.

So, this issue remains dormant, until it has been shown that either:

xenomachina commented 4 years ago

The lack of nullalbe annotations on things that can be null also leaves a lot to be desired when you're used to Kotlin warning you when you forgot to properly handle the null values. Every time I see fetchOne or fetchSingle in code I have to go look up whether it's the one that always returns a value, or the one that sometimes returns null. If they were properly annotated, I'd know from the type.

If the JSR annotations have legal problems, why not use one of the alternatives? I agree it isn't ideal having to depend on a non "standard" library like org.jetbrains:annotations, but if a better alternative ever arises in the future it should be pretty easy to search and replace.

lukaseder commented 4 years ago

Thanks for your suggestion, @xenomachina.

Annotating each and every method is definitely not practical. JSR-305 has this @TypeQualifierDefault annotation, which can be placed on a package in order to specify that the entire API is "non-null" by default (which jOOQ's DSL is). I don't think that the JetBrains annotations have a similar mechanism?

lukaseder commented 4 years ago

Currently looking into this again as jOOQ 3.14 will offer many improvements for Kotlin users. Additional reservations in the area of JSR-305 are listed here: https://github.com/google/guava/issues/2960

Including an important JPMS problem: https://blog.codefx.org/java/jsr-305-java-9

lukaseder commented 4 years ago

I will play around with the JetBrains annotations: https://www.jetbrains.com/help/idea/nullable-and-notnull-annotations.html

lukaseder commented 4 years ago

Jetbrains doesn't have their own @ParametersAreNonnullByDefault but refers to the JSR-305 one: https://www.jetbrains.com/help/idea/parametersarenonnullbydefault-annotation.html

Without this annotation, we'd be putting a gazillion annotations all over the place, which is what I definitely want to avoid.

This isn't going anywhere, it seems...

lukaseder commented 4 years ago

I guess we can add some value still by annotating methods only, not parameters or types

lukaseder commented 4 years ago

The JetBrains annotations have CLASS retention, which makes writing tests much more difficult...

lukaseder commented 4 years ago

OK, I found a solution using ByteBuddy to write tests. The approach seems promising. I think we're going ahead with annotating some of the jOOQ API with @Nullable and @NotNull for jOOQ 3.14

jorsol commented 4 years ago

Jetbrains doesn't have their own @ParametersAreNonnullByDefault but refers to the JSR-305 one: https://www.jetbrains.com/help/idea/parametersarenonnullbydefault-annotation.html

Without this annotation, we'd be putting a gazillion annotations all over the place, which is what I definitely want to avoid.

This isn't going anywhere, it seems...

Eclipse annotations has @NonNullByDefault https://javadoc.io/doc/org.eclipse.jdt/org.eclipse.jdt.annotation but they also use CLASS retention.

lukaseder commented 4 years ago

The CLASS retention isn't really an issue, as it turned out, but the Eclipse annotations are licensed EPL 2.0, which is a (weak) copyleft license, to my understanding? That makes it a bit of a showstopper...

lukaseder commented 4 years ago

Marking this as an "incompatible change", because it will produce some compilation errors in client Kotlin code.

lukaseder commented 4 years ago

The tests covering the API have detected a variety of API that is lacking @Support annotations. This will be fixed without a separate issue or backport.

lukaseder commented 4 years ago

Most of the API is annotated now. There might have been 1-2 oversights which can be amended in this issue, or in new defects.

lukaseder commented 4 years ago

This feature isn't done yet. If we want to go all in with type annotations, we need to annotate also generic type variables, such as:

    @NotNull
    <@Nullable E> List<E> fetchInto(Class<? extends E> type) throws DataAccessException, MappingException;

... nope, this was already discussed. They're generic type variables, and as such, will have their type inferred to E or E? in Kotlin

lukaseder commented 4 years ago

I'm reopening this, because for the past weeks, this has been getting heavily on my nerves: https://stackoverflow.com/q/63036897/521799

This is a show stopper for the JetBrains annotations, if there's no way to turn off this feature in content-assist. Another library would still be possible, though.

lukaseder commented 4 years ago

This is due to a number of things:

  1. The annotation has TYPE_USE as target (inevitable)
  2. The project uses Java 8+ compliance (inevitable)
  3. The project has Enable annotation-based null analysis turned off (this used to be buggy, but maybe works now)
  4. The project doesn't have these annotations configured as NotNull annotations

See: https://stackoverflow.com/a/63051248/521799

So, the workaround could be to turn on this feature, or upvote my suggested UX improvement by allowing to turn off auto-completing the generation of type annotations: https://bugs.eclipse.org/bugs/show_bug.cgi?id=565463

lukaseder commented 4 years ago

OK, I guess this is Eclipse's stance on the problem: Works as designed. I'll think about this again, and review if we should thus replace JetBrains annotations with something else, again.

lukaseder commented 4 years ago

Making the Maven dependency optional/provided seems to resolve this UX issue in Eclipse: https://stackoverflow.com/a/63131683/521799

lukaseder commented 4 years ago

The dependency can be pulled in transitively, nonetheless: https://github.com/testcontainers/testcontainers-java/issues/3057. A workaround would then be to do:

<dependency>
    <groupId>org.testcontainers</groupId>
    <artifactId>testcontainers</artifactId>
    <version>${org.testcontainers.version}</version>

    <exclusions>
        <exclusion>
            <groupId>org.jetbrains</groupId>
            <artifactId>annotations</artifactId>
        </exclusion>
    </exclusions>
</dependency>
kevinb9n commented 2 years ago

Soo a group of us have been slowly working on an attempt to "fix" nullness annotations for JVM languages. Kotlin in particular is on-board.

I've been working on better public-facing explanatory documentation for our site http://jspecify.org, but in the meantime thought I would at least mention here at this is going on and we'd be more than open to talking with your project about how to make sure our artifact would meet your needs.

If interested in that conversation, how/where/with-whom would you like it to happen?

(I'll keep working on making the site more self-explanatory.)

lukaseder commented 2 years ago

We have seen the xkcd comic. Please do not send us the xkcd comic. We know about the xkcd comic.

😅

That's great news, thanks for chiming in @kevinb9n.

So far, I'm quite happy with the results of having integrated the JetBrains annotations, which were a pragmatic choice here. But an improved (and finalised) JSR 305 could be even better, which is somewhat similar to what you're working on? The best solution would be a JEP, but I guess that's a ton of spec work, making success less probable.

If interested in that conversation, how/where/with-whom would you like it to happen?

Very interested!

How

I think that by writing in public is always good, to benefit future discoverability of the discussion (compared to phone calls, etc).

Where

This issue tracker is fine, though a new issue because this one here is already quite specific to 1) kotlin, 2) previous work. Your suggestions would be quite long term, and not necessarily kotlin specific. E.g. Scala 3 (to my pleasant surprise) went the Ceylon way and used union types to model nulls, and also supports a variety of popular annotations: https://dotty.epfl.ch/docs/reference/other-new-features/explicit-nulls.html

Since jOOQ is also quite powerful and somewhat popular in Scala, I think it would be good to move your discussion to a new issue.

Alternatively, I'll be happy to join discussions on your issue tracker.

With-whom

That would be me

kevinb9n commented 2 years ago

Thanks for the quick and detailed response!

Since jOOQ is also quite powerful and somewhat popular in Scala, I think it would be good to move your discussion to a new issue.

done #12949