From f586542d7ed6cf50926778578f40cc9e13ade8d0 Mon Sep 17 00:00:00 2001 From: Andrus Adamchik Date: Wed, 2 Nov 2022 09:37:01 +0100 Subject: [PATCH 1/4] Unifying the naming of custom properties in ResourceEntity and ProcessingContext --- .../cayenne/processor/CayenneProcessor.java | 14 ++++---- .../delete/stage/CayenneDeleteStartStage.java | 4 +-- .../stage/CayenneUnrelateStartStage.java | 4 +-- .../update/stage/CayenneUpdateStartStage.java | 4 +-- .../main/java/io/agrest/ResourceEntity.java | 34 ++++++++++++++----- .../processor/BaseProcessingContext.java | 4 +-- .../agrest/processor/ProcessingContext.java | 27 ++++++++++++--- 7 files changed, 63 insertions(+), 28 deletions(-) diff --git a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/CayenneProcessor.java b/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/CayenneProcessor.java index 3508fe76b..164d28427 100644 --- a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/CayenneProcessor.java +++ b/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/CayenneProcessor.java @@ -17,14 +17,14 @@ public class CayenneProcessor { */ public static CayenneResourceEntityExt getEntity(ResourceEntity entity) { String key = (entity instanceof RootResourceEntity) ? CAYENNE_ROOT_ENTITY_KEY : CAYENNE_RELATED_ENTITY_KEY; - return entity.getRequestProperty(key); + return entity.getProperty(key); } /** * @since 5.0 */ public static CayenneRootResourceEntityExt getRootEntity(RootResourceEntity entity) { - return entity.getRequestProperty(CAYENNE_ROOT_ENTITY_KEY); + return entity.getProperty(CAYENNE_ROOT_ENTITY_KEY); } /** @@ -38,11 +38,11 @@ public static CayenneRootResourceEntityExt getOrCreateRootEntity(RootReso } CayenneRootResourceEntityExt newExt = new CayenneRootResourceEntityExt<>(); - entity.setRequestProperty(CAYENNE_ROOT_ENTITY_KEY, newExt); + entity.setProperty(CAYENNE_ROOT_ENTITY_KEY, newExt); // copy MapBy owner's query to MapBy to ensure its own resolvers work properly if (entity.getMapBy() != null) { - entity.getMapBy().setRequestProperty(CAYENNE_ROOT_ENTITY_KEY, newExt); + entity.getMapBy().setProperty(CAYENNE_ROOT_ENTITY_KEY, newExt); } return newExt; @@ -52,7 +52,7 @@ public static CayenneRootResourceEntityExt getOrCreateRootEntity(RootReso * @since 5.0 */ public static CayenneRelatedResourceEntityExt getRelatedEntity(RelatedResourceEntity entity) { - return entity.getRequestProperty(CAYENNE_RELATED_ENTITY_KEY); + return entity.getProperty(CAYENNE_RELATED_ENTITY_KEY); } /** @@ -66,11 +66,11 @@ public static CayenneRelatedResourceEntityExt getOrCreateRelatedEntity(Relat } CayenneRelatedResourceEntityExt newExt = new CayenneRelatedResourceEntityExt(); - entity.setRequestProperty(CAYENNE_RELATED_ENTITY_KEY, newExt); + entity.setProperty(CAYENNE_RELATED_ENTITY_KEY, newExt); // copy MapBy owner's query to MapBy to ensure its own resolvers work properly if (entity.getMapBy() != null) { - entity.getMapBy().setRequestProperty(CAYENNE_RELATED_ENTITY_KEY, newExt); + entity.getMapBy().setProperty(CAYENNE_RELATED_ENTITY_KEY, newExt); } return newExt; diff --git a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/delete/stage/CayenneDeleteStartStage.java b/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/delete/stage/CayenneDeleteStartStage.java index 7c68905a6..344d31337 100644 --- a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/delete/stage/CayenneDeleteStartStage.java +++ b/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/delete/stage/CayenneDeleteStartStage.java @@ -21,7 +21,7 @@ public class CayenneDeleteStartStage extends DeleteStartStage { * by this stage. */ public static ObjectContext cayenneContext(ProcessingContext context) { - return (ObjectContext) context.getAttribute(CayenneDeleteStartStage.DELETE_OBJECT_CONTEXT_ATTRIBUTE); + return (ObjectContext) context.getProperty(CayenneDeleteStartStage.DELETE_OBJECT_CONTEXT_ATTRIBUTE); } private final ICayennePersister persister; @@ -42,7 +42,7 @@ public ProcessorOutcome execute(DeleteContext context) { } protected void initCayenneContext(DeleteContext context) { - context.setAttribute(DELETE_OBJECT_CONTEXT_ATTRIBUTE, persister.newContext()); + context.setProperty(DELETE_OBJECT_CONTEXT_ATTRIBUTE, persister.newContext()); } } diff --git a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/unrelate/stage/CayenneUnrelateStartStage.java b/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/unrelate/stage/CayenneUnrelateStartStage.java index c734156a1..fff25e1e7 100644 --- a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/unrelate/stage/CayenneUnrelateStartStage.java +++ b/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/unrelate/stage/CayenneUnrelateStartStage.java @@ -20,7 +20,7 @@ public class CayenneUnrelateStartStage extends UnrelateStartStage { * by this stage. */ public static ObjectContext cayenneContext(ProcessingContext context) { - return (ObjectContext) context.getAttribute(CayenneUnrelateStartStage.UNRELATE_OBJECT_CONTEXT_ATTRIBUTE); + return (ObjectContext) context.getProperty(CayenneUnrelateStartStage.UNRELATE_OBJECT_CONTEXT_ATTRIBUTE); } private final ICayennePersister persister; @@ -31,7 +31,7 @@ public CayenneUnrelateStartStage(@Inject ICayennePersister persister) { @Override public ProcessorOutcome execute(UnrelateContext context) { - context.setAttribute(UNRELATE_OBJECT_CONTEXT_ATTRIBUTE, persister.newContext()); + context.setProperty(UNRELATE_OBJECT_CONTEXT_ATTRIBUTE, persister.newContext()); return ProcessorOutcome.CONTINUE; } } diff --git a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/update/stage/CayenneUpdateStartStage.java b/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/update/stage/CayenneUpdateStartStage.java index 9eb853525..90488c4b3 100644 --- a/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/update/stage/CayenneUpdateStartStage.java +++ b/agrest-cayenne/src/main/java/io/agrest/cayenne/processor/update/stage/CayenneUpdateStartStage.java @@ -20,7 +20,7 @@ public class CayenneUpdateStartStage extends UpdateStartStage { * by this stage. */ public static ObjectContext cayenneContext(ProcessingContext context) { - return (ObjectContext) context.getAttribute(CayenneUpdateStartStage.UPDATE_OBJECT_CONTEXT_ATTRIBUTE); + return (ObjectContext) context.getProperty(CayenneUpdateStartStage.UPDATE_OBJECT_CONTEXT_ATTRIBUTE); } private ICayennePersister persister; @@ -31,7 +31,7 @@ public CayenneUpdateStartStage(@Inject ICayennePersister persister) { @Override public ProcessorOutcome execute(UpdateContext context) { - context.setAttribute(UPDATE_OBJECT_CONTEXT_ATTRIBUTE, persister.newContext()); + context.setProperty(UPDATE_OBJECT_CONTEXT_ATTRIBUTE, persister.newContext()); return ProcessorOutcome.CONTINUE; } } diff --git a/agrest-engine/src/main/java/io/agrest/ResourceEntity.java b/agrest-engine/src/main/java/io/agrest/ResourceEntity.java index edc15aaf7..b724afea7 100644 --- a/agrest-engine/src/main/java/io/agrest/ResourceEntity.java +++ b/agrest-engine/src/main/java/io/agrest/ResourceEntity.java @@ -27,7 +27,7 @@ public abstract class ResourceEntity { private final Map attributes; private final Set defaultAttributes; private final Map> children; - private final Map requestProperties; + private final Map properties; private String mapByPath; private ResourceEntity mapBy; @@ -45,7 +45,7 @@ public ResourceEntity(AgEntity agEntity) { this.defaultAttributes = new HashSet<>(); this.children = new HashMap<>(); this.orderings = new ArrayList<>(2); - this.requestProperties = new HashMap<>(5); + this.properties = new HashMap<>(5); } /** @@ -269,23 +269,39 @@ public boolean isFiltered() { } /** - * Returns a previously stored object for a given name. Request properties mechanism allows pluggable processing + * Returns a previously stored custom object for a given name. The properties mechanism allows pluggable processing * pipelines to store and exchange data within a given request. * - * @since 3.7 + * @since 5.0 */ - public

P getRequestProperty(String name) { - return (P) requestProperties.get(name); + public

P getProperty(String name) { + return (P) properties.get(name); } /** - * Sets a property value for a given name. Request properties mechanism allows pluggable processing pipelines to + * Sets a property value for a given name. The properties mechanism allows pluggable processing pipelines to * store and exchange data within a given request. * - * @since 3.7 + * @since 5.0 + */ + public void setProperty(String name, Object value) { + properties.put(name, value); + } + + /** + * @deprecated in favor of {@link #getProperty(String)} + */ + @Deprecated(since = "5.0") + public

P getRequestProperty(String name) { + return getProperty(name); + } + + /** + * @deprecated in favor of {@link #setProperty(String, Object)} */ + @Deprecated(since = "5.0") public void setRequestProperty(String name, Object value) { - requestProperties.put(name, value); + setProperty(name, value); } /** diff --git a/agrest-engine/src/main/java/io/agrest/processor/BaseProcessingContext.java b/agrest-engine/src/main/java/io/agrest/processor/BaseProcessingContext.java index 69038f617..6cfff11ae 100644 --- a/agrest-engine/src/main/java/io/agrest/processor/BaseProcessingContext.java +++ b/agrest-engine/src/main/java/io/agrest/processor/BaseProcessingContext.java @@ -58,12 +58,12 @@ public Class getType() { } @Override - public Object getAttribute(String name) { + public Object getProperty(String name) { return attributes != null ? attributes.get(name) : null; } @Override - public void setAttribute(String name, Object value) { + public void setProperty(String name, Object value) { // Presumably BaseProcessingContext is single-threaded (one per request), so lazy init and using HashMap is ok. if (attributes == null) { diff --git a/agrest-engine/src/main/java/io/agrest/processor/ProcessingContext.java b/agrest-engine/src/main/java/io/agrest/processor/ProcessingContext.java index 301e4b239..95ce81cde 100644 --- a/agrest-engine/src/main/java/io/agrest/processor/ProcessingContext.java +++ b/agrest-engine/src/main/java/io/agrest/processor/ProcessingContext.java @@ -12,15 +12,34 @@ public interface ProcessingContext { Class getType(); /** - * Returns a previously stored context attribute or null if none was set for - * a given key. + * Returns a previously stored context attribute or null if none was set for a given key. + * + * @since 5.0 */ - Object getAttribute(String name); + Object getProperty(String name); /** * Allows to store an arbitrary attribute in the context during processing. + * + * @since 5.0 + */ + void setProperty(String name, Object value); + + /** + * @deprecated in favor of {@link #getProperty(String)} + */ + @Deprecated(since = "5.0") + default Object getAttribute(String name) { + return getProperty(name); + } + + /** + * @deprecated in favor of {@link #setProperty(String, Object)} */ - void setAttribute(String name, Object value); + @Deprecated(since = "5.0") + default void setAttribute(String name, Object value) { + setProperty(name, value); + } /** * Provides access to a desired service from the Agrest stack that created the current context. From dedd7a2de85476e0866256ad4806d59692e13667 Mon Sep 17 00:00:00 2001 From: Andrus Adamchik Date: Wed, 2 Nov 2022 09:38:41 +0100 Subject: [PATCH 2/4] cleanup --- .../main/java/io/agrest/processor/BaseProcessingContext.java | 4 ++-- .../src/main/java/io/agrest/processor/ProcessingContext.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/agrest-engine/src/main/java/io/agrest/processor/BaseProcessingContext.java b/agrest-engine/src/main/java/io/agrest/processor/BaseProcessingContext.java index 6cfff11ae..4f216bf95 100644 --- a/agrest-engine/src/main/java/io/agrest/processor/BaseProcessingContext.java +++ b/agrest-engine/src/main/java/io/agrest/processor/BaseProcessingContext.java @@ -28,7 +28,7 @@ public BaseProcessingContext(Class type, Injector injector) { * @since 5.0 */ @Override - public T service(Class type) { + public S service(Class type) { return injector.getInstance(type); } @@ -38,7 +38,7 @@ public T service(Class type) { * @since 5.0 */ @Override - public T service(Key key) { + public S service(Key key) { return injector.getInstance(key); } diff --git a/agrest-engine/src/main/java/io/agrest/processor/ProcessingContext.java b/agrest-engine/src/main/java/io/agrest/processor/ProcessingContext.java index 95ce81cde..ba31d288d 100644 --- a/agrest-engine/src/main/java/io/agrest/processor/ProcessingContext.java +++ b/agrest-engine/src/main/java/io/agrest/processor/ProcessingContext.java @@ -46,12 +46,12 @@ default void setAttribute(String name, Object value) { * * @since 5.0 */ - T service(Class type); + S service(Class type); /** * Provides access to a desired service from the Agrest stack that created the current context. * * @since 5.0 */ - T service(Key key); + S service(Key key); } From 6aacd98dd19aaf8ff222c468ed5bb43cff39fc60 Mon Sep 17 00:00:00 2001 From: Andrus Adamchik Date: Wed, 2 Nov 2022 10:10:55 +0100 Subject: [PATCH 3/4] refactoring - stop passing "mapByPath" around this simplifies the ResourceEntity code at the expense of a bit more obscure error reporting --- .../main/java/io/agrest/ResourceEntity.java | 15 ++++++++----- .../java/io/agrest/encoder/MapByEncoder.java | 8 ++----- .../runtime/encoder/DataEncoderFactory.java | 22 ++++++++----------- .../io/agrest/runtime/entity/MapByMerger.java | 2 +- 4 files changed, 21 insertions(+), 26 deletions(-) diff --git a/agrest-engine/src/main/java/io/agrest/ResourceEntity.java b/agrest-engine/src/main/java/io/agrest/ResourceEntity.java index b724afea7..d2c11bedd 100644 --- a/agrest-engine/src/main/java/io/agrest/ResourceEntity.java +++ b/agrest-engine/src/main/java/io/agrest/ResourceEntity.java @@ -29,7 +29,6 @@ public abstract class ResourceEntity { private final Map> children; private final Map properties; - private String mapByPath; private ResourceEntity mapBy; private final List orderings; private Exp exp; @@ -185,16 +184,20 @@ public ResourceEntity getMapBy() { } /** + * @deprecated in favor of {@link #mapBy(ResourceEntity)}. mapByPath parameter is ignored. * @since 1.1 */ + @Deprecated(since = "5.0") public ResourceEntity mapBy(ResourceEntity mapBy, String mapByPath) { - this.mapByPath = mapByPath; - this.mapBy = mapBy; - return this; + return mapBy(mapBy); } - public String getMapByPath() { - return mapByPath; + /** + * @since 5.0 + */ + public ResourceEntity mapBy(ResourceEntity mapBy) { + this.mapBy = mapBy; + return this; } public Class getType() { diff --git a/agrest-engine/src/main/java/io/agrest/encoder/MapByEncoder.java b/agrest-engine/src/main/java/io/agrest/encoder/MapByEncoder.java index 985e93544..110432842 100644 --- a/agrest-engine/src/main/java/io/agrest/encoder/MapByEncoder.java +++ b/agrest-engine/src/main/java/io/agrest/encoder/MapByEncoder.java @@ -11,20 +11,17 @@ public class MapByEncoder implements Encoder { - private final String mapByPath; private final List mapByReaders; private final Encoder collectionEncoder; private final boolean byId; private final ValueStringConverter fieldNameConverter; public MapByEncoder( - String mapByPath, List mapByReaders, Encoder collectionEncoder, boolean byId, ValueStringConverter fieldNameConverter) { - this.mapByPath = mapByPath; this.mapByReaders = mapByReaders; this.collectionEncoder = collectionEncoder; this.byId = byId; @@ -88,10 +85,9 @@ private Map> mapBy(List objects) { } // disallow nulls as JSON keys... - // note that converter below will throw an NPE if we pass NULL - // further down... the error here has more context. + // note that converter below will throw an NPE if we pass "null" further down. The error here has more context. if (key == null) { - throw AgException.internalServerError("Null mapBy value for key '%s'", mapByPath); + throw AgException.internalServerError("Null mapBy value for object '%s'", o); } String keyString = fieldNameConverter.asString(key); diff --git a/agrest-engine/src/main/java/io/agrest/runtime/encoder/DataEncoderFactory.java b/agrest-engine/src/main/java/io/agrest/runtime/encoder/DataEncoderFactory.java index f328776ce..dc913e82f 100644 --- a/agrest-engine/src/main/java/io/agrest/runtime/encoder/DataEncoderFactory.java +++ b/agrest-engine/src/main/java/io/agrest/runtime/encoder/DataEncoderFactory.java @@ -131,22 +131,21 @@ protected Map propertyEncoders(ResourceEntity enti } protected MapByEncoder mapByEncoder(ResourceEntity entity, Encoder encoder, ProcessingContext context) { - return mapByEncoder(entity.getMapBy(), new ArrayList<>(), encoder, entity.getMapByPath(), context); + return mapByEncoder(entity.getMapBy(), new ArrayList<>(), encoder, context); } protected MapByEncoder mapByEncoder( ResourceEntity mapBy, List readerChain, Encoder encoder, - String mapByPath, ProcessingContext context) { // map by id if (mapBy.isIdIncluded()) { - validateLeafMapBy(mapBy, mapByPath); + validateLeafMapBy(mapBy); readerChain.add(mapBy.getAgEntity().getIdReader()); - return new MapByEncoder(mapByPath, + return new MapByEncoder( readerChain, encoder, true, @@ -155,11 +154,11 @@ protected MapByEncoder mapByEncoder( // map by property if (!mapBy.getAttributes().isEmpty()) { - validateLeafMapBy(mapBy, mapByPath); + validateLeafMapBy(mapBy); Map.Entry attribute = mapBy.getAttributes().entrySet().iterator().next(); readerChain.add(propertyFactory.getAttributeProperty(mapBy, attribute.getValue()).getReader()); - return new MapByEncoder(mapByPath, + return new MapByEncoder( readerChain, encoder, false, @@ -176,20 +175,20 @@ protected MapByEncoder mapByEncoder( AgRelationship relationship = mapBy.getChild(child.getKey()).getIncoming(); readerChain.add(propertyFactory.getRelationshipProperty(mapBy, relationship, null, context).getReader()); - return mapByEncoder(mapBy.getChildren().get(child.getKey()), readerChain, encoder, mapByPath, context); + return mapByEncoder(mapBy.getChildren().get(child.getKey()), readerChain, encoder, context); } // map by relationship (implicitly by id) readerChain.add(mapBy.getAgEntity().getIdReader()); - return new MapByEncoder(mapByPath, + return new MapByEncoder( readerChain, encoder, true, converters.getConverter(Object.class)); } - protected void validateLeafMapBy(ResourceEntity mapBy, String mapByPath) { + protected void validateLeafMapBy(ResourceEntity mapBy) { if (!mapBy.getChildren().isEmpty()) { @@ -197,10 +196,7 @@ protected void validateLeafMapBy(ResourceEntity mapBy, String mapByPath) { ? ((RelatedResourceEntity) mapBy).getIncoming().getName() : ""; - throw AgException.badRequest( - "'mapBy' path segment '%s' should not have children. Full 'mapBy' path: %s", - pathSegment, - mapByPath); + throw AgException.badRequest("'mapBy' path segment '%s' should not have children", pathSegment); } } } diff --git a/agrest-engine/src/main/java/io/agrest/runtime/entity/MapByMerger.java b/agrest-engine/src/main/java/io/agrest/runtime/entity/MapByMerger.java index e025b30f5..08bbc0521 100644 --- a/agrest-engine/src/main/java/io/agrest/runtime/entity/MapByMerger.java +++ b/agrest-engine/src/main/java/io/agrest/runtime/entity/MapByMerger.java @@ -47,7 +47,7 @@ public void merge(ResourceEntity entity, String mapByPath, Map, : mapByCompanionEntity((RootResourceEntity) entity); new ResourceEntityTreeBuilder(mapByCompanionEntity, schema, overlays).inflatePath(mapByPath); - entity.mapBy(mapByCompanionEntity, mapByPath); + entity.mapBy(mapByCompanionEntity); } protected RootResourceEntity mapByCompanionEntity(RootResourceEntity entity) { From 75a221d582c60369af57d1de6b8764cd0e797ffa Mon Sep 17 00:00:00 2001 From: Andrus Adamchik Date: Wed, 2 Nov 2022 11:03:48 +0100 Subject: [PATCH 4/4] cleanup --- .../main/java/io/agrest/ResourceEntity.java | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/agrest-engine/src/main/java/io/agrest/ResourceEntity.java b/agrest-engine/src/main/java/io/agrest/ResourceEntity.java index d2c11bedd..51166e51f 100644 --- a/agrest-engine/src/main/java/io/agrest/ResourceEntity.java +++ b/agrest-engine/src/main/java/io/agrest/ResourceEntity.java @@ -69,9 +69,9 @@ public Exp getExp() { } /** - * @deprecated since 5.0 in favor of {@link #getExp()} + * @deprecated in favor of {@link #getExp()} */ - @Deprecated + @Deprecated(since = "5.0") public Exp getQualifier() { return getExp(); } @@ -84,9 +84,9 @@ public void andExp(Exp exp) { } /** - * @deprecated since 5.0 in favor of {@link #andExp(Exp)} + * @deprecated in favor of {@link #andExp(Exp)} */ - @Deprecated + @Deprecated(since = "5.0") public void andQualifier(Exp qualifier) { andExp(qualifier); } @@ -212,9 +212,9 @@ public int getStart() { } /** - * @deprecated since 5.0 in favor of {@link #getStart()} + * @deprecated in favor of {@link #getStart()} */ - @Deprecated + @Deprecated(since = "5.0") public int getFetchOffset() { return getStart(); } @@ -227,9 +227,9 @@ public void setStart(int start) { } /** - * @deprecated since 5.0 in favor of {@link #setStart(int)} + * @deprecated in favor of {@link #setStart(int)} */ - @Deprecated + @Deprecated(since = "5.0") public void setFetchOffset(int fetchOffset) { setStart(fetchOffset); } @@ -242,9 +242,9 @@ public int getLimit() { } /** - * @deprecated since 5.0 in favor of {@link #getLimit()} + * @deprecated in favor of {@link #getLimit()} */ - @Deprecated + @Deprecated(since = "5.0") public int getFetchLimit() { return getLimit(); } @@ -257,9 +257,9 @@ public void setLimit(int limit) { } /** - * @deprecated since 5.0 in favor of {@link #setLimit(int)} + * @deprecated in favor of {@link #setLimit(int)} */ - @Deprecated + @Deprecated(since = "5.0") public void setFetchLimit(int fetchLimit) { setLimit(fetchLimit); }