#3695 Behaviour change: findSingleAttribute() to throw when multiple … (#3697)

* #3695 Behaviour change: findSingleAttribute() to throw when multiple rows returned by query

On the plus, this makes the behaviour consistent with all the other findOne() methods (ORM, DTO, SqlQuery) and this very much feels like buggy behaviour [to just get the first result and ignore any subsequent results in the ResultSet]

On the negative of merging this bug fix, any application code relying on the existing behaviour with this fix will break and throw a NonUniqueResultException at RUNTIME (not ideal). However, for these cases where the application now throws an exception, people might not be aware that they were relying on this behaviour and this exception could be useful to highlight that (a potential non-deterministic query result was being used and a potential source of bugs was being).

* Change SqlQuery + Mapper + findOne() also to throw NonUniqueResultException

Author: rbygrave

ebean-core/src/main/java/io/ebeaninternal/server/query/DefaultRelationalQueryEngine.java

@@ -14,6 +14,7 @@
 import io.ebeaninternal.server.core.RowReader;
 import io.ebeaninternal.server.persist.Binder;
 
+import jakarta.persistence.NonUniqueResultException;
 import jakarta.persistence.PersistenceException;
 
 import java.sql.SQLException;
@@ -115,9 +116,14 @@ public <T> T findOne(RelationalQueryRequest request, RowMapper<T> mapper) {
     try {
       request.executeSql(binder, SpiQuery.Type.BEAN);
       T value = request.mapOne(mapper);
+      if (request.next()) {
+        throw new NonUniqueResultException("Got more than 1 result for findOne");
+      }
       request.logSummary();
       return value;
 
+    } catch (NonUniqueResultException e) {
+      throw e;
     } catch (Exception e) {
       throw new PersistenceException(errMsg(e.getMessage(), request.getSql()), e);
 
@@ -160,13 +166,17 @@ public <T> T findSingleAttribute(RelationalQueryRequest request, Class<T> cls) {
       T value = null;
       if (dataReader.next()) {
         value = scalarType.read(dataReader);
+        if (dataReader.next()) {
+          throw new NonUniqueResultException("Got more than 1 result for findSingleAttribute");
+        }
       }
       request.logSummary();
       return value;
 
+    } catch (NonUniqueResultException e) {
+      throw e;
     } catch (Exception e) {
       throw new PersistenceException(errMsg(e.getMessage(), request.getSql()), e);
-
     } finally {
       request.close();
     }

ebean-test/src/test/java/org/tests/query/sqlquery/SqlQueryTests.java

@@ -6,6 +6,7 @@
 import io.ebean.annotation.Platform;
 import io.ebean.meta.MetaTimedMetric;
 import io.ebean.test.LoggedSql;
+import jakarta.persistence.NonUniqueResultException;
 import org.junit.jupiter.api.Test;
 import org.tests.model.basic.Order;
 import org.tests.model.basic.ResetBasicData;
@@ -21,6 +22,7 @@
 import java.util.concurrent.atomic.AtomicLong;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 class SqlQueryTests extends BaseTestCase {
@@ -85,6 +87,19 @@ void selectFunction() {
     assertThat(val2).isEqualTo(11);
   }
 
+  @Test
+  void findSingleAttribute_whenMultipleRows_expect_NonUniqueResultException() {
+    ResetBasicData.reset();
+    String sql = "select id from o_order order by id desc";
+
+    assertThatThrownBy(() -> {
+      DB.sqlQuery(sql)
+        .mapToScalar(Long.class)
+        .findOne();
+    }).isInstanceOf(NonUniqueResultException.class)
+      .hasMessageContaining("Got more than 1 result for findSingleAttribute");
+  }
+
   @Test
   void findSingleAttributeList_decimal() {
     ResetBasicData.reset();
@@ -373,6 +388,18 @@ void findOne_mapper() {
     assertThat(rob.name).isEqualTo("Rob");
   }
 
+  @Test
+  void findOne_mapper_when_notUnique() {
+    ResetBasicData.reset();
+
+    String sql = "select id, name, status from o_customer order by name desc";
+    assertThatThrownBy(() -> DB.sqlQuery(sql)
+      .mapTo(CUST_MAPPER)
+      .findOne())
+      .isInstanceOf(NonUniqueResultException.class)
+      .hasMessageContaining("Got more than 1 result for findOne");
+  }
+
   @Test
   void findList_mapper() {
     ResetBasicData.reset();