AMBARI-15363 - Postgres And c3p0 Queries Can Hang Ambari On Large Queries (jonathanhurley)

ambari-project/pom.xml

@@ -210,7 +210,7 @@
       <dependency>
         <groupId>org.eclipse.persistence</groupId>
         <artifactId>eclipselink</artifactId>
-        <version>2.5.2</version>
+        <version>2.6.2</version>
       </dependency>
       <dependency>
         <groupId>org.postgresql</groupId>

ambari-server/pom.xml

@@ -327,7 +327,7 @@
           <dependency>
             <groupId>org.eclipse.persistence</groupId>
             <artifactId>eclipselink</artifactId>
-            <version>2.4.2</version>
+            <version>2.6.2</version>
           </dependency>
         </dependencies>
       </plugin>
@@ -399,7 +399,7 @@
                 </source>
               </sources>
             </mapping>
-			<mapping>
+      			<mapping>
               <directory>/usr</directory>
               <username>root</username>
               <groupname>root</groupname>
@@ -1194,6 +1194,12 @@
       <groupId>org.quartz-scheduler</groupId>
       <artifactId>quartz</artifactId>
       <version>2.2.1</version>
+      <exclusions>
+        <exclusion>
+          <groupId>c3p0</groupId>
+          <artifactId>c3p0</artifactId>
+        </exclusion>
+      </exclusions>      
     </dependency>
     <dependency>
       <groupId>org.quartz-scheduler</groupId>
@@ -1267,6 +1273,12 @@
       <artifactId>commons-cli</artifactId>
       <version>1.3.1</version>
     </dependency>
+    <dependency>
+      <groupId>com.mchange</groupId>
+      <artifactId>c3p0</artifactId>
+      <version>[0.9.5.2]</version>
+      <scope>compile</scope>
+    </dependency>
   </dependencies>
 
   <pluginRepositories>

ambari-server/src/main/java/org/apache/ambari/server/api/query/JpaPredicateVisitor.java

@@ -55,12 +55,17 @@ public abstract class JpaPredicateVisitor<T> implements PredicateVisitor {
   /**
    * The root that the {@code from} clause requests from.
    */
-  private Root<T> m_root;
+  final private Root<T> m_root;
 
   /**
    * The query to submit to JPA.
    */
-  private CriteriaQuery<T> m_query;
+  final private CriteriaQuery<T> m_query;
+
+  /**
+   * The entity class that the root of the query is built from.
+   */
+  final private Class<T> m_entityClass;
 
   /**
    * The last calculated predicate.
@@ -87,6 +92,7 @@ public abstract class JpaPredicateVisitor<T> implements PredicateVisitor {
   public JpaPredicateVisitor(EntityManager entityManager, Class<T> entityClass) {
     m_entityManager = entityManager;
     m_builder = m_entityManager.getCriteriaBuilder();
+    m_entityClass = entityClass;
     m_query = m_builder.createQuery(entityClass);
     m_root = m_query.from(entityClass);
   }

ambari-server/src/main/java/org/apache/ambari/server/api/query/JpaSortBuilder.java

@@ -19,16 +19,20 @@ package org.apache.ambari.server.api.query;
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Iterator;
 import java.util.List;
+import java.util.Set;
 
 import javax.persistence.criteria.CriteriaBuilder;
 import javax.persistence.criteria.CriteriaQuery;
 import javax.persistence.criteria.Order;
 import javax.persistence.criteria.Path;
+import javax.persistence.criteria.Root;
 import javax.persistence.metamodel.SingularAttribute;
 
 import org.apache.ambari.server.controller.spi.SortRequest;
 import org.apache.ambari.server.controller.spi.SortRequestProperty;
+import org.apache.commons.lang.ObjectUtils;
 
 /**
  * The {@link JpaSortBuilder} class is used to convert and Ambari
@@ -84,9 +88,32 @@ public class JpaSortBuilder<T> {
       Path<?> path = null;
       for (SingularAttribute<?, ?> singularAttribute : singularAttributes) {
         if (null == path) {
+
           CriteriaQuery<T> query = visitor.getCriteriaQuery();
-          path = query.from(visitor.getEntityClass()).get(
-              singularAttribute.getName());
+          Set<Root<?>> roots = query.getRoots();
+
+          // if there are existing roots; use the existing roots to prevent more
+          // roots from being added potentially causing a cartesian product
+          // where we don't want one
+          if (null != roots && !roots.isEmpty()) {
+            Iterator<Root<?>> iterator = roots.iterator();
+            while (iterator.hasNext()) {
+              Root<?> root = iterator.next();
+
+              Class<?> visitorEntityClass = visitor.getEntityClass();
+              if (ObjectUtils.equals(visitorEntityClass, root.getJavaType())
+                  || ObjectUtils.equals(visitorEntityClass, root.getModel().getJavaType())) {
+                path = root.get(singularAttribute.getName());
+                break;
+              }
+            }
+          }
+
+          // no roots exist already which match this entity class, create a new
+          // path
+          if (null == path) {
+            path = query.from(visitor.getEntityClass()).get(singularAttribute.getName());
+          }
         } else {
           path = path.get(singularAttribute.getName());
         }

ambari-server/src/test/java/org/apache/ambari/server/api/query/JpaSortBuilderTest.java

@@ -0,0 +1,153 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ambari.server.api.query;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+import javax.persistence.EntityManager;
+import javax.persistence.criteria.CriteriaQuery;
+import javax.persistence.criteria.Order;
+import javax.persistence.criteria.Root;
+import javax.persistence.metamodel.SingularAttribute;
+
+import org.apache.ambari.server.controller.internal.AlertHistoryResourceProvider;
+import org.apache.ambari.server.controller.internal.SortRequestImpl;
+import org.apache.ambari.server.controller.spi.Predicate;
+import org.apache.ambari.server.controller.spi.SortRequest;
+import org.apache.ambari.server.controller.spi.SortRequestProperty;
+import org.apache.ambari.server.controller.utilities.PredicateBuilder;
+import org.apache.ambari.server.controller.utilities.PredicateHelper;
+import org.apache.ambari.server.orm.GuiceJpaInitializer;
+import org.apache.ambari.server.orm.InMemoryDefaultTestModule;
+import org.apache.ambari.server.orm.entities.AlertHistoryEntity;
+import org.apache.ambari.server.orm.entities.AlertHistoryEntity_;
+import org.junit.Before;
+import org.junit.Test;
+
+import com.google.inject.Guice;
+import com.google.inject.Injector;
+
+import junit.framework.Assert;
+
+/**
+ * Tests the {@link JpaSortBuilder}.
+ */
+public class JpaSortBuilderTest {
+
+  private Injector m_injector;
+
+  @Before
+  public void before() {
+    m_injector = Guice.createInjector(new InMemoryDefaultTestModule());
+    m_injector.getInstance(GuiceJpaInitializer.class);
+    m_injector.injectMembers(this);
+  }
+
+  /**
+   * Tests that adding a sort does not create another {@link Root} in the
+   * {@link CriteriaQuery}. A duplicate root will cause a cartesian product
+   * similar to:
+   *
+   * <pre>
+   * SELECT t0.alert_id,
+   *   t0.alert_instance,
+   *   t0.alert_label,
+   *   t0.alert_state,
+   *   t0.alert_text,
+   *   t0.alert_timestamp,
+   *   t0.cluster_id,
+   *   t0.component_name,
+   *   t0.host_name,
+   *   t0.service_name,
+   *   t0.alert_definition_id
+   * FROM   alert_history t0,
+   *   alert_history t2,
+   *   alert_definition t1
+   * WHERE  ( ( t1.definition_name = ? )
+   *     AND ( t1.definition_id = t2.alert_definition_id ) )
+   * ORDER  BY t0.alert_timestamp DESC
+   * </pre>
+   *
+   * where the root for {@code alert_history} is added twice.
+   *
+   * @throws Exception
+   */
+  @Test
+  public void testSortDoesNotAddExtraRootPaths() throws Exception {
+    // create a sort request against the entity directly
+    List<SortRequestProperty> sortRequestProperties = new ArrayList<>();
+
+    sortRequestProperties.add(
+        new SortRequestProperty(AlertHistoryResourceProvider.ALERT_HISTORY_TIMESTAMP,
+            org.apache.ambari.server.controller.spi.SortRequest.Order.ASC));
+
+    SortRequest sortRequest = new SortRequestImpl(sortRequestProperties);
+
+    // create a complex, cross-entity predicate
+    Predicate predicate = new PredicateBuilder().property(
+            AlertHistoryResourceProvider.ALERT_HISTORY_DEFINITION_NAME).equals("foo").toPredicate();
+
+    MockAlertHistoryredicateVisitor visitor = new MockAlertHistoryredicateVisitor();
+    PredicateHelper.visit(predicate, visitor);
+
+    JpaSortBuilder<AlertHistoryEntity> sortBuilder = new JpaSortBuilder<AlertHistoryEntity>();
+    List<Order> sortOrders = sortBuilder.buildSortOrders(sortRequest, visitor);
+
+    Assert.assertEquals(sortOrders.size(), 1);
+
+    // verify the CriteriaQuery has the correct roots
+    // it should have one for the main query predicate
+    CriteriaQuery<AlertHistoryEntity> query = visitor.getCriteriaQuery();
+    Set<Root<?>> roots = query.getRoots();
+    Assert.assertEquals(1, roots.size());
+  }
+
+  /**
+   * The {@link HistoryPredicateVisitor} is used to convert an Ambari
+   * {@link Predicate} into a JPA {@link javax.persistence.criteria.Predicate}.
+   */
+  private final class MockAlertHistoryredicateVisitor
+      extends JpaPredicateVisitor<AlertHistoryEntity> {
+
+    /**
+     * Constructor.
+     *
+     */
+    public MockAlertHistoryredicateVisitor() {
+      super(m_injector.getInstance(EntityManager.class), AlertHistoryEntity.class);
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public Class<AlertHistoryEntity> getEntityClass() {
+      return AlertHistoryEntity.class;
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public List<? extends SingularAttribute<?, ?>> getPredicateMapping(String propertyId) {
+      return AlertHistoryEntity_.getPredicateMapping().get(propertyId);
+    }
+  }
+}