Skip to content

SlingModelPersist type handling cleanup #73

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
badvision wants to merge 2 commits into
base: master
Choose a base branch
from
Open

SlingModelPersist type handling cleanup #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffdc31b
Merge pull request #2 from apache/master
badvision Mar 30, 2021
2e224e8
Simplifying primary type and resource type handling rules
Mar 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion SlingModelPersist/src/main/java/org/apache/sling/models/persistor/ModelPersistor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@
*/
package org.apache.sling.models.persistor;

import com.drew.lang.annotations.NotNull;
Copy link
Copy Markdown
Contributor

@klcodanr klcodanr Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@badvision any particular reason for changing annotation types? A number of other projects use the Jetbrains annotations, so it'd be better to be consistent

Copy link
Copy Markdown
Contributor Author

@badvision badvision Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm good point, I think that was an oversight as I was having difficulty building in this one case -- happy to revert them back.

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
Expand Down
19 changes: 12 additions & 7 deletions ...ModelPersist/src/main/java/org/apache/sling/models/persistor/impl/ModelPersistorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
package org.apache.sling.models.persistor.impl;

import com.drew.lang.annotations.NotNull;
Copy link
Copy Markdown
Contributor

@klcodanr klcodanr Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment re:annotation

import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
Expand All @@ -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;
Expand Down Expand Up @@ -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<String, Object> 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)) {
Expand All @@ -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));
}
Expand Down Expand Up @@ -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);
}
Expand Down
42 changes: 28 additions & 14 deletions SlingModelPersist/src/main/java/org/apache/sling/models/persistor/impl/ResourceTypeKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -33,15 +35,15 @@ public class ResourceTypeKey {
static final Map<Class<?>, 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;
}
Expand All @@ -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")) {
Copy link
Copy Markdown
Contributor

@klcodanr klcodanr Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should use: http://jackrabbit.apache.org/api/2.20/org/apache/jackrabbit/JcrConstants.html#JCR_PRIMARYTYPE

Copy link
Copy Markdown
Contributor Author

@badvision badvision Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, I try to avoid hard-coding too. Thanks for catching that.

return String.valueOf(FieldUtils.readField(f, obj, true));
}
}
return null;
}
}
4 changes: 3 additions & 1 deletion SlingModelPersist/src/test/java/org/apache/sling/models/persist/ModelPersistTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));

Expand Down
15 changes: 9 additions & 6 deletions ...gModelPersist/src/test/java/org/apache/sling/models/persist/impl/ResourceTypeKeyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}
}