[Blazebit#517] Implemented SET_NULL inverse remove strategy validation

beikov · Oct 30, 2018 · ab97228 · ab97228
1 parent 712c9d4
commit ab97228
Show file tree

Hide file tree

Showing 19 changed files with 238 additions and 22 deletions.
diff --git a/core/impl/src/main/java/com/blazebit/persistence/impl/EmbeddableSplittingVisitor.java b/core/impl/src/main/java/com/blazebit/persistence/impl/EmbeddableSplittingVisitor.java
@@ -26,8 +26,9 @@
 import com.blazebit.persistence.parser.expression.Expression;
 import com.blazebit.persistence.parser.expression.FunctionExpression;
 import com.blazebit.persistence.parser.expression.GeneralCaseExpression;
+import com.blazebit.persistence.parser.expression.ListIndexExpression;
 import com.blazebit.persistence.parser.expression.MapEntryExpression;
-import com.blazebit.persistence.parser.expression.MapValueExpression;
+import com.blazebit.persistence.parser.expression.MapKeyExpression;
 import com.blazebit.persistence.parser.expression.NullExpression;
 import com.blazebit.persistence.parser.expression.NumericLiteral;
 import com.blazebit.persistence.parser.expression.ParameterExpression;
@@ -50,6 +51,7 @@
 import javax.persistence.metamodel.EmbeddableType;
 import javax.persistence.metamodel.EntityType;
 import javax.persistence.metamodel.ManagedType;
+import javax.persistence.metamodel.MapAttribute;
 import javax.persistence.metamodel.PluralAttribute;
 import javax.persistence.metamodel.SingularAttribute;
 import java.util.ArrayList;
@@ -72,7 +74,7 @@ class EmbeddableSplittingVisitor extends AbortableVisitorAdapter {
     protected final AliasManager aliasManager;
     protected final SplittingVisitor splittingVisitor;
     protected final List<Expression> splittedOffExpressions;
-    protected PathExpression expressionToSplit;
+    protected Expression expressionToSplit;
 
     public EmbeddableSplittingVisitor(EntityMetamodel metamodel, JpaProvider jpaProvider, AliasManager aliasManager, SplittingVisitor splittingVisitor) {
         this.metamodel = metamodel;
@@ -102,20 +104,31 @@ public List<Expression> splitOff(Expression expression) {
     protected boolean collectSplittedOffExpressions(Expression expression) {
         splittedOffExpressions.clear();
         if (expressionToSplit != null) {
-            PathReference pathReference = expressionToSplit.getPathReference();
-            JoinNode baseNode = (JoinNode) pathReference.getBaseNode();
-            String fieldPrefix = pathReference.getField() == null ? "" : pathReference.getField() + ".";
+            JoinNode baseNode;
+            String field;
+            if (expressionToSplit instanceof PathExpression) {
+                PathReference pathReference = ((PathExpression) expressionToSplit).getPathReference();
+                baseNode = (JoinNode) pathReference.getBaseNode();
+                field = pathReference.getField();
+            } else if (expressionToSplit instanceof MapKeyExpression) {
+                baseNode = ((JoinNode) ((MapKeyExpression) expressionToSplit).getPath().getBaseNode()).getKeyJoinNode();
+                field = null;
+            } else {
+                // This should never happen
+                return false;
+            }
+            String fieldPrefix = field == null ? "" : field + ".";
             ExtendedManagedType<?> managedType = metamodel.getManagedType(ExtendedManagedType.class, baseNode.getJavaType());
             Set<String> orderedAttributes = new TreeSet<>();
             EntityType<?> ownerType;
-            if (baseNode.getParentTreeNode() == null && pathReference.getField() == null) {
+            if (baseNode.getParentTreeNode() == null && field == null) {
                 ownerType = baseNode.getEntityType();
                 for (SingularAttribute<?, ?> idAttribute : managedType.getIdAttributes()) {
                     addAttributes(ownerType, null, fieldPrefix, "", idAttribute, orderedAttributes);
                 }
             } else {
                 Map<String, ? extends ExtendedAttribute<?, ?>> ownedAttributes;
-                String prefix = pathReference.getField();
+                String prefix = field;
                 if (baseNode.getParentTreeNode() != null && baseNode.getParentTreeNode().getAttribute().getPersistentAttributeType() == Attribute.PersistentAttributeType.ELEMENT_COLLECTION) {
                     String elementCollectionPath = baseNode.getParentTreeNode().getRelationName();
                     ExtendedManagedType entityManagedType = metamodel.getManagedType(ExtendedManagedType.class, baseNode.getParent().getEntityType());
@@ -214,6 +227,37 @@ protected void addAttributes(EntityType<?> ownerType, String elementCollectionPa
         }
     }
 
+    @Override
+    public Boolean visit(ListIndexExpression expression) {
+        return false;
+    }
+
+    @Override
+    public Boolean visit(MapKeyExpression expression) {
+        PathExpression path = expression.getPath();
+        PathReference pathReference = path.getPathReference();
+        while (pathReference == null) {
+            Expression aliasedExpression = ((SelectInfo) aliasManager.getAliasInfo(path.toString())).getExpression();
+            if (aliasedExpression instanceof PathExpression) {
+                path = (PathExpression) aliasedExpression;
+                pathReference = path.getPathReference();
+            } else {
+                // This should never happen
+                return false;
+            }
+        }
+
+        JoinNode baseNode = (JoinNode) pathReference.getBaseNode();
+        Attribute attr;
+        if (baseNode.getParentTreeNode() != null) {
+            attr = baseNode.getParentTreeNode().getAttribute();
+            if (attr instanceof MapAttribute<?, ?, ?> && ((MapAttribute<?, ?, ?>) attr).getKeyType() instanceof EmbeddableType<?>) {
+                expressionToSplit = expression;
+            }
+        }
+        return false;
+    }
+
     @Override
     public Boolean visit(NullExpression expression) {
         // The actual semantics of NULL are, that NULL != NULL
@@ -267,11 +311,6 @@ public Boolean visit(MapEntryExpression expression) {
         return false;
     }
 
-    @Override
-    public Boolean visit(MapValueExpression expression) {
-        return false;
-    }
-
     @Override
     public Boolean visit(SubqueryExpression expression) {
         return false;

diff --git a/core/impl/src/main/java/com/blazebit/persistence/impl/JoinNode.java b/core/impl/src/main/java/com/blazebit/persistence/impl/JoinNode.java
@@ -530,6 +530,11 @@ public Map<String, JoinNode> getTreatedJoinNodes() {
         return treatedJoinNodes;
     }
 
+    public JoinNode getKeyJoinNode() {
+        JoinTreeNode treeNode = getOrCreateTreeNode("KEY(" + getParentTreeNode().getRelationName() + ")", getParentTreeNode().getAttribute());
+        return treeNode.getDefaultNode();
+    }
+
     public JoinTreeNode getOrCreateTreeNode(String joinRelationName, Attribute<?, ?> attribute) {
         JoinTreeNode node = nodes.get(joinRelationName);
 

diff --git a/core/impl/src/main/java/com/blazebit/persistence/impl/SplittingVisitor.java b/core/impl/src/main/java/com/blazebit/persistence/impl/SplittingVisitor.java
@@ -19,6 +19,8 @@
 import com.blazebit.persistence.parser.EntityMetamodel;
 import com.blazebit.persistence.parser.expression.Expression;
 import com.blazebit.persistence.parser.expression.LazyCopyingResultVisitorAdapter;
+import com.blazebit.persistence.parser.expression.MapKeyExpression;
+import com.blazebit.persistence.parser.expression.MapValueExpression;
 import com.blazebit.persistence.parser.expression.PathElementExpression;
 import com.blazebit.persistence.parser.expression.PathExpression;
 import com.blazebit.persistence.parser.expression.PropertyExpression;
@@ -38,7 +40,7 @@ public class SplittingVisitor extends LazyCopyingResultVisitorAdapter {
     private final EntityMetamodel metamodel;
     private final JpaProvider jpaProvider;
     private final AliasManager aliasManager;
-    private PathExpression expressionToSplit;
+    private Expression expressionToSplit;
     private String subAttribute;
 
     public SplittingVisitor(EntityMetamodel metamodel, JpaProvider jpaProvider, AliasManager aliasManager) {
@@ -47,6 +49,45 @@ public SplittingVisitor(EntityMetamodel metamodel, JpaProvider jpaProvider, Alia
         this.aliasManager = aliasManager;
     }
 
+    @Override
+    public Expression visit(MapKeyExpression expression) {
+        if (expression == expressionToSplit) {
+            List<PathElementExpression> expressions = new ArrayList<>(2);
+            expressions.add(expression);
+            for (String subAttributePart : subAttribute.split("\\.")) {
+                expressions.add(new PropertyExpression(subAttributePart));
+            }
+
+            JoinNode node = ((JoinNode) expression.getPath().getBaseNode()).getKeyJoinNode();
+            String field = subAttribute;
+            Class<?> fieldClass = jpaProvider.getJpaMetamodelAccessor().getAttributePath(metamodel, node.getManagedType(), field).getAttributeClass();
+            Type<?> fieldType = metamodel.type(fieldClass);
+
+            return new PathExpression(
+                    expressions,
+                    new SimplePathReference(node, field, fieldType),
+                    false,
+                    false
+            );
+        }
+        return expression;
+    }
+
+    @Override
+    public Expression visit(MapValueExpression expression) {
+        if (expression == expressionToSplit) {
+            Expression newExpression = expression.getPath().accept(this);
+            if (newExpression != expression.getPath()) {
+                // A split map value expression is not surrounded by the map value operator anymore
+                return newExpression;
+            }
+        } else {
+            return expression.getPath().accept(this);
+        }
+
+        return expression;
+    }
+
     @Override
     public Expression visit(PathExpression expression) {
         if (expression.getBaseNode() == null) {
@@ -81,7 +122,7 @@ public Expression visit(PathExpression expression) {
         return expression;
     }
 
-    public Expression splitOff(Expression expression, PathExpression expressionToSplit, String subAttribute) {
+    public Expression splitOff(Expression expression, Expression expressionToSplit, String subAttribute) {
         this.expressionToSplit = expressionToSplit;
         this.subAttribute = subAttribute;
         try {

diff --git a/...r/src/main/java/com/blazebit/persistence/parser/PathTargetResolvingExpressionVisitor.java b/...r/src/main/java/com/blazebit/persistence/parser/PathTargetResolvingExpressionVisitor.java
@@ -164,8 +164,13 @@ PathPosition copy() {
     public PathTargetResolvingExpressionVisitor(EntityMetamodel metamodel, Type<?> startClass, String skipBaseNodeAlias) {
         this.metamodel = metamodel;
         this.pathPositions = new ArrayList<>();
-        this.pathPositions.add(currentPosition = new PathPosition(startClass, null));
         this.skipBaseNodeAlias = skipBaseNodeAlias;
+        reset(startClass);
+    }
+
+    public void reset(Type<?> startClass) {
+        pathPositions.clear();
+        pathPositions.add(currentPosition = new PathPosition(startClass, null));
     }
 
     private Type<?> getType(Type<?> baseType, Attribute<?, ?> attribute) {

diff --git a/core/parser/src/main/java/com/blazebit/persistence/parser/expression/MapValueExpression.java b/core/parser/src/main/java/com/blazebit/persistence/parser/expression/MapValueExpression.java
@@ -30,7 +30,7 @@ public MapValueExpression(PathExpression path) {
 
     @Override
     public MapValueExpression clone(boolean resolved) {
-        return new MapValueExpression((PathExpression) path.clone(resolved));
+        return new MapValueExpression(path.clone(resolved));
     }
 
     @Override

diff --git a/core/testsuite/src/test/java/com/blazebit/persistence/testsuite/GroupByTest.java b/core/testsuite/src/test/java/com/blazebit/persistence/testsuite/GroupByTest.java
@@ -150,4 +150,30 @@ public void testGroupByEmbeddable() {
         }
         cb.getResultList();
     }
+
+    @Test
+    public void testGroupByElementCollectionKey() {
+        CriteriaBuilder<Long> cb = cbf.create(em, Long.class)
+                .from(Document.class, "d")
+                .select("d.id")
+                .groupBy("KEY(nameMap)");
+
+        assertEquals("SELECT d.id FROM Document d LEFT JOIN d.nameMap nameMap_1 GROUP BY KEY(nameMap_1), d.id", cb.getQueryString());
+        cb.getResultList();
+    }
+
+    @Test
+    public void testGroupByElementCollectionValue() {
+        CriteriaBuilder<Long> cb = cbf.create(em, Long.class)
+                .from(Document.class, "d")
+                .select("d.id")
+                .groupBy("VALUE(nameMap)");
+
+        if (jpaProvider.supportsSingleValuedAssociationIdExpressions()) {
+            assertEquals("SELECT d.id FROM Document d LEFT JOIN d.nameMap nameMap_1 GROUP BY nameMap_1.intIdEntity.id, nameMap_1.primaryName, nameMap_1.secondaryName, d.id", cb.getQueryString());
+        } else {
+            assertEquals("SELECT d.id FROM Document d LEFT JOIN d.nameMap nameMap_1 LEFT JOIN nameMap_1.intIdEntity intIdEntity_1 GROUP BY intIdEntity_1.id, nameMap_1.primaryName, nameMap_1.secondaryName, d.id", cb.getQueryString());
+        }
+        cb.getResultList();
+    }
 }
diff --git a/core/testsuite/src/test/java/com/blazebit/persistence/testsuite/JpqlFunctionTest.java b/core/testsuite/src/test/java/com/blazebit/persistence/testsuite/JpqlFunctionTest.java
@@ -25,6 +25,7 @@
 
 import com.blazebit.persistence.spi.JpqlFunction;
 import com.blazebit.persistence.testsuite.base.jpa.category.NoEclipselink;
+import com.blazebit.persistence.testsuite.entity.DocumentType;
 import com.blazebit.persistence.testsuite.tx.TxVoidWork;
 import org.junit.Test;
 
@@ -132,6 +133,7 @@ public void builtinFunctionsReturnCorrectTypes() {
         String coalesceName = resolveRegisteredFunctionName("coalesce");
         String countName = resolveRegisteredFunctionName("count");
         String lengthName = resolveRegisteredFunctionName("length");
+        String maxName = resolveRegisteredFunctionName("max");
         Map<String, JpqlFunction> functions = cbf.getRegisteredFunctions();
         assertEquals(String.class, functions.get(coalesceName).getReturnType(String.class));
         assertEquals(Integer.class, functions.get(coalesceName).getReturnType(Integer.class));
@@ -140,5 +142,6 @@ public void builtinFunctionsReturnCorrectTypes() {
         assertEquals(Long.class, functions.get(countName).getReturnType(Integer.class));
         assertEquals(Long.class, functions.get(countName).getReturnType(Long.class));
         assertEquals(Integer.class, functions.get(lengthName).getReturnType(String.class));
+        assertEquals(DocumentType.class, functions.get(maxName).getReturnType(DocumentType.class));
     }
 }
diff --git a/...ew/impl/src/main/java/com/blazebit/persistence/view/impl/metamodel/AbstractAttribute.java b/...ew/impl/src/main/java/com/blazebit/persistence/view/impl/metamodel/AbstractAttribute.java
@@ -772,7 +772,6 @@ public void checkAttribute(ManagedType<?> managedType, MetamodelBuildingContext
                         context.addError("Multiple possible target type for the mapping in the " + getLocation() + ": " + possibleTargets);
                     }
 
-                    javax.persistence.metamodel.Attribute<?, ?> jpaAttribute = possibleTargets.keySet().iterator().next();
                     // TODO: maybe allow to override this per-attribute?
                     if (isDisallowOwnedUpdatableSubview()) {
                         for (Type<?> updateCascadeAllowedSubtype : getUpdateCascadeAllowedSubtypes()) {

diff --git a/...l/src/main/java/com/blazebit/persistence/view/impl/metamodel/AbstractMethodAttribute.java b/...l/src/main/java/com/blazebit/persistence/view/impl/metamodel/AbstractMethodAttribute.java
@@ -17,10 +17,12 @@
 package com.blazebit.persistence.view.impl.metamodel;
 
 import com.blazebit.annotation.AnnotationUtils;
+import com.blazebit.persistence.parser.PathTargetResolvingExpressionVisitor;
 import com.blazebit.persistence.parser.expression.SyntaxErrorException;
 import com.blazebit.persistence.view.AttributeFilter;
 import com.blazebit.persistence.view.AttributeFilters;
 import com.blazebit.persistence.view.IdMapping;
+import com.blazebit.persistence.view.InverseRemoveStrategy;
 import com.blazebit.persistence.view.LockMode;
 import com.blazebit.persistence.view.Mapping;
 import com.blazebit.persistence.view.MappingCorrelated;
@@ -42,6 +44,8 @@
 import com.blazebit.reflection.ReflectionUtils;
 
 import javax.persistence.metamodel.Attribute;
+import javax.persistence.metamodel.ManagedType;
+import javax.persistence.metamodel.SingularAttribute;
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
 import java.util.Collection;
@@ -285,6 +289,51 @@ protected boolean determineOptimisticLockProtected(MethodAttributeMapping mappin
         }
     }
 
+    @Override
+    public void checkAttribute(ManagedType<?> managedType, MetamodelBuildingContext context) {
+        super.checkAttribute(managedType, context);
+        if (isUpdatable() && declaringType.isUpdatable()) {
+            String mappedBy = getMappedBy();
+            if (mappedBy != null && getInverseRemoveStrategy() == InverseRemoveStrategy.SET_NULL) {
+                Type<?> elementType = getElementType();
+                ManagedType<?> elementJpaType;
+                if (elementType instanceof ManagedViewTypeImplementor<?>) {
+                    elementJpaType = ((ManagedViewTypeImplementor<?>) elementType).getJpaManagedType();
+                } else {
+                    elementJpaType = ((BasicTypeImpl<?>) elementType).getManagedType();
+                }
+                Map<String, String> writableMappedByMappings = getWritableMappedByMappings();
+                if (writableMappedByMappings == null) {
+                    Attribute<?, ?> attribute = elementJpaType.getAttribute(mappedBy);
+                    // SET_NULL for plural attributes i.e. @ManyToMany is like removing just the join table entry
+                    // So we just care about singular attributes here
+                    if (attribute instanceof SingularAttribute<?, ?>) {
+                        if (!((SingularAttribute<?, ?>) attribute).isOptional()) {
+                            context.addError("Illegal use of the remove strategy SET_NULL for non-nullable mapped by attribute '" + mappedBy + "' at " + getLocation() + " Use a different strategy via @MappingInverse(removeStrategy = InverseRemoveStrategy...)");
+                        }
+                    }
+                } else {
+                    PathTargetResolvingExpressionVisitor visitor = new PathTargetResolvingExpressionVisitor(context.getEntityMetamodel(), elementJpaType, null);
+                    for (String value : writableMappedByMappings.values()) {
+                        visitor.reset(elementJpaType);
+                        context.getTypeValidationExpressionFactory().createPathExpression(value).accept(visitor);
+                        Map<Attribute<?, ?>, javax.persistence.metamodel.Type<?>> possibleTargets = visitor.getPossibleTargets();
+                        if (possibleTargets.size() > 1) {
+                            context.addError("Multiple possible target type for the mapping in the " + getLocation() + ": " + possibleTargets);
+                        }
+                        Attribute<?, ?> attribute = possibleTargets.keySet().iterator().next();
+                        if (attribute instanceof SingularAttribute<?, ?>) {
+                            if (!((SingularAttribute<?, ?>) attribute).isOptional()) {
+                                context.addError("Illegal use of the remove strategy SET_NULL for non-nullable mapped by attribute '" + mappedBy + "' because writable mapping '" + value + "' is non-optional at " + getLocation() + " Use a different strategy via @MappingInverse(removeStrategy = InverseRemoveStrategy...)");
+                            }
+                        }
+                    }
+
+                }
+            }
+        }
+    }
+
     protected static String getAttributeName(Method getterOrSetter) {
         String name = getterOrSetter.getName();
         StringBuilder sb = new StringBuilder(name.length());

diff --git a/...main/java/com/blazebit/persistence/view/impl/metamodel/AbstractMethodPluralAttribute.java b/...main/java/com/blazebit/persistence/view/impl/metamodel/AbstractMethodPluralAttribute.java
@@ -161,7 +161,7 @@ public AbstractMethodPluralAttribute(ManagedViewTypeImplementor<X> viewType, Met
             }
         }
 
-        if (this.inverseRemoveStrategy == null && mapping.getInverseRemoveStrategy() != null) {
+        if (this.inverseRemoveStrategy == null && mapping.getInverseRemoveStrategy() != null && this.dirtyStateIndex != -1) {
             context.addError("Found use of @MappingInverse on attribute that isn't an inverse relationship. Invalid definition found on the " + mapping.getErrorLocation() + "!");
         }
 

diff --git a/...in/java/com/blazebit/persistence/view/impl/metamodel/AbstractMethodSingularAttribute.java b/...in/java/com/blazebit/persistence/view/impl/metamodel/AbstractMethodSingularAttribute.java
@@ -181,7 +181,7 @@ public AbstractMethodSingularAttribute(ManagedViewTypeImplementor<X> viewType, M
             }
         }
 
-        if (this.inverseRemoveStrategy == null && mapping.getInverseRemoveStrategy() != null) {
+        if (this.inverseRemoveStrategy == null && mapping.getInverseRemoveStrategy() != null && this.dirtyStateIndex != -1) {
             context.addError("Found use of @MappingInverse on attribute that isn't an inverse relationship. Invalid definition found on the " + mapping.getErrorLocation() + "!");
         }
 

diff --git a/...pl/objectbuilder/transformer/correlation/AbstractCorrelatedBatchTupleListTransformer.java b/...pl/objectbuilder/transformer/correlation/AbstractCorrelatedBatchTupleListTransformer.java
@@ -141,7 +141,7 @@ private String applyAndGetCorrelationRoot(BatchCorrelationMode batchCorrelationM
                 }
             } else {
                 // The correlation key is basic type
-                correlationSelectExpression = correlationKeyExpression = CORRELATION_KEY_ALIAS + ".value";
+                correlationSelectExpression = correlationKeyExpression = CORRELATION_KEY_ALIAS;
             }
         } else {
             this.correlationParamName = generateCorrelationParamName();
-Original file line number
+Diff line change
@@ Expand Up @@
                 }
             }
-            if (this.inverseRemoveStrategy == null && mapping.getInverseRemoveStrategy() != null) {
+            if (this.inverseRemoveStrategy == null && mapping.getInverseRemoveStrategy() != null && this.dirtyStateIndex != -1) {
                 context.addError("Found use of @MappingInverse on attribute that isn't an inverse relationship. Invalid definition found on the " + mapping.getErrorLocation() + "!");
             }
@@ Expand Down @@