From 2e224e8c00145c1a84c006ee66c7b3489f261781 Mon Sep 17 00:00:00 2001 From: Brendan Robert Date: Tue, 30 Mar 2021 09:59:16 -0500 Subject: [PATCH] Simplifying primary type and resource type handling rules --- .../models/persistor/ModelPersistor.java | 2 +- .../persistor/impl/ModelPersistorImpl.java | 19 +++++---- .../persistor/impl/ResourceTypeKey.java | 42 ++++++++++++------- .../models/persist/ModelPersistTest.java | 4 +- .../persist/impl/ResourceTypeKeyTest.java | 15 ++++--- 5 files changed, 53 insertions(+), 29 deletions(-) diff --git a/SlingModelPersist/src/main/java/org/apache/sling/models/persistor/ModelPersistor.java b/SlingModelPersist/src/main/java/org/apache/sling/models/persistor/ModelPersistor.java index 7c5c9f463..4bba71f98 100644 --- a/SlingModelPersist/src/main/java/org/apache/sling/models/persistor/ModelPersistor.java +++ b/SlingModelPersist/src/main/java/org/apache/sling/models/persistor/ModelPersistor.java @@ -18,10 +18,10 @@ */ package org.apache.sling.models.persistor; +import com.drew.lang.annotations.NotNull; import javax.jcr.RepositoryException; import org.apache.sling.api.resource.PersistenceException; import org.apache.sling.api.resource.ResourceResolver; -import org.jetbrains.annotations.NotNull; /** * Definition of Model Persistor service diff --git a/SlingModelPersist/src/main/java/org/apache/sling/models/persistor/impl/ModelPersistorImpl.java b/SlingModelPersist/src/main/java/org/apache/sling/models/persistor/impl/ModelPersistorImpl.java index 8d2d9ffab..a921ce6e3 100644 --- a/SlingModelPersist/src/main/java/org/apache/sling/models/persistor/impl/ModelPersistorImpl.java +++ b/SlingModelPersist/src/main/java/org/apache/sling/models/persistor/impl/ModelPersistorImpl.java @@ -18,6 +18,7 @@ */ package org.apache.sling.models.persistor.impl; +import com.drew.lang.annotations.NotNull; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -42,7 +43,6 @@ import org.apache.sling.models.persistor.ModelPersistor; import org.apache.sling.models.persistor.annotations.DirectDescendants; import org.apache.sling.models.persistor.impl.util.ReflectionUtils; -import org.jetbrains.annotations.NotNull; import org.osgi.service.component.annotations.Component; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -112,12 +112,14 @@ public void persist(final @NotNull String nodePath, final @NotNull Object instan final ResourceTypeKey resourceType = ResourceTypeKey.fromObject(instance); // let's create the resource first - LOGGER.debug("Creating node at: {} of type: {}", nodePath, resourceType.primaryType); + LOGGER.debug("Creating node at: {} of type: {}", nodePath, resourceType.nodeType); boolean isUpdate = resourceResolver.getResource(nodePath) != null; - resource = ResourceUtil.getOrCreateResource(resourceResolver, nodePath, resourceType.primaryType, NT_UNSTRUCTURED, true); - if (StringUtils.isNotEmpty(resourceType.childType)) { - LOGGER.debug("Needs a child node, creating node at: {} of type: {}", nodePath, resourceType.childType); - resource = ResourceUtil.getOrCreateResource(resourceResolver, nodePath + "/" + JCR_CONTENT, resourceType.childType, NT_UNSTRUCTURED, true); + Map properties = new HashMap<>(); + properties.put("jcr:primaryType", resourceType.nodeType); + + resource = ResourceUtil.getOrCreateResource(resourceResolver, nodePath, properties, NT_UNSTRUCTURED, true); + if (resourceType.resourceType != null) { + resource.adaptTo(ModifiableValueMap.class).put("sling:resourceType", resourceType.resourceType); } if (ReflectionUtils.isArrayOrCollection(instance)) { @@ -132,6 +134,7 @@ public void persist(final @NotNull String nodePath, final @NotNull Object instan fields.stream() .filter(field -> ReflectionUtils.isNotTransient(field, isUpdate)) .filter(field -> ReflectionUtils.isSupportedType(field) || ReflectionUtils.isCollectionOfPrimitiveType(field)) + // TODO: Be smarter about transient fields declared with Lombok .filter(f -> ReflectionUtils.hasNoTransientGetter(f.getName(), instance.getClass())) .forEach(field -> persistField(r, instance, field, deepPersist)); } @@ -160,7 +163,9 @@ private void persistField(@NotNull Resource resource, @NotNull Object instance, Object value = ReflectionUtils.getStorableValue(field.get(instance)); // remove the attribute that is null, or remove in case it changes type - values.remove(fieldName); + if (!fieldName.equals("jcr:primaryType")) { + values.remove(fieldName); + } if (value != null) { values.put(fieldName, value); } diff --git a/SlingModelPersist/src/main/java/org/apache/sling/models/persistor/impl/ResourceTypeKey.java b/SlingModelPersist/src/main/java/org/apache/sling/models/persistor/impl/ResourceTypeKey.java index 108038b55..04c093052 100644 --- a/SlingModelPersist/src/main/java/org/apache/sling/models/persistor/impl/ResourceTypeKey.java +++ b/SlingModelPersist/src/main/java/org/apache/sling/models/persistor/impl/ResourceTypeKey.java @@ -17,10 +17,12 @@ * under the License. */package org.apache.sling.models.persistor.impl; +import java.lang.reflect.Field; import java.util.HashMap; import java.util.Map; +import javax.inject.Named; +import org.apache.commons.lang3.reflect.FieldUtils; import org.apache.sling.models.annotations.Model; -import org.apache.sling.models.persistor.annotations.ChildType; import org.apache.sling.models.persistor.annotations.ResourceType; import static org.apache.sling.models.persistor.impl.util.ReflectionUtils.getAnnotatedValue; @@ -33,15 +35,15 @@ public class ResourceTypeKey { static final Map, ResourceTypeKey> NODE_TYPE_FOR_CLASS = new HashMap<>(); static public final ResourceTypeKey NT_UNSTRUCTURED = new ResourceTypeKey("nt:unstructured", null); - final public String primaryType; - final public String childType; + final public String nodeType; + final public String resourceType; - public ResourceTypeKey(String primaryType, String childType) { - this.primaryType = primaryType; - this.childType = childType; + public ResourceTypeKey(String nodeType, String resourceType) { + this.nodeType = nodeType != null ? nodeType : "nt:unstructured"; + this.resourceType = resourceType; } - public static ResourceTypeKey fromObject(Object obj) { + public static ResourceTypeKey fromObject(Object obj) throws IllegalArgumentException, IllegalAccessException { if (obj == null) { return ResourceTypeKey.NT_UNSTRUCTURED; } @@ -50,22 +52,34 @@ public static ResourceTypeKey fromObject(Object obj) { return NODE_TYPE_FOR_CLASS.get(obj.getClass()); } + String nodeType = getNodeType(obj); Model modelAnnotation = obj.getClass().getAnnotation(Model.class); - String primaryType = (String) getAnnotatedValue(obj, ResourceType.class); + String resourceType = (String) getAnnotatedValue(obj, ResourceType.class); + // Use the model annotation for resource type as needed - if (primaryType == null + if (resourceType == null && modelAnnotation != null && modelAnnotation.resourceType() != null - && modelAnnotation.resourceType().length == 1) { - primaryType = modelAnnotation.resourceType()[0]; + && modelAnnotation.resourceType().length >= 1) { + resourceType = modelAnnotation.resourceType()[0]; } - String childType = (String) getAnnotatedValue(obj, ChildType.class); - if (primaryType != null) { - ResourceTypeKey key = new ResourceTypeKey(primaryType, childType); + + if (nodeType != null || resourceType != null) { + ResourceTypeKey key = new ResourceTypeKey(nodeType, resourceType); NODE_TYPE_FOR_CLASS.put(obj.getClass(), key); return key; } else { return ResourceTypeKey.NT_UNSTRUCTURED; } } + + private static String getNodeType(Object obj) throws IllegalArgumentException, IllegalAccessException { + for (Field f : obj.getClass().getDeclaredFields()) { + Named namedAnnotation = f.getAnnotation(Named.class); + if (namedAnnotation != null && namedAnnotation.value().equals("jcr:primaryType")) { + return String.valueOf(FieldUtils.readField(f, obj, true)); + } + } + return null; + } } diff --git a/SlingModelPersist/src/test/java/org/apache/sling/models/persist/ModelPersistTest.java b/SlingModelPersist/src/test/java/org/apache/sling/models/persist/ModelPersistTest.java index 39319ce45..37f72e0f8 100644 --- a/SlingModelPersist/src/test/java/org/apache/sling/models/persist/ModelPersistTest.java +++ b/SlingModelPersist/src/test/java/org/apache/sling/models/persist/ModelPersistTest.java @@ -95,7 +95,9 @@ public void testPersistBeanPath() throws RepositoryException, PersistenceExcepti BeanWithPathField bean2 = new BeanWithPathField(); jcrPersist.persist(bean2, rr, false); - res = rr.getResource(bean2.path + "/jcr:content"); + // At one point this was testing if a jcr:content child was created automatically, but that concept was removed + // because it was ultimately too confusing to understand what the code was doing when using that feature. + res = rr.getResource(bean2.path); assertNotNull("Resource not created at expected path", res); assertEquals("Expected property not found", bean2.prop1, res.getValueMap().get("prop1", "missing")); diff --git a/SlingModelPersist/src/test/java/org/apache/sling/models/persist/impl/ResourceTypeKeyTest.java b/SlingModelPersist/src/test/java/org/apache/sling/models/persist/impl/ResourceTypeKeyTest.java index e0b5b4a94..80c0a647f 100644 --- a/SlingModelPersist/src/test/java/org/apache/sling/models/persist/impl/ResourceTypeKeyTest.java +++ b/SlingModelPersist/src/test/java/org/apache/sling/models/persist/impl/ResourceTypeKeyTest.java @@ -41,15 +41,18 @@ public void setUp() { * Test of fromObject method, of class ResourceTypeKey relying on class annotations */ @Test - public void testFromObject() { + public void testFromObject() throws IllegalAccessException { BeanWithPathField bean = new BeanWithPathField(); ResourceTypeKey result = ResourceTypeKey.fromObject(bean); - assertEquals("test/testBean", result.primaryType); - assertEquals("test/testBean/field-path", result.childType); + assertEquals("Should be NT UNSTRUCTURED", "nt:unstructured", result.nodeType); + assertEquals("test/testBean", result.resourceType); + // Child type wasn't making much sense, this was removed in favor of objects creating their own jcr:content children + // It was a tough call but ultimately makes it easier to understand what's going on since there are fewer rules to remember. + //assertEquals("test/testBean/field-path", result.childType); } @Test - public void testFromObjectCache() { + public void testFromObjectCache() throws IllegalAccessException { BeanWithPathField bean1 = new BeanWithPathField(); BeanWithPathField bean2 = new BeanWithPathField(); ResourceTypeKey result1 = ResourceTypeKey.fromObject(bean1); @@ -58,9 +61,9 @@ public void testFromObjectCache() { } @Test - public void testNullObject() { + public void testNullObject() throws IllegalAccessException { ResourceTypeKey result = ResourceTypeKey.fromObject(null); assertNotNull("Should not return null", result); - assertEquals("Should be NT UNSTRUCTURED", "nt:unstructured", result.primaryType); + assertEquals("Should be NT UNSTRUCTURED", "nt:unstructured", result.nodeType); } }