Merge pull request #145 from JFrogDev/master

NullPointerException is thrown when cascade deleting a broken -to-many relationship

grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/engine/NativeEntryEntityPersister.java

@@ -144,7 +144,12 @@ protected void deleteEntity(PersistentEntity persistentEntity, Object obj) {
         firePostDeleteEvent(persistentEntity, entityAccess);
     }
 
-    protected void cascadeDeleteCollection(Collection collection) {
+    protected void cascadeDeleteCollection(EntityAccess entityAccess, Association association) {
+        Object propValue = entityAccess.getProperty(association.getName());
+        if (!(propValue instanceof Collection)) {
+            return;
+        }
+        Collection collection = ((Collection) propValue);
         for (Iterator iter = collection.iterator(); iter.hasNext(); ) {
             Object child = iter.next();
             deleteEntity(getMappingContext().getPersistentEntity(child.getClass().getName()), child);
@@ -188,18 +193,17 @@ protected void cascadeBeforeDelete(PersistentEntity persistentEntity, EntityAcce
             if (prop instanceof OneToMany) {
                 OneToMany oneToMany = (OneToMany)prop;
                 if (oneToMany.isOwningSide() && oneToMany.doesCascade(CascadeType.REMOVE)) {
-                    Object propValue = entityAccess.getProperty(oneToMany.getName());
-                    if (propValue instanceof Collection) {
-                        cascadeDeleteCollection((Collection) propValue);
+                    if (Collection.class.isAssignableFrom(oneToMany.getType())) {
+                        cascadeDeleteCollection(entityAccess, oneToMany);
                     }
                 }
             }
             else if (prop instanceof ManyToMany) {
                 ManyToMany manyToMany = (ManyToMany)prop;
                 if (manyToMany.isOwningSide() && manyToMany.doesCascade(CascadeType.REMOVE)) {
                     Object propValue = entityAccess.getProperty(manyToMany.getName());
-                    if (propValue instanceof Collection) {
-                        cascadeDeleteCollection((Collection) propValue);
+                    if (Collection.class.isAssignableFrom(manyToMany.getType())) {
+                        cascadeDeleteCollection(entityAccess, manyToMany);
                     }
                 }
             }
@@ -221,9 +225,8 @@ protected void cascadeAfterDelete(PersistentEntity persistentEntity, EntityAcces
             else if (prop instanceof OneToMany) {
                 OneToMany oneToMany = (OneToMany)prop;
                 if (oneToMany.isOwningSide() && oneToMany.doesCascade(CascadeType.REMOVE)) {
-                    Object propValue = entityAccess.getProperty(oneToMany.getName());
-                    if (propValue instanceof Collection) {
-                        cascadeDeleteCollection((Collection) propValue);
+                    if (Collection.class.isAssignableFrom(oneToMany.getType())) {
+                        cascadeDeleteCollection(entityAccess, oneToMany);
                     }
                 }
             }

grails-datastore-gorm-mongo/src/test/groovy/org/grails/datastore/gorm/mongo/BrokenManyToManyAssociationSpec.groovy

@@ -0,0 +1,61 @@
+package org.grails.datastore.gorm.mongo
+
+import com.mongodb.BasicDBObject
+import com.mongodb.DBCollection
+import grails.gorm.tests.GormDatastoreSpec
+import grails.persistence.Entity
+
+/**
+ * @author Noam Y. Tenne
+ */
+class BrokenManyToManyAssociationSpec extends GormDatastoreSpec {
+
+    def 'Perform a cascading delete on a broken many-to-many relationship'() {
+        given:'An owning entity with 2 owned entities'
+        ReferencingEntity referencing = new ReferencingEntity()
+        referencing = referencing.save(flush: true)
+        referencing.addToReferencedEntities(new ReferencedEntity().save())
+        referencing.addToReferencedEntities(new ReferencedEntity().save())
+
+        referencing.save(flush: true)
+        session.clear()
+
+        when:'Low-level deleting 1 owned entity to simulate a broken relationship'
+        ((DBCollection) ReferencedEntity.collection).remove(new BasicDBObject('_id', ReferencedEntity.find{}.id))
+        session.clear()
+        referencing = ReferencingEntity.find{}
+
+        then:'Expect to still find 2 owned entities, but 1 of them is null (because the reference is broken)'
+        referencing.referencedEntities.size() == 2
+        referencing.referencedEntities.any { it == null }
+
+        and:
+        when:'Deleting the owning entity, thus invoking a cascading delete'
+        referencing.delete(flush: true)
+        session.clear()
+
+        then:'Expect all the entities to be removed with no error'
+        ReferencedEntity.count == 0
+        ReferencingEntity.count == 0
+    }
+
+    @Override
+    List getDomainClasses() {
+        [ReferencingEntity, ReferencedEntity]
+    }
+}
+
+@Entity
+class ReferencingEntity {
+    String id
+    Set<ReferencedEntity> referencedEntities
+    static hasMany = [referencedEntities: ReferencedEntity]
+}
+
+@Entity
+class ReferencedEntity {
+    String id
+    static belongsTo = ReferencingEntity
+    Set<ReferencingEntity> referencingEntities
+    static hasMany = [referencingEntities: ReferencingEntity]
+}

grails-datastore-mongo/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoEntityPersister.java

@@ -51,6 +51,8 @@
 import org.grails.datastore.mapping.mongo.config.MongoMappingContext;
 import org.grails.datastore.mapping.mongo.query.MongoQuery;
 import org.grails.datastore.mapping.query.Query;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.springframework.context.ApplicationEventPublisher;
 import org.springframework.core.convert.ConversionService;
 import org.springframework.core.convert.TypeDescriptor;
@@ -79,6 +81,8 @@
 @SuppressWarnings({"rawtypes", "unchecked"})
 public class MongoEntityPersister extends NativeEntryEntityPersister<DBObject, Object> {
 
+    static Logger log = LoggerFactory.getLogger(MongoEntityPersister.class);
+
     private static final String NEXT_ID_SUFFIX = ".next_id";
     private boolean hasNumericalIdentifier = false;
     private boolean hasStringIdentifier = false;
@@ -714,10 +718,20 @@ public Object doInDB(DB con) throws MongoException, DataAccessException {
     }
 
     @Override
-    protected void cascadeDeleteCollection(Collection collection) {
+    protected void cascadeDeleteCollection(EntityAccess entityAccess, Association association) {
+        Object propValue = entityAccess.getProperty(association.getName());
+        if (!(propValue instanceof Collection)) {
+            return;
+        }
+        Collection collection = ((Collection) propValue);
         Persister persister = null;
         for (Iterator iter = collection.iterator(); iter.hasNext(); ) {
             Object child = iter.next();
+            if (child == null) {
+                log.warn("Encountered a null associated reference while cascade-deleting '{}' as part of {} (ID {})",
+                        association.getReferencedPropertyName(), entityAccess.getEntity().getClass().getName(), entityAccess.getIdentifier());
+                continue;
+            }
             if(persister == null) {
                 persister = session.getPersister(child);
             }