SONAR-2225 JpaDatabaseSession: improve exception message in case of NonUniqueResultEx...

diff --git a/sonar-core/src/main/java/org/sonar/jpa/session/JpaDatabaseSession.java b/sonar-core/src/main/java/org/sonar/jpa/session/JpaDatabaseSession.java
index b407bacfe2a4bd94554de5372fb959413cace969..6c87056f278598db581e4931fcfb2e59359001d4 100644 (file)
--- a/sonar-core/src/main/java/org/sonar/jpa/session/JpaDatabaseSession.java
+++ b/sonar-core/src/main/java/org/sonar/jpa/session/JpaDatabaseSession.java
@@ -23,14 +23,12 @@ import org.apache.commons.lang.StringUtils;
 import org.slf4j.LoggerFactory;
 import org.sonar.api.database.DatabaseSession;
 
+import java.util.*;
+
 import javax.persistence.EntityManager;
-import javax.persistence.NoResultException;
 import javax.persistence.NonUniqueResultException;
+import javax.persistence.PersistenceException;
 import javax.persistence.Query;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
 
 public class JpaDatabaseSession extends DatabaseSession {
 
@@ -43,6 +41,10 @@ public class JpaDatabaseSession extends DatabaseSession {
     this.connector = connector;
   }
 
+  /**
+   * Note that usage of this method is discouraged, because it allows to construct and execute queries without additional exception handling,
+   * which done in methods of this class.
+   */
   public EntityManager getEntityManager() {
     return entityManager;
   }
@@ -143,17 +145,43 @@ public class JpaDatabaseSession extends DatabaseSession {
     }
   }
 
-
+  /**
+   * Note that not recommended to directly execute {@link Query#getSingleResult()}, because it will bypass exception handling,
+   * which done in {@link #getSingleResult(Query, Object)}.
+   */
   public Query createQuery(String hql) {
     startTransaction();
     return entityManager.createQuery(hql);
   }
 
+  /**
+   * @return the result or <code>defaultValue</code>, if not found
+   * @throws NonUniqueResultException if more than one result
+   */
   public <T> T getSingleResult(Query query, T defaultValue) {
-    try {
-      return (T) query.getSingleResult();
-    } catch (NoResultException ex) {
+    /*
+     * See http://jira.codehaus.org/browse/SONAR-2225
+     * By default Hibernate throws NonUniqueResultException without meaningful information about context,
+     * so we improve it here by adding all results in error message.
+     * Note that in some rare situations we can receive too many results, which may lead to OOME,
+     * but actually it will mean that database is corrupted as we don't expect more than one result
+     * and in fact org.hibernate.ejb.QueryImpl#getSingleResult() anyway does loading of several results under the hood.
+     */
+    List<T> result = query.getResultList();
+
+    if (result.size() == 1) {
+      return result.get(0);
+
+    } else if (result.isEmpty()) {
       return defaultValue;
+
+    } else {
+      Set<T> uniqueResult = new HashSet<T>(result);
+      if (uniqueResult.size() > 1) {
+        throw new NonUniqueResultException("Expected single result, but got : " + result.toString());
+      } else {
+        return uniqueResult.iterator().next();
+      }
     }
   }
 
@@ -162,11 +190,19 @@ public class JpaDatabaseSession extends DatabaseSession {
     return getEntityManager().find(entityClass, id);
   }
 
+  /**
+   * @return the result or <code>null</code>, if not found
+   * @throws NonUniqueResultException if more than one result
+   */
   public <T> T getSingleResult(Class<T> entityClass, Object... criterias) {
     try {
       return getSingleResult(getQueryForCriterias(entityClass, true, criterias), (T) null);
 
     } catch (NonUniqueResultException ex) {
+      /*
+       * TODO Log and throw is anti-pattern ( see http://today.java.net/article/2006/04/04/exception-handling-antipatterns#logAndThrow ),
+       * but NonUniqueResultException doesn't have a constructor with cause
+       */
       LoggerFactory.getLogger(JpaDatabaseSession.class).warn("NonUniqueResultException on entity {} with criterias : {}",
           entityClass.getSimpleName(), StringUtils.join(criterias, ","));
       throw ex;

diff --git a/sonar-core/src/test/java/org/sonar/jpa/session/JpaDatabaseSessionTest.java b/sonar-core/src/test/java/org/sonar/jpa/session/JpaDatabaseSessionTest.java
new file mode 100644 (file)
index 0000000..d1314ea
--- /dev/null
+++ b/sonar-core/src/test/java/org/sonar/jpa/session/JpaDatabaseSessionTest.java
@@ -0,0 +1,68 @@
+/*
+ * Sonar, open source software quality management tool.
+ * Copyright (C) 2008-2011 SonarSource
+ * mailto:contact AT sonarsource DOT com
+ *
+ * Sonar is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * Sonar is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with Sonar; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02
+ */
+package org.sonar.jpa.session;
+
+import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.Arrays;
+import java.util.Collections;
+
+import javax.persistence.NonUniqueResultException;
+import javax.persistence.Query;
+
+public class JpaDatabaseSessionTest {
+
+  private JpaDatabaseSession session;
+
+  @Before
+  public void setUp() {
+    session = new JpaDatabaseSession(null);
+  }
+
+  @Test(expected = NonUniqueResultException.class)
+  public void shouldThrowNonUniqueResultException() {
+    Query query = mock(Query.class);
+    when(query.getResultList()).thenReturn(Arrays.asList("foo", "bar"));
+    session.getSingleResult(query, null);
+  }
+
+  @Test
+  public void shouldReturnSingleResult() {
+    Query query = mock(Query.class);
+
+    when(query.getResultList()).thenReturn(Arrays.asList("foo", "foo"), Arrays.asList("bar"));
+    assertThat(session.getSingleResult(query, "default"), is("foo"));
+    assertThat(session.getSingleResult(query, "default"), is("bar"));
+  }
+
+  @Test
+  public void shouldReturnDefaultValue() {
+    Query query = mock(Query.class);
+    when(query.getResultList()).thenReturn(Collections.emptyList());
+    assertThat(session.getSingleResult(query, "default"), is("default"));
+  }
+
+}