Attachment 160820 Details for Bug 246211 – proposed patch

Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

[patch] proposed patch

246211.patch (text/plain), 13.65 KB, created by Chris Delahunt

on 2010-03-03 13:33:19 EST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Delahunt

Created: 2010-03-03 13:33:19 EST

Size: 13.65 KB

patch

obsolete

>Index: foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/AttributeNode.java
>===================================================================
>--- foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/AttributeNode.java	(revision 6703)
>+++ foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/AttributeNode.java	(working copy)
>@@ -80,8 +80,12 @@
>             if (context.hasMemberOfNode()) {
>                 return parentExpression.noneOf(name, new ExpressionBuilder().equal(context.getMemberOfNode().getLeftExpression()));
>             }
>-            return outerJoin ? parentExpression.anyOfAllowingNone(name) : 
>-                parentExpression.anyOf(name);
>+            //Bug246211: need to use an outer join if specified by the context
>+            if (  isOuterJoin() || context.shouldUseOuterJoins() ) {
>+                return parentExpression.anyOfAllowingNone(name);
>+            } else {
>+                return parentExpression.anyOf(name);
>+            }
>         } else {
>             // check whether collection attribute is required
>             if (requiresCollectionAttribute()) {
>@@ -89,8 +93,9 @@
>                     context.getParseTreeContext().getQueryInfo(), 
>                     getLine(), getColumn(), name);
>             }
>-
>-            if (context.shouldUseOuterJoins() || isOuterJoin()) {
>+            //Bug246211: context changed to have shouldUseOuterJoins set more often. Need to check that it is a reference 
>+            //  mapping before using the outer join 
>+            if (  isOuterJoin() || (context.shouldUseOuterJoins()&& this.mapping!=null && this.mapping.isForeignReferenceMapping())) {
>                 return parentExpression.getAllowingNull(name);
>             } else {
>                 return parentExpression.get(name);
>Index: foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/ConstructorNode.java
>===================================================================
>--- foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/ConstructorNode.java	(revision 6703)
>+++ foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/ConstructorNode.java	(working copy)
>@@ -51,14 +51,13 @@
>             ReportQuery reportQuery = (ReportQuery)theQuery;
>             reportQuery.beginAddingConstructorArguments(
>                 getConstructorClass(context.getParseTreeContext()));
>+            //bug246211: use outer joins outside the where clause, as inner joins will filter out null results
>+            selectContext.useOuterJoins();
>             for (Iterator i = constructorItems.iterator(); i.hasNext();) {
>                 Node node = (Node)i.next();
>-                if (selectingRelationshipField(node, context)) {
>-                    selectContext.useOuterJoins();
>-                }
>                 node.applyToQuery(reportQuery, context);
>-                selectContext.dontUseOuterJoins();
>             }
>+            selectContext.dontUseOuterJoins();
>             reportQuery.endAddingToConstructorItem();
>         }
>     }
>@@ -136,16 +135,6 @@
> 
>     /**
>      * INTERNAL
>-     */
>-    private boolean selectingRelationshipField(Node node, GenerationContext context) {
>-        if ((node == null) || !node.isDotNode()) {
>-            return false;
>-        }
>-        return !((DotNode)node).endsWithDirectToField(context);
>-    }
>-
>-    /**
>-     * INTERNAL
>      * Get the string representation of this node.
>      */
>     public String getAsString() {
>Index: foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/GroupByNode.java
>===================================================================
>--- foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/GroupByNode.java	(revision 6703)
>+++ foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/GroupByNode.java	(working copy)
>@@ -76,10 +76,14 @@
>     public void addGroupingToQuery(ObjectLevelReadQuery theQuery, GenerationContext context) {
>         if (theQuery.isReportQuery()) {
>             Iterator iter = getGroupByItems().iterator();
>+            SelectGenerationContext selectContext = (SelectGenerationContext)context;
>+            //bug246211: outer joins are used outside the where clause, as inner joins will filter null results
>+            selectContext.useOuterJoins();
>             while (iter.hasNext()) {
>                 Node nextNode = (Node)iter.next();
>                 ((ReportQuery)theQuery).addGrouping(nextNode.generateExpression(context));
>             }
>+            selectContext.dontUseOuterJoins();
>         }
>     }
> 
>Index: foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/HavingNode.java
>===================================================================
>--- foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/HavingNode.java	(revision 6703)
>+++ foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/HavingNode.java	(working copy)
>@@ -52,8 +52,12 @@
>      */
>     public void addHavingToQuery(ObjectLevelReadQuery theQuery, GenerationContext context) {
>         if (theQuery.isReportQuery()) {
>+            //bug246211: outer joins are used outside the where clause, as inner joins will filter null results
>+            SelectGenerationContext selectContext = (SelectGenerationContext)context;
>+            selectContext.useOuterJoins();
>             Expression havingExpression = getHaving().generateExpression(context);
>             ((ReportQuery)theQuery).setHavingExpression(havingExpression);
>+            selectContext.dontUseOuterJoins();
>         }
>     }
> 
>Index: foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/SelectNode.java
>===================================================================
>--- foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/SelectNode.java	(revision 6703)
>+++ foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/SelectNode.java	(working copy)
>@@ -160,11 +160,11 @@
>             }
>         }
>         SelectGenerationContext selectContext = (SelectGenerationContext)context;
>+        //bug246211: outer joins are used on all nodes in the select, allowing nested DotNodes to each use an outer join 
>+        selectContext.useOuterJoins();
>         for (int i=0;i<selectExpressions.size();i++){
>             Node node = (Node)selectExpressions.get(i);
>-            if (selectingRelationshipField(node, context)) {
>-                selectContext.useOuterJoins();
>-            }
>+
>             if (node.isAliasableNode() && identifiers != null){
>                 String alias = (String)identifiers.get(i);
>                 if (alias != null){
>@@ -172,9 +172,8 @@
>                 }
>             }
>             node.applyToQuery(readQuery, context); 
>-            selectContext.dontUseOuterJoins();
>         }
>-
>+        selectContext.dontUseOuterJoins();
>         //indicate on the query if "return null if primary key null"
>         //This means we want nulls returned if we expect an outer join
>         readQuery.setShouldBuildNullForNullPk(this.hasOneToOneSelected(context));
>@@ -371,17 +370,6 @@
>         return false;
>     }
> 
>-    private boolean selectingRelationshipField(Node node, GenerationContext context) {
>-        if ((node == null) || !node.isDotNode()) {
>-            return false;
>-        }
>-        TypeHelper typeHelper = context.getParseTreeContext().getTypeHelper();
>-        Node path = node.getLeft();
>-        AttributeNode attribute = (AttributeNode)node.getRight();
>-        return typeHelper.isRelationship(path.getType(), 
>-                                         attribute.getAttributeName());
>-    }
>-
>     /**
>      * Answer true if the SELECT ends in a direct-to-field.
>      * true: SELECT phone.areaCode
>Index: foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/TypeHelperImpl.java
>===================================================================
>--- foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/TypeHelperImpl.java	(revision 6703)
>+++ foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/jpa/parsing/TypeHelperImpl.java	(working copy)
>@@ -143,10 +143,7 @@
>     public boolean isRelationship(Object ownerClass, String attribute) {
>         DatabaseMapping mapping = 
>             resolveAttributeMapping(ownerClass, attribute);
>-        return (mapping != null) && 
>-               (mapping.isObjectReferenceMapping() || 
>-                mapping.isOneToManyMapping() || 
>-                mapping.isManyToManyMapping());
>+        return (mapping != null) && mapping.isForeignReferenceMapping();
>     }
> 
>     /** Returns true if the specified attribute denotes a single valued
>Index: jpa/eclipselink.jpa.test/src/org/eclipse/persistence/testing/tests/jpa/jpql/JUnitJPQLComplexTestSuite.java
>===================================================================
>--- jpa/eclipselink.jpa.test/src/org/eclipse/persistence/testing/tests/jpa/jpql/JUnitJPQLComplexTestSuite.java	(revision 6703)
>+++ jpa/eclipselink.jpa.test/src/org/eclipse/persistence/testing/tests/jpa/jpql/JUnitJPQLComplexTestSuite.java	(working copy)
>@@ -164,6 +164,9 @@
>         suite.addTest(new JUnitJPQLComplexTestSuite("complexNavigatingTwoLevelOfEmbeddeds"));
>         suite.addTest(new JUnitJPQLComplexTestSuite("complexNamedQueryResultPropertiesTest"));
>         suite.addTest(new JUnitJPQLComplexTestSuite("complexOuterJoinQuery"));
>+        suite.addTest(new JUnitJPQLComplexTestSuite("complexCountOuterJoinTest"));
>+        suite.addTest(new JUnitJPQLComplexTestSuite("complexCountNestedOuterJoinTest"));
>+        suite.addTest(new JUnitJPQLComplexTestSuite("complexOuterJoinGroupByTest"));
>         
>         suite.addTest(new JUnitJPQLComplexTestSuite("complexInheritanceTest"));
>         suite.addTest(new JUnitJPQLComplexTestSuite("complexInheritanceUsingNamedQueryTest"));
>@@ -1711,6 +1714,78 @@
>         Assert.assertNull  ("Complex outer join query (2): unexpected result value (1, 3):", result1[3]);
>                 
>     }
>+    
>+    /**
>+     * Test bug 246211: Inner join used outside of the where clause causes unintentional filtering of results.
>+     * Ie "Select count(a.b) from A a" can never return 0 since all As with 0 Bs get filtered out if an inner join
>+     * is used.  
>+     */
>+    public void complexCountOuterJoinTest()
>+    {
>+        EntityManager em = createEntityManager();
>+        String jpql = "Select e.lastName, count(e.department) from Employee e group by e.lastName";
>+        Query q = em.createQuery(jpql);
>+        List results = q.getResultList();
>+        boolean passed = false;
>+        Iterator i = results.iterator();
>+        //check all rows until a row with count=0 is found
>+        while (!passed && i.hasNext() ){
>+            Object[] row = (Object[])i.next();
>+            if (((Number)row[1]).intValue() == 0) {
>+                passed = true;
>+            }
>+        }
>+        
>+        this.assertTrue("complexCountOuterJoinTest did not return results rows with a count of 0", passed);      
>+    }
>+    
>+    /**
>+     * Test bug 246211: Inner join used outside of the where clause causes unintentional filtering of results.
>+     * Ie "Select count(a.b.c) from A a" can never return 0 since all As with 0 Bs get filtered out if an inner join
>+     * is used.  This test tests that an outer join is used from A->B as well as B->C
>+     */
>+    public void complexCountNestedOuterJoinTest()
>+    {
>+        EntityManager em = createEntityManager();
>+        String jpql = "Select e.lastName, count(e.department.managers) from Employee e group by e.lastName";
>+        Query q = em.createQuery(jpql);
>+        List results = q.getResultList();
>+        
>+        boolean passed = false;
>+        Iterator i = results.iterator();
>+        //check all rows until a row with count=0 is found
>+        while (!passed && i.hasNext() ){
>+            Object[] row = (Object[])i.next();
>+            if (((Number)row[1]).intValue() == 0) {
>+                passed = true;
>+            }
>+        }
>+        
>+        this.assertTrue("complexCountNestedOuterJoinTest did not return results rows with a count of 0", passed);      
>+    }
>+    
>+    /**
>+     * Test bug 246211: Inner join used outside of the where clause causes unintentional filtering of results.
>+     * This tests verifies that the group by statement uses an outer join
>+     */
>+    public void complexOuterJoinGroupByTest(){
>+        EntityManager em = createEntityManager();
>+        //Employee to department is a many to one relation.  
>+        String jpql = "Select count(e), e.department from Employee e group by e.department";
>+        Query q = em.createQuery(jpql);
>+        List results = q.getResultList();Employee e;
>+        boolean passed = false;
>+        Iterator i = results.iterator();
>+        //check all rows until a row with null departments is found
>+        while (!passed && i.hasNext() ){
>+            Object[] row = (Object[])i.next();
>+            if ( row[1]  == null ){
>+                //verify that employees with a null department are included in the results
>+                passed = true;
>+            }
>+        }
>+        this.assertTrue("complexOuterJoinCountTest did not return results rows with a count of 0", passed);  
>+    }
> 
>     // Helper methods and classes for constructor query test cases
>

Actions: View | Diff

Attachments on bug 246211: 160820