Idempotent DELETE by ids, optimizing performance #659

agrestio · Dec 1, 2023 · 8dcb1cf · 8dcb1cf
1 parent beab1cb
commit 8dcb1cf
Show file tree

Hide file tree

Showing 7 changed files with 97 additions and 20 deletions.
diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md
@@ -1,6 +1,7 @@
 ## Release 5.0-M20
 
 * #658 DeleteBuilder: byIds(..), byMultiIds(..)
+* #659 Idempotent DELETE by ids, optimizing performance
 
 ## Release 5.0.M19
 

diff --git a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/CayenneQueryAssembler.java b/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/CayenneQueryAssembler.java
@@ -37,6 +37,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.function.Consumer;
+import java.util.function.Function;
 
 /**
  * @since 3.4
@@ -119,6 +120,58 @@ public <T> Property<?>[] queryColumns(RelatedResourceEntity<T> entity) {
         return columns;
     }
 
+    @Override
+    public <T> ObjectSelect<T> createQueryForIds(AgEntity<T> entity, Collection<AgObjectId> ids) {
+
+        if (ids.isEmpty()) {
+            throw AgException.badRequest("No ids specified");
+        }
+
+        ObjEntity objEntity = entityResolver.getObjEntity(entity.getType());
+
+        // sanity checking...
+        if (objEntity == null) {
+            throw AgException.internalServerError("Unknown entity class: %s", entity.getType());
+        }
+
+        Function<AgObjectId, Expression> qualifierMaker = idQualifierMaker(entity);
+        List<Expression> idQualifiers = new ArrayList<>(ids.size());
+
+        // TODO: for single-column IDs, a better qualifier would be "IN (..)" instead of "OR"
+        for (AgObjectId id : ids) {
+            idQualifiers.add(qualifierMaker.apply(id));
+        }
+
+        return ObjectSelect.query(entity.getType()).where(ExpressionFactory.or(idQualifiers));
+    }
+
+    private Function<AgObjectId, Expression> idQualifierMaker(AgEntity<?> entity) {
+        int idSize = entity.getIdParts().size();
+
+        if (idSize == 1) {
+            String partName = entity.getIdParts().iterator().next().getName();
+            ASTPath idPath = pathResolver.resolve(entity.getName(), partName).getPathExp();
+            return id -> ExpressionFactory.matchExp(idPath, id.get(partName));
+        } else {
+
+            List<String> partNames = new ArrayList<>(idSize);
+            List<ASTPath> paths = new ArrayList<>(idSize);
+            for (AgIdPart idPart : entity.getIdParts()) {
+                partNames.add(idPart.getName());
+                paths.add(pathResolver.resolve(entity.getName(), idPart.getName()).getPathExp());
+            }
+
+            return id -> {
+                List<Expression> idQualifier = new ArrayList<>(idSize);
+                for (int i = 0; i < idSize; i++) {
+                    idQualifier.add(ExpressionFactory.matchExp(paths.get(i), id.get(partNames.get(i))));
+                }
+
+                return ExpressionFactory.and(idQualifier);
+            };
+        }
+    }
+
     private ObjRelationship findRelationship(ObjEntity entity, String name) {
 
         // Take inheritance into account... ObjRelationship may not be present in the superclass, but the underlying

diff --git a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/CayenneUtil.java b/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/CayenneUtil.java
@@ -81,6 +81,7 @@ public static ObjectId toObjectId(
     }
 
 
+    // TODO: this logic is somewhat duplicated in CayenneQueryAssembler.createQueryForIds. Maybe it belongs there to begin with?
     public static <A> A findById(
             IPathResolver pathResolver,
             ObjectContext context,

diff --git a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/ICayenneQueryAssembler.java b/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/ICayenneQueryAssembler.java
@@ -1,11 +1,15 @@
 package io.agrest.cayenne.processor;
 
 import io.agrest.RelatedResourceEntity;
+import io.agrest.id.AgObjectId;
+import io.agrest.meta.AgEntity;
 import io.agrest.runtime.processor.select.SelectContext;
 import org.apache.cayenne.exp.property.Property;
 import org.apache.cayenne.query.ColumnSelect;
 import org.apache.cayenne.query.ObjectSelect;
 
+import java.util.Collection;
+
 /**
  * @since 3.7
  */
@@ -21,5 +25,10 @@ <T, P> ColumnSelect<Object[]> createQueryWithParentIdsQualifier(
 
     <T> Property<?>[] queryColumns(RelatedResourceEntity<T> entity);
 
+    /**
+     * @since 5.0
+     */
+    <T> ObjectSelect<T> createQueryForIds(AgEntity<T> entity, Collection<AgObjectId> ids);
+
 }
 
diff --git a/.../src/main/java/io/agrest/cayenne/processor/delete/stage/CayenneDeleteMapChangesStage.java b/.../src/main/java/io/agrest/cayenne/processor/delete/stage/CayenneDeleteMapChangesStage.java
@@ -1,12 +1,12 @@
 package io.agrest.cayenne.processor.delete.stage;
 
 import io.agrest.AgException;
-import io.agrest.id.AgObjectId;
-import io.agrest.runtime.EntityParent;
 import io.agrest.cayenne.path.IPathResolver;
 import io.agrest.cayenne.processor.CayenneUtil;
+import io.agrest.cayenne.processor.ICayenneQueryAssembler;
 import io.agrest.meta.AgEntity;
 import io.agrest.processor.ProcessorOutcome;
+import io.agrest.runtime.EntityParent;
 import io.agrest.runtime.processor.delete.DeleteContext;
 import io.agrest.runtime.processor.delete.stage.DeleteMapChangesStage;
 import io.agrest.runtime.processor.update.ChangeOperation;
@@ -19,6 +19,7 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.stream.Collectors;
 
 /**
  * A processor for the {@link io.agrest.DeleteStage#MAP_CHANGES} stage that associates persistent objects with delete
@@ -29,9 +30,13 @@
 public class CayenneDeleteMapChangesStage extends DeleteMapChangesStage {
 
     private final IPathResolver pathResolver;
+    private final ICayenneQueryAssembler queryAssembler;
 
-    public CayenneDeleteMapChangesStage(@Inject IPathResolver pathResolver) {
+    public CayenneDeleteMapChangesStage(
+            @Inject IPathResolver pathResolver,
+            @Inject ICayenneQueryAssembler queryAssembler) {
         this.pathResolver = pathResolver;
+        this.queryAssembler = queryAssembler;
     }
 
     @Override
@@ -56,8 +61,8 @@ protected <T extends DataObject> void mapDeleteOperations(DeleteContext<T> conte
 
     protected <T extends DataObject> List<T> findObjectsToDelete(DeleteContext<T> context) {
 
-        if (context.isById()) {
-            return findById(context);
+        if (context.isByIds()) {
+            return findByIds(context);
         } else if (context.getParent() != null) {
             return findByParent(context, context.getParent());
         }
@@ -67,23 +72,20 @@ protected <T extends DataObject> List<T> findObjectsToDelete(DeleteContext<T> co
         }
     }
 
-    protected <T extends DataObject> List<T> findById(DeleteContext<T> context) {
+    protected <T extends DataObject> List<T> findByIds(DeleteContext<T> context) {
 
-        List<T> objects = new ArrayList<>(context.getIds().size());
         ObjectContext cayenneContext = CayenneDeleteStartStage.cayenneContext(context);
+        List<T> objects = queryAssembler.createQueryForIds(context.getAgEntity(), context.getIds()).select(cayenneContext);
 
-        for (AgObjectId id : context.getIds()) {
-
-            // TODO: batch objects retrieval into a single query
-
-            T o = CayenneUtil.findById(pathResolver, cayenneContext, context.getAgEntity(), id);
+        // DELETE is idempotent, so if some objects are missing, we should proceed ...
+        // Also, only return 404 if zero objects matched
 
-            if (o == null) {
-                ObjEntity entity = cayenneContext.getEntityResolver().getObjEntity(context.getType());
-                throw AgException.notFound("No object for ID '%s' and entity '%s'", id, entity.getName());
-            }
+        if (objects.isEmpty()) {
+            ObjEntity entity = cayenneContext.getEntityResolver().getObjEntity(context.getType());
 
-            objects.add(o);
+            String idsString = context.getIds().stream().map(id -> id.toString()).collect(Collectors.joining(","));
+            throw AgException.notFound("No matching objects for entity '%s' and ids: %s",
+                    entity.getName(), idsString);
         }
 
         return objects;

diff --git a/agrest-cayenne/src/test/java/io/agrest/cayenne/DELETE/BasicIT.java b/agrest-cayenne/src/test/java/io/agrest/cayenne/DELETE/BasicIT.java
@@ -110,7 +110,7 @@ public void deleteById_BadId() {
         tester.target("/e4/7")
                 .delete()
                 .wasNotFound()
-                .bodyEquals("{\"message\":\"No object for ID '7' and entity 'E4'\"}");
+                .bodyEquals("{\"message\":\"No matching objects for entity 'E4' and ids: 7\"}");
 
         tester.e4().matcher().assertMatches(1);
     }
@@ -130,7 +130,7 @@ public void deleteTwice() {
         tester.target("/e4/8")
                 .delete()
                 .wasNotFound()
-                .bodyEquals("{\"message\":\"No object for ID '8' and entity 'E4'\"}");
+                .bodyEquals("{\"message\":\"No matching objects for entity 'E4' and ids: 8\"}");
     }
 
     @Test

diff --git a/agrest-engine/src/main/java/io/agrest/runtime/processor/delete/DeleteContext.java b/agrest-engine/src/main/java/io/agrest/runtime/processor/delete/DeleteContext.java
@@ -30,10 +30,21 @@ public DeleteContext(Class<T> type, RequestSchema schema, Injector injector) {
         this.deleteOperations = Collections.emptyList();
     }
 
-    public boolean isById() {
+    /**
+     * @since 5.0
+     */
+    public boolean isByIds() {
         return ids != null && !ids.isEmpty();
     }
 
+    /**
+     * @deprecated in favor of {@link #isByIds()}
+     */
+    @Deprecated(since = "5.0", forRemoval = true)
+    public boolean isById() {
+        return isByIds();
+    }
+
     public Collection<AgObjectId> getIds() {
         return ids != null ? ids : Collections.emptyList();
     }