Annotate DSL API with @CheckReturnValue to help detect when calling execute() etc is forgotten

[lukaseder]

Forgetting to execute DML queries is a common mistake in jOOQ:

// This looks reasonable
ctx.insertInto(T).columns(T.A, T.B).values(1, 2);

// But it should have been this
ctx.insertInto(T).columns(T.A, T.B).values(1, 2).execute();

The problem appears mostly when the result of query execution (in this case an update count) is discarded, which is mostly the case for DML statements. There's no easy way for jOOQ to fail compilation when such an execute() call is omitted.

Static code analysis could help here, but isn't ready yet:
https://youtrack.jetbrains.com/issue/IDEA-265263

A way jOOQ could help here would be by leveraging GC related tools like Object.finalize() (which has a lot of disadvantages), or the newer java.lang.ref.Cleaner API or PhantomReference, etc.

[atamanroman]

Saw this on Twitter and had a similar problem lately where missing void do() calls caused bugs. Hope you don‘t mind me chiming in.

We‘ve registered every builder instance and removed it in do(). Gave us at least the chance to log warnings periodically for stale builders or report on JVM shutdown. Maybe only in dev/local/test mode.

Hard to fix properly, though. Would need tooling support like Closable which pretty much fixed unfreed resources?

[lukaseder]

where missing void do() calls caused bugs

What are those do() calls?

Hard to fix properly, though. Would need tooling support like Closable which pretty much fixed unfreed resources?

See the discussion in https://youtrack.jetbrains.com/issue/IDEA-265263

[lukaseder]

Awesome news from https://youtrack.jetbrains.com/issue/IDEA-265263

Just look at this nice trick!

So, all we have to do is add an org.jooq.CheckReturnValue annotation, add that to all of our DSL's intermediate methods, and more than half of these mistakes can be avoided, at least if users use IntelliJ.

[lukaseder]

So cool, and it even works with SOURCE retention (if the sources are loaded), so no new byte code needs to be added!

[lukaseder]

Most API is now annotated with this new annotation. I've searched for all @NotNull annotated DSL API in a set of types. In the future, more and more API is generated from a meta DSL (e.g. a lot of DDL already is), so the missing API will be annotated automatically as we move forward.

[ben-manes]

ErrorProne also includes this annotation, so you can fail the build if it was missed.

[lukaseder]

Yes I've seen that one, that's even more interesting. If someone is interested, I'm sure they can adapt the ErrorProne rule to the new jOOQ annotation.

But for the same reasons I've chosen to use the JetBrains annotations @Nullable and @NotNull, I won't use the JSR-305 annotations this time, either.

Besides, I'm glad that the JetBrains annotations have CLASS retention only, not RUNTIME retention. With an API as big as jOOQ's that would be a lot of overhead for no good reason...

[ben-manes]

ErrorProne checks on the simple name so it should catch the jetbrains one too. That means more users might see a benefit in their builds, not just those who use a particular IDE. So maybe even better than you thought.

[lukaseder]

ErrorProne checks on the simple name so it should catch the jetbrains one too

JetBrains doesn't have one yet, which is why I added the org.jooq.CheckReturnValue annotation for now. But anyway, that's great news!

So maybe even better than you thought.

Absolutely! Thanks for pointing this out :)

[uwolfer]

With this change, you have also added @CheckReturnValue to methods like SelectLimitStep#limit. In my understanding, these methods are mutable.

Because of this change, our SpotBugs setup alerts for code like this:

Optional<Integer> offset = ...;
offset.ifPresent(query::offset);

I see no clean way to write the same code without triggering the SpotBugs warning.

Is this change on such classes also by intention?

Using jooq 3.15.1 and SpotBugs 4.0.4.

[lukaseder]

Thanks for your pointer, @uwolfer. I'm aware of this limitation for the case when a mutable call chain is still consumed in the end. See also the discussion here, that triggered this improvement: https://youtrack.jetbrains.com/issue/IDEA-265263 (look for the StringBuilder example).

Once a better annotation than @CheckReturnValue is available (notably @Contract(pure = true, mutates = "this") or similar), then I'm happy to replace annotations. Until then, I suspect that the few false positives are a reasonable price to pay for the number of true positives this improvement helps find.

Surely, there's a way of ignoring the warning in SpotBugs for some scope?

[lukaseder]

Of course, this would not be an issue (in your case), if we had #2333

[uwolfer]

Thanks for your feedback!

Surely, there's a way of ignoring the warning in SpotBugs for some scope?

Yes, there is, but unfortunately it seems to work only on a class level:

@SuppressFBWarnings(
    value = "RV_RETURN_VALUE_IGNORED",
    justification = "ignoring jOOQ @CheckReturnValue")

Of course, this would not be an issue (in your case), if we had #2333

Yes, in the case of limit / offset. But I also get these alerts for example for adding and conditions in such a way.

It's okay for me for now to add such annotations, but let's hope infrastructure improves in the future.

[lukaseder]

Yes, in the case of limit / offset. But I also get these alerts for example for adding and conditions in such a way.

Then don't! It's almost never necessary to leverage the mutable DSL API this way. Use DSL.noCondition() and embrace a more functional style: https://blog.jooq.org/2020/03/06/create-empty-optional-sql-clauses-with-jooq/

In fact, it's great news that the mutating style triggers such warnings. That way, in the future, we might be able to move towards immutability more easily!