Improved Null-checking annotations #3060

rPraml · 2023-05-17T09:18:12Z

Hello @rbygrave, in the following example

String value = query.findSingleAttribute()
if (value == null) ...

IntelliJ complains, that value could never be null, which is not true.

In this PR, I tried to improve the API and added more @NonNullApi / @Nullable annotations.

I moved some null checks, so this PR is only 99% no code change.

Please take a look, if the annotations makes sense this way.

rPraml · 2023-05-17T09:19:17Z

ebean-api/src/main/java/io/ebean/DB.java

@@ -251,7 +250,7 @@ public static Transaction currentTransaction() {
   * </ul>
   */
  public static void flush() {
-    currentTransaction().flush();
+    getDefault().flush();


Delegating to DefaultServer.flush, which may throw a "no currentTransaction" exception.

rPraml · 2023-05-17T09:19:59Z

ebean-api/src/main/java/io/ebean/DB.java

@@ -313,7 +312,7 @@ public static void endTransaction() {
   * Mark the current transaction as rollback only.
   */
  public static void setRollbackOnly() {
-    getDefault().currentTransaction().setRollbackOnly();
+    Objects.requireNonNull(currentTransaction(),"no currentTransaction").setRollbackOnly();


Note: There is no "setRollbackOnly" in DefaultServer. Intention?

rPraml · 2023-05-17T09:20:21Z

ebean-api/src/main/java/io/ebean/Database.java

@@ -35,7 +35,7 @@
 * <h5>The 'default' Database</h5>
 * <p>
 * One Database can be designated as the 'default' or 'primary' Database
- * (see {@link DatabaseConfig#setDefaultServer(boolean)}. Many methods on DB
+ * (see {@link DatabaseConfig#setDefaultServer(boolean)}). Many methods on DB


Fixed missing )

This is good.

rPraml · 2023-05-17T09:20:52Z

ebean-api/src/main/java/io/ebean/Database.java

@@ -166,13 +167,15 @@ public interface Database {
  /**
   * Return the BeanState for a given entity bean.
   * <p>
-   * This will return null if the bean is not an enhanced entity bean.
+   * This will throw an IllegalArgumentException if the bean is not an enhanced entity bean.


checked impl. will never return null

Good thanks.

rPraml · 2023-05-17T09:21:35Z

ebean-api/src/main/java/io/ebean/ExpressionList.java

@@ -404,6 +404,7 @@
   *
   * }</pre>
   */
+  @Nullable
  default <A> A findSingleAttribute() {


This was the root cause, why I created this PR

rPraml · 2023-05-17T09:22:06Z

ebean-core/src/main/java/io/ebeaninternal/api/SpiEbeanServer.java

@@ -14,7 +15,6 @@
 import io.ebeaninternal.server.query.CQuery;
 import io.ebeaninternal.server.transaction.RemoteTransactionEvent;

-import javax.annotation.Nullable;


Note: Wrong annotation was used here.

rPraml · 2023-05-17T09:24:18Z

ebean-core/src/main/java/io/ebeaninternal/server/core/DefaultServer.java

-      throw new IllegalArgumentException("No bean type " + type.getName() + " registered");
-    }
-    return desc.createBean();
+    return descriptor(type).createBean();


Code change: I moved this check to descriptor(class) as it claims to never return null due @NonNullApi

Hmm. Unfortunaltely, this is not completely true.
While DefaultServer is annotated with @NonNullApi, SpiEbeanServer does not. And we have also some code, in our app, where we check if a class is registered. So it might be better to annotate descriptor with @nullable

rPraml · 2023-05-17T09:26:29Z

ebean-core/src/main/java/io/ebeaninternal/server/core/DefaultServer.java

+    T bean = find(type).select(idProp.name()).setId(id).findOne();
+    if (bean == null) {
+      throw new NullPointerException("Could not find reference bean " + id + " for " + desc);
+    }


Code change: I throw an NPE now.

NPE is more like for preconditions or if you really messed up your code, don't you think? How does this exception propagate? I would not expect to received a NPE in user code if an entity isn't found. Isn't there an EntityNotFoundException?

yes, thanks. EntityNotFoundException is the better choice here

Behaviour change means it should be in its own PR. We don't want subtle behaviour changes in a PR that says "Improved Null-checking annotations" and we do want behaviour changes in their own PR that justifies why the change is good.

jnehlmeier · 2023-05-17T20:24:33Z

ebean-api/src/main/java/io/ebean/BeanFinder.java

@@ -63,6 +64,7 @@ public Database db() {
  /**
   * Return the current transaction.


... or null if there is no current transaction in scope.

jnehlmeier · 2023-05-17T20:27:03Z

ebean-api/src/main/java/io/ebean/Database.java

@@ -866,7 +871,7 @@ public interface Database {
   * the bean does not exist then this returns false indicating that nothing was
   * deleted. However, if JDBC batch mode is used then this always returns true.
   */
-  boolean delete(Object bean, Transaction transaction) throws OptimisticLockException;
+  boolean delete(Object bean, @Nullable Transaction transaction) throws OptimisticLockException;


Really? (haven't checked it). But if JavaDoc states ... with an explicit transaction. on an API method it feels weird to me that this explicit transaction can actually be null. Could very well hide some coding problems in user code.

If that is really true, then JavaDoc should describe that you can pass in null as a transaction and what this means if you do so. Same for all the other methods taking an explicit transaction as input parameter. In that case JavaDoc of the equivalent method without transaction parameter could say something like this method behaves exactly as passing a null transaction to delete(bean, transaction) and link to the other method. Would avoid duplicating JavaDoc on all these methods.

Really? (haven't checked it). But if JavaDoc states ... with an explicit transaction.

Yes, the implementation checks it, and the methods without transaction parameter just pass null there.

If that is really true, then JavaDoc should describe that you can pass in null as a transaction and what this means if you do so.

ACK

jnehlmeier · 2023-05-17T20:32:46Z

ebean-core/src/main/java/io/ebeaninternal/server/core/DefaultServer.java

+    T bean = find(type).select(idProp.name()).setId(id).findOne();
+    if (bean == null) {
+      throw new NullPointerException("Could not find reference bean " + id + " for " + desc);
+    }


NPE is more like for preconditions or if you really messed up your code, don't you think? How does this exception propagate? I would not expect to received a NPE in user code if an entity isn't found. Isn't there an EntityNotFoundException?

jnehlmeier · 2023-05-17T20:42:25Z

Generally I feel like Ebean JavaDoc could make more use of @return, @throws and @param so you don't have to read through possibly lengthly JavaDoc text to get the details.

rPraml · 2023-05-19T05:46:36Z

Generally I feel like Ebean JavaDoc could make more use of @return, @throws and @param so you don't have to read through possibly lengthly JavaDoc text to get the details.

I partially agree, What I don't like, if you just repeat the documentation like in this example ¹

/**
 * Returns the current transaction
 * @return the current transaction
 */ 
 Transaction currentTransaction();

But you are right, for longer javadocs, a small summary would not too bad.

(I don't want to (have to) revise the complete documentation in this PR, I just want to fix the NPE warnings in the IDE ;) )

¹) Personally, I read the documentation in 90% in the IDE by jumping to the source. (Or in doubt, reading the source itself, because code never lies, while documentation can be incorrect)

rob-bygrave · 2023-05-23T03:40:56Z

Right so I will not merge this PR because it contains some changes that I don't like / see as being a negative.

Some changes look good but once they are included with other changes that went into behaviour changes and other changes that I believe are net negative.

rob-bygrave · 2023-05-23T03:55:06Z

ebean-api/src/main/java/io/ebean/Database.java

@@ -35,7 +35,7 @@
 * <h5>The 'default' Database</h5>
 * <p>
 * One Database can be designated as the 'default' or 'primary' Database
- * (see {@link DatabaseConfig#setDefaultServer(boolean)}. Many methods on DB
+ * (see {@link DatabaseConfig#setDefaultServer(boolean)}). Many methods on DB


This is good.

rob-bygrave · 2023-05-23T03:58:42Z

ebean-api/src/main/java/io/ebean/DB.java

   * <p>
   * For DB's supporting getGeneratedKeys and sequences such as Oracle10 you do
   * not need to use this method generally. It is made available for more
   * complex cases where it is useful to get an ID prior to some processing.
   */
+  @Nullable


This has the potential effect that code getting the nextId ... really really should expect and handle null. We may not want that because probably this is the 0.01% case.

To me this is not the obviously correct thing to do in that practically what we have today might be better.

rob-bygrave · 2023-05-23T03:59:51Z

ebean-api/src/main/java/io/ebean/DB.java

@@ -232,6 +230,7 @@ public static Transaction beginTransaction(TxScope scope) {
  /**
   * Returns the current transaction or null if there is no current transaction in scope.
   */
+  @Nullable


This to me has real potential to be a net negative. More controversial than it looks.

rob-bygrave · 2023-05-23T04:01:02Z

ebean-api/src/main/java/io/ebean/Database.java

@@ -120,6 +120,7 @@ public interface Database {
  /**
   * Return the associated read only DataSource for this Database instance (can be null).
   */
+  @Nullable


I suspect net negative

rob-bygrave · 2023-05-23T04:01:20Z

ebean-api/src/main/java/io/ebean/Database.java

@@ -166,13 +167,15 @@ public interface Database {
  /**
   * Return the BeanState for a given entity bean.
   * <p>
-   * This will return null if the bean is not an enhanced entity bean.
+   * This will throw an IllegalArgumentException if the bean is not an enhanced entity bean.


Good thanks.

rob-bygrave · 2023-05-23T04:07:44Z

ebean-core/src/main/java/io/ebeaninternal/server/core/DefaultServer.java

+    T bean = find(type).select(idProp.name()).setId(id).findOne();
+    if (bean == null) {
+      throw new NullPointerException("Could not find reference bean " + id + " for " + desc);
+    }


Behaviour change means it should be in its own PR. We don't want subtle behaviour changes in a PR that says "Improved Null-checking annotations" and we do want behaviour changes in their own PR that justifies why the change is good.

…ute()

From #3060 - Add missing @nullable to ExpressionList.findSingleAttribute()

rPraml · 2023-05-23T06:28:12Z

Hello @rob-bygrave, @jnehlmeier,

thank you for your constructive feedback.

Rob already merged some changes with #3066

I'll try to summarize, what's left in this PR

I will revert all code changes here and create extra PRs
nextId: Yes - it makes no sense here for @nullable
currentTransaction: I'm unsure myself here.
I understand Rob's argument that this could be a negative overall.
However, at least we have pieces of code where we make sure that this is not called with an active transaction.
For example, this is mostly code, that interacts with external APIs, which can't be roll backed.

void deleteUser(String userId) {
   assert DB.currentTransaction() == null : "must not run in transaction";
   if (externalApi.deleteUser(userId)) {
       user = DB.find(User.class, userId);
       user.setDeleted(true);
       DB.save(user);
   }
}

It would be bad, if this code runs in a transaction, that is rolled back afterwards.
I can live with a potential IDE warning 'currentTransaction() is never null here'

what to do with methods like boolean delete(Object bean, @Nullable Transaction transaction)?
Shoud I use Objects.requireNonNull(currentTransaction(),"no currentTransaction") or rely on java 17 detailed NPE messages?

I will not continue with the pull request for the time being, but will make it available for discussion first, since it only solves cosmetic problems in the IDE for us.

Roland

…gleAttribute()

rbygrave · 2023-05-23T08:31:52Z

assert DB.currentTransaction() == null : "must not run in transaction";

Ok, I see. To me I'd say this use case is like @Transaction(type=NEVER) but in programatic form.

My gut says that the alternative is for currentTransaction() to always return a non nullable transaction and that a new boolean hasCurrentTransaction() be added. This is probably based on the thought that the most common use of currentTransaction() [that I was expecting] is it being used inside methods annotated with @Transactional and for these cases the code is always expects non-nullable transaction.

Hmm, pondering.

boolean delete(Object bean, @nullable Transaction transaction)

For these cases I don't see any downside apart from verbosity. I don't think it negatively impacts existing code that is using those methods.

rbygrave · 2023-06-16T23:57:54Z

I think we close this one and have a new fresh PR if desired.

…gleAttribute()

rPraml added 2 commits May 17, 2023 11:10

Improved Null-checking annotations

616da40

FIX: import

12e278e

rPraml commented May 17, 2023

View reviewed changes

jnehlmeier reviewed May 17, 2023

View reviewed changes

rob-bygrave reviewed May 23, 2023

View reviewed changes

rob-bygrave added a commit that referenced this pull request May 23, 2023

From #3060 - Add missing @nullable to ExpressionList.findSingleAttrib…

b210d1a

…ute()

rob-bygrave added a commit that referenced this pull request May 23, 2023

Merge pull request #3066 from ebean-orm/feature/3060-part2

1b8df12

From #3060 - Add missing @nullable to ExpressionList.findSingleAttribute()

rPraml added 2 commits May 23, 2023 07:44

Merge branch 'master-upstream' into improved-null-checks

764a8b2

Revert some changes and fixed some more annotations

4c4b998

rPraml mentioned this pull request May 23, 2023

DB.refrence throws EntityNotFoundException instead returning null #3068

Merged

rPraml pushed a commit to FOCONIS/ebean that referenced this pull request May 23, 2023

From ebean-orm#3060 - Add missing @nullable to ExpressionList.findSin…

dbdfa9c

…gleAttribute()

rbygrave closed this Jun 16, 2023

rPraml pushed a commit to FOCONIS/ebean that referenced this pull request Aug 3, 2023

From ebean-orm#3060 - Add missing @nullable to ExpressionList.findSin…

2cf470f

…gleAttribute()

rPraml pushed a commit to FOCONIS/ebean that referenced this pull request Aug 3, 2023

From ebean-orm#3060 - Add missing @nullable to ExpressionList.findSin…

9b58e15

…gleAttribute()

rPraml pushed a commit to FOCONIS/ebean that referenced this pull request Aug 3, 2023

From ebean-orm#3060 - Add missing @nullable to ExpressionList.findSin…

08b64d3

…gleAttribute()

rPraml pushed a commit to FOCONIS/ebean that referenced this pull request Aug 8, 2023

From ebean-orm#3060 - Add missing @nullable to ExpressionList.findSin…

ea71d5a

…gleAttribute()

rPraml deleted the improved-null-checks branch August 10, 2023 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved Null-checking annotations #3060

Improved Null-checking annotations #3060

rPraml commented May 17, 2023

rPraml May 17, 2023

rPraml May 17, 2023

rPraml May 17, 2023

rob-bygrave May 23, 2023

rPraml May 17, 2023

rob-bygrave May 23, 2023

rPraml May 17, 2023

rPraml May 17, 2023

rPraml May 17, 2023

rPraml May 19, 2023

rPraml May 17, 2023

jnehlmeier May 17, 2023

rPraml May 19, 2023

rob-bygrave May 23, 2023

jnehlmeier May 17, 2023

jnehlmeier May 17, 2023

rPraml May 19, 2023

jnehlmeier May 17, 2023

jnehlmeier commented May 17, 2023

rPraml commented May 19, 2023

rob-bygrave commented May 23, 2023

rob-bygrave May 23, 2023

rob-bygrave May 23, 2023

rob-bygrave May 23, 2023

rob-bygrave May 23, 2023

rob-bygrave May 23, 2023

rob-bygrave May 23, 2023

rPraml commented May 23, 2023 •

edited

Loading

rbygrave commented May 23, 2023

rbygrave commented Jun 16, 2023

		@@ -63,6 +64,7 @@ public Database db() {
		/**
		* Return the current transaction.

Improved Null-checking annotations #3060

Improved Null-checking annotations #3060

Conversation

rPraml commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnehlmeier commented May 17, 2023

rPraml commented May 19, 2023

rob-bygrave commented May 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rPraml commented May 23, 2023 • edited Loading

rbygrave commented May 23, 2023

rbygrave commented Jun 16, 2023

rPraml commented May 23, 2023 •

edited

Loading