diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index 6afea5d3f..8d486d6e3 100644 --- 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 index 4bf181c49..cdf9cbea2 100644 --- 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 Property[] queryColumns(RelatedResourceEntity entity) { return columns; } + @Override + public ObjectSelect createQueryForIds(AgEntity entity, Collection 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 qualifierMaker = idQualifierMaker(entity); + List 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 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 partNames = new ArrayList<>(idSize); + List 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 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 index 7f5965453..3a0896ef1 100644 --- 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 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 index 958367053..c7fcafa84 100644 --- 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 @@ ColumnSelect createQueryWithParentIdsQualifier( Property[] queryColumns(RelatedResourceEntity entity); + /** + * @since 5.0 + */ + ObjectSelect createQueryForIds(AgEntity entity, Collection ids); + } diff --git a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/delete/stage/CayenneDeleteMapChangesStage.java b/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/delete/stage/CayenneDeleteMapChangesStage.java index 612a8cf62..5345be3c7 100644 --- a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/delete/stage/CayenneDeleteMapChangesStage.java +++ b/agrest-cayenne/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 void mapDeleteOperations(DeleteContext conte protected List findObjectsToDelete(DeleteContext 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 List findObjectsToDelete(DeleteContext co } } - protected List findById(DeleteContext context) { + protected List findByIds(DeleteContext context) { - List objects = new ArrayList<>(context.getIds().size()); ObjectContext cayenneContext = CayenneDeleteStartStage.cayenneContext(context); + List 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 index 63138d8ab..8c63ba4e9 100644 --- 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 index 927ac35d2..24675ad2e 100644 --- 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 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 getIds() { return ids != null ? ids : Collections.emptyList(); }