From cd686e5121cf091323974ba4925626c2c074f806 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Weso=C5=82owski?= Date: Tue, 30 Mar 2021 18:36:39 +0200 Subject: [PATCH 1/6] Fix dependencies loop --- .../themecleanflex/models/ReferenceModel.java | 63 ++++++++++++++++++- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/themecleanflex/models/ReferenceModel.java b/core/src/main/java/com/themecleanflex/models/ReferenceModel.java index 4995700c..2e29435f 100644 --- a/core/src/main/java/com/themecleanflex/models/ReferenceModel.java +++ b/core/src/main/java/com/themecleanflex/models/ReferenceModel.java @@ -3,6 +3,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.peregrine.commons.util.PerConstants; +import com.peregrine.commons.util.PerUtil; import com.peregrine.nodetypes.models.AbstractComponent; import com.peregrine.nodetypes.models.IComponent; import org.apache.sling.api.resource.Resource; @@ -21,12 +22,18 @@ import java.io.StringWriter; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.Map; +import java.util.Optional; +import java.util.Set; import javax.inject.Inject; import static java.util.Objects.isNull; +import static org.apache.commons.lang.StringUtils.contains; +import static org.apache.commons.lang.StringUtils.isBlank; + /* //GEN[:DATA { @@ -567,10 +574,11 @@ public String getReferenceJson() { } private String generateReferenceJson() { - if(reference == null || "".equals(reference)) { + if (isBlank(reference) || createsReferenceLoop(getResource(), new HashSet<>())) { return null; } - ResourceResolver resourceResolver = getResource().getResourceResolver(); + + ResourceResolver resourceResolver = getResource().getResourceResolver(); Resource referencedResource = resourceResolver.getResource(reference+"/jcr:content"); if(referencedResource == null) { LOG.error("Reference '{}' does not resolve to a resource.", reference); @@ -602,7 +610,56 @@ private String generateReferenceJson() { return null; } - // find a node with the given key/value pair in our json output + private boolean createsReferenceLoop(final Resource resource, final Set forbiddenPaths) { + if (forbiddenPaths.contains(resource.getPath())) { + return true; + } + + forbiddenPaths.addAll(forbiddenReferences(resource.getPath())); + if (Optional.of(resource) + .filter(r -> r.isResourceType("themecleanflex/components/reference")) + .map(Resource::getValueMap) + .map(props -> props.get("reference", String.class)) + .map(resource.getResourceResolver()::getResource) + .map(r -> createsReferenceLoop(r, forbiddenPaths)) + .orElse(false)) { + return true; + } + + for (final Resource child : resource.getChildren()) { + if (createsReferenceLoop(child, forbiddenPaths)) { + return true; + } + } + + return false; + } + + private static Set forbiddenReferences(final String path) { + final Set result = new HashSet<>(); + result.add(path); + if (!isJcrContentDescendant(path)) { + if (PerUtil.isJcrContent(path)) { + result.add(PerUtil.getParent(path)); + } + + return result; + } + + String parent = path; + do { + parent = PerUtil.getParent(parent); + result.add(parent); + } while (!PerUtil.isJcrContent(parent)); + result.add(PerUtil.getParent(parent)); + return result; + } + + private static boolean isJcrContentDescendant(final String path) { + return contains(path, "/jcr:content/"); + } + + // find a node with the given key/value pair in our json output private Map findNode(Map map, String name, String value) { for (Object key : map.keySet()) { Object val = map.get(key); From f78cdd4c9b2529c97bd97c9b8fe5590aed06b343 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Weso=C5=82owski?= Date: Fri, 9 Apr 2021 15:04:30 +0200 Subject: [PATCH 2/6] Use ThreadLocal to enhance performance As requested in PR review --- .../themecleanflex/models/ReferenceModel.java | 72 +++++++++++++++---- 1 file changed, 58 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/com/themecleanflex/models/ReferenceModel.java b/core/src/main/java/com/themecleanflex/models/ReferenceModel.java index 2e29435f..debec926 100644 --- a/core/src/main/java/com/themecleanflex/models/ReferenceModel.java +++ b/core/src/main/java/com/themecleanflex/models/ReferenceModel.java @@ -22,6 +22,7 @@ import java.io.StringWriter; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Optional; @@ -328,6 +329,9 @@ //GEN] public class ReferenceModel extends AbstractComponent { + private static final ThreadLocal> forbiddenPaths = ThreadLocal.withInitial(HashSet::new); + private static final ThreadLocal> previousReferences = ThreadLocal.withInitial(HashMap::new); + public ReferenceModel(Resource r) { super(r); } //GEN[:INJECT @@ -574,11 +578,23 @@ public String getReferenceJson() { } private String generateReferenceJson() { - if (isBlank(reference) || createsReferenceLoop(getResource(), new HashSet<>())) { - return null; - } + if (isBlank(reference)) { + return null; + } + + final Resource resource = getResource(); + final ResourceResolver resourceResolver = resource.getResourceResolver(); + final Map references = previousReferences.get(); + final Set paths = forbiddenPaths.get(); + if (referencesChanged(references, resourceResolver)) { + references.clear(); + paths.clear(); + } + + if (createsReferenceLoop(resource, references, paths)) { + return null; + } - ResourceResolver resourceResolver = getResource().getResourceResolver(); Resource referencedResource = resourceResolver.getResource(reference+"/jcr:content"); if(referencedResource == null) { LOG.error("Reference '{}' does not resolve to a resource.", reference); @@ -610,24 +626,41 @@ private String generateReferenceJson() { return null; } - private boolean createsReferenceLoop(final Resource resource, final Set forbiddenPaths) { - if (forbiddenPaths.contains(resource.getPath())) { + private boolean referencesChanged(final Map references, final ResourceResolver resourceResolver) { + for (final var e : references.entrySet()) { + final String path = e.getKey(); + if (Optional.of(path) + .map(resourceResolver::getResource) + .map(ReferenceModel::getReferencePath) + .map(ref -> !equals(e.getValue(), ref)) + .orElse(true) + ) { + return true; + } + } + + return false; + } + + private boolean createsReferenceLoop(final Resource resource, final Map references, final Set forbiddenPaths) { + final Optional referenceOpt = Optional.of(resource) + .filter(r -> r.isResourceType("themecleanflex/components/reference")) + .map(ReferenceModel::getReferencePath); + final String path = resource.getPath(); + referenceOpt.ifPresent(ref -> references.put(path, ref)); + if (forbiddenPaths.contains(path)) { return true; } - forbiddenPaths.addAll(forbiddenReferences(resource.getPath())); - if (Optional.of(resource) - .filter(r -> r.isResourceType("themecleanflex/components/reference")) - .map(Resource::getValueMap) - .map(props -> props.get("reference", String.class)) - .map(resource.getResourceResolver()::getResource) - .map(r -> createsReferenceLoop(r, forbiddenPaths)) + forbiddenPaths.addAll(forbiddenReferences(path)); + if (referenceOpt.map(resource.getResourceResolver()::getResource) + .map(r -> createsReferenceLoop(r, references, forbiddenPaths)) .orElse(false)) { return true; } for (final Resource child : resource.getChildren()) { - if (createsReferenceLoop(child, forbiddenPaths)) { + if (createsReferenceLoop(child, references, forbiddenPaths)) { return true; } } @@ -659,6 +692,17 @@ private static boolean isJcrContentDescendant(final String path) { return contains(path, "/jcr:content/"); } + private static String getReferencePath(final Resource resource) { + return getStringProperty(resource, "reference"); + } + + private static String getStringProperty(final Resource resource, final String name) { + return Optional.ofNullable(resource) + .map(Resource::getValueMap) + .map(props -> props.get(name, String.class)) + .orElse(null); + } + // find a node with the given key/value pair in our json output private Map findNode(Map map, String name, String value) { for (Object key : map.keySet()) { From b8442907dcab7f73197db50948a3be06578d4bf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Weso=C5=82owski?= Date: Fri, 9 Apr 2021 23:30:11 +0200 Subject: [PATCH 3/6] Simplify checks with ThreadLocal As it should have been done --- .../themecleanflex/models/ReferenceModel.java | 64 +------------------ 1 file changed, 2 insertions(+), 62 deletions(-) diff --git a/core/src/main/java/com/themecleanflex/models/ReferenceModel.java b/core/src/main/java/com/themecleanflex/models/ReferenceModel.java index 4a9cb514..1351eb5d 100644 --- a/core/src/main/java/com/themecleanflex/models/ReferenceModel.java +++ b/core/src/main/java/com/themecleanflex/models/ReferenceModel.java @@ -22,10 +22,8 @@ import java.io.StringWriter; import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.HashSet; import java.util.Map; -import java.util.Optional; import java.util.Set; import javax.inject.Inject; @@ -358,7 +356,6 @@ public class ReferenceModel extends AbstractComponent { private static final ThreadLocal> forbiddenPaths = ThreadLocal.withInitial(HashSet::new); - private static final ThreadLocal> previousReferences = ThreadLocal.withInitial(HashMap::new); public ReferenceModel(Resource r) { super(r); } @@ -612,14 +609,10 @@ private String generateReferenceJson() { final Resource resource = getResource(); final ResourceResolver resourceResolver = resource.getResourceResolver(); - final Map references = previousReferences.get(); final Set paths = forbiddenPaths.get(); - if (referencesChanged(references, resourceResolver)) { - references.clear(); + paths.addAll(forbiddenReferences(resource.getPath())); + if (paths.contains(reference)) { paths.clear(); - } - - if (createsReferenceLoop(resource, references, paths)) { return null; } @@ -654,48 +647,6 @@ private String generateReferenceJson() { return null; } - private boolean referencesChanged(final Map references, final ResourceResolver resourceResolver) { - for (final var e : references.entrySet()) { - final String path = e.getKey(); - if (Optional.of(path) - .map(resourceResolver::getResource) - .map(ReferenceModel::getReferencePath) - .map(ref -> !equals(e.getValue(), ref)) - .orElse(true) - ) { - return true; - } - } - - return false; - } - - private boolean createsReferenceLoop(final Resource resource, final Map references, final Set forbiddenPaths) { - final Optional referenceOpt = Optional.of(resource) - .filter(r -> r.isResourceType("themecleanflex/components/reference")) - .map(ReferenceModel::getReferencePath); - final String path = resource.getPath(); - referenceOpt.ifPresent(ref -> references.put(path, ref)); - if (forbiddenPaths.contains(path)) { - return true; - } - - forbiddenPaths.addAll(forbiddenReferences(path)); - if (referenceOpt.map(resource.getResourceResolver()::getResource) - .map(r -> createsReferenceLoop(r, references, forbiddenPaths)) - .orElse(false)) { - return true; - } - - for (final Resource child : resource.getChildren()) { - if (createsReferenceLoop(child, references, forbiddenPaths)) { - return true; - } - } - - return false; - } - private static Set forbiddenReferences(final String path) { final Set result = new HashSet<>(); result.add(path); @@ -720,17 +671,6 @@ private static boolean isJcrContentDescendant(final String path) { return contains(path, "/jcr:content/"); } - private static String getReferencePath(final Resource resource) { - return getStringProperty(resource, "reference"); - } - - private static String getStringProperty(final Resource resource, final String name) { - return Optional.ofNullable(resource) - .map(Resource::getValueMap) - .map(props -> props.get(name, String.class)) - .orElse(null); - } - // find a node with the given key/value pair in our json output private Map findNode(Map map, String name, String value) { for (Object key : map.keySet()) { From 289f82d2d0dede895a20356925c23380f0571148 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Weso=C5=82owski?= Date: Mon, 12 Apr 2021 13:36:06 +0200 Subject: [PATCH 4/6] Fix weird issues with the editor --- .../themecleanflex/models/ReferenceModel.java | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/themecleanflex/models/ReferenceModel.java b/core/src/main/java/com/themecleanflex/models/ReferenceModel.java index 1351eb5d..33847bd4 100644 --- a/core/src/main/java/com/themecleanflex/models/ReferenceModel.java +++ b/core/src/main/java/com/themecleanflex/models/ReferenceModel.java @@ -356,6 +356,8 @@ public class ReferenceModel extends AbstractComponent { private static final ThreadLocal> forbiddenPaths = ThreadLocal.withInitial(HashSet::new); + private static final ThreadLocal foundLoop = ThreadLocal.withInitial(() -> false); + private static final ThreadLocal recursionStarter = ThreadLocal.withInitial(() -> true); public ReferenceModel(Resource r) { super(r); } @@ -612,7 +614,7 @@ private String generateReferenceJson() { final Set paths = forbiddenPaths.get(); paths.addAll(forbiddenReferences(resource.getPath())); if (paths.contains(reference)) { - paths.clear(); + foundLoop.set(true); return null; } @@ -622,8 +624,20 @@ private String generateReferenceJson() { return null; } try { + final boolean isLevelZero = recursionStarter.get(); + recursionStarter.set(false); Map referenceMap = modelFactory.exportModelForResource(referencedResource, - PerConstants.JACKSON, Map.class, Collections.emptyMap()); + PerConstants.JACKSON, Map.class, Collections.emptyMap()); + final boolean looped = foundLoop.get(); + if (isLevelZero) { + paths.clear(); + foundLoop.set(false); + recursionStarter.set(true); + } + + if (looped) { + return null; + } // TODO: finding the node should happen before the export due to the fact that this // could result in a recursion if we point to content on the same page From 0c2cf24fce702c3bdc010c3868e48d3b34ef0c3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Weso=C5=82owski?= Date: Mon, 12 Apr 2021 16:04:11 +0200 Subject: [PATCH 5/6] Include contentnameref in the loop checking --- .../themecleanflex/models/ReferenceModel.java | 142 ++++++++++-------- 1 file changed, 81 insertions(+), 61 deletions(-) diff --git a/core/src/main/java/com/themecleanflex/models/ReferenceModel.java b/core/src/main/java/com/themecleanflex/models/ReferenceModel.java index 33847bd4..dee2740e 100644 --- a/core/src/main/java/com/themecleanflex/models/ReferenceModel.java +++ b/core/src/main/java/com/themecleanflex/models/ReferenceModel.java @@ -6,8 +6,10 @@ import com.peregrine.commons.util.PerUtil; import com.peregrine.nodetypes.models.AbstractComponent; import com.peregrine.nodetypes.models.IComponent; +import org.apache.commons.lang.StringUtils; import org.apache.sling.api.resource.Resource; import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.api.resource.ValueMap; import org.apache.sling.models.annotations.Default; import org.apache.sling.models.annotations.DefaultInjectionStrategy; import org.apache.sling.models.annotations.Exporter; @@ -20,18 +22,17 @@ import java.io.IOException; import java.io.StringWriter; -import java.util.ArrayList; -import java.util.Collections; -import java.util.HashSet; -import java.util.Map; -import java.util.Set; +import java.util.*; +import java.util.stream.Collectors; import javax.inject.Inject; +import static com.peregrine.commons.util.PerConstants.JCR_CONTENT; +import static com.peregrine.commons.util.PerConstants.NT_UNSTRUCTURED; import static java.util.Objects.isNull; +import static java.util.Objects.nonNull; import static org.apache.commons.lang.StringUtils.contains; -import static org.apache.commons.lang.StringUtils.isBlank; /* //GEN[:DATA @@ -355,11 +356,15 @@ //GEN] public class ReferenceModel extends AbstractComponent { + private static final String PN_CONTENT_NAME = "contentname"; + + private static final ObjectMapper MAPPER = new ObjectMapper(); + private static final ThreadLocal> forbiddenPaths = ThreadLocal.withInitial(HashSet::new); private static final ThreadLocal foundLoop = ThreadLocal.withInitial(() -> false); private static final ThreadLocal recursionStarter = ThreadLocal.withInitial(() -> true); - public ReferenceModel(Resource r) { super(r); } + public ReferenceModel(Resource r) { super(r); } //GEN[:INJECT /* {"type":"string","x-source":"inject","x-form-label":"Reference","x-form-group":"content","x-form-type":"pathbrowser","x-form-browserRoot":"/content/themecleanflex/pages"} */ @@ -605,28 +610,30 @@ public String getReferenceJson() { } private String generateReferenceJson() { - if (isBlank(reference)) { + final Resource resource = getResource(); + final ResourceResolver resourceResolver = resource.getResourceResolver(); + final String contentName = getContentnameref(); + final Resource referencedResource = Optional.ofNullable(reference) + .map(resourceResolver::getResource) + .map(r -> r.getChild(JCR_CONTENT)) + .map(r -> findComponentWithProperty(r, PN_CONTENT_NAME, contentName)) + .orElse(null); + if (isNull(referencedResource)) { + LOG.error("Reference '{}' does not resolve to a resource.", reference); return null; } - final Resource resource = getResource(); - final ResourceResolver resourceResolver = resource.getResourceResolver(); final Set paths = forbiddenPaths.get(); - paths.addAll(forbiddenReferences(resource.getPath())); - if (paths.contains(reference)) { + paths.addAll(forbiddenReferences(resource)); + if (paths.contains(ref(reference, contentName))) { foundLoop.set(true); return null; } - Resource referencedResource = resourceResolver.getResource(reference+"/jcr:content"); - if(referencedResource == null) { - LOG.error("Reference '{}' does not resolve to a resource.", reference); - return null; - } try { final boolean isLevelZero = recursionStarter.get(); - recursionStarter.set(false); - Map referenceMap = modelFactory.exportModelForResource(referencedResource, + recursionStarter.set(false); + final Map result = modelFactory.exportModelForResource(referencedResource, PerConstants.JACKSON, Map.class, Collections.emptyMap()); final boolean looped = foundLoop.get(); if (isLevelZero) { @@ -635,22 +642,14 @@ private String generateReferenceJson() { recursionStarter.set(true); } - if (looped) { + if (looped || isNull(result)) { return null; } - // TODO: finding the node should happen before the export due to the fact that this - // could result in a recursion if we point to content on the same page - Map result = findNode(referenceMap, "contentname", getContentnameref()); - if(result != null) { - StringWriter writer = new StringWriter(); - ObjectMapper mapper = new ObjectMapper(); - mapper.writeValue(writer, result); - writer.close(); - return writer.toString(); - } else { - return null; - } + StringWriter writer = new StringWriter(); + MAPPER.writeValue(writer, result); + writer.close(); + return writer.toString(); } catch (ExportException e) { LOG.error("Export failed for resource " + reference, e); } catch (MissingExporterException e) { @@ -661,51 +660,72 @@ private String generateReferenceJson() { return null; } - private static Set forbiddenReferences(final String path) { + private static String ref(final String path, final Object contentName) { + return isNull(contentName) ? path : contentName + "@" + path; + } + + private static Set forbiddenReferences(final Resource resource) { final Set result = new HashSet<>(); - result.add(path); + final Set contentNames = new HashSet<>(); + contentNames.add(contentName(resource)); + result.addAll(refs(resource, contentNames)); + final String path = resource.getPath(); if (!isJcrContentDescendant(path)) { if (PerUtil.isJcrContent(path)) { - result.add(PerUtil.getParent(path)); + result.addAll(refs(resource.getParent(), contentNames)); } return result; } - String parent = path; + Resource parent = resource.getParent(); + contentNames.add(contentName(parent)); do { - parent = PerUtil.getParent(parent); - result.add(parent); - } while (!PerUtil.isJcrContent(parent)); - result.add(PerUtil.getParent(parent)); + parent = parent.getParent(); + contentNames.add(contentName(parent)); + result.addAll(refs(parent, contentNames)); + } while (!JCR_CONTENT.equals(parent.getName())); + parent = parent.getParent(); + contentNames.add(contentName(parent)); + result.addAll(refs(parent, contentNames)); return result; } + private static Set refs(final Resource resource, final Set contentNames) { + return refs(resource.getPath(), contentNames); + } + + private static Set refs(final String path, final Set contentNames) { + return contentNames.stream().map(cn -> ref(path, cn)).collect(Collectors.toSet()); + } + + private static String contentName(final Resource resource) { + return Optional.ofNullable(resource) + .map(Resource::getValueMap) + .map(props -> props.get(PN_CONTENT_NAME, String.class)) + .orElse(null); + } + private static boolean isJcrContentDescendant(final String path) { return contains(path, "/jcr:content/"); } - // find a node with the given key/value pair in our json output - private Map findNode(Map map, String name, String value) { - for (Object key : map.keySet()) { - Object val = map.get(key); - if(equals(key, name)) { - if(equals(value, val)) { - return map; - } - } - if("children".equals(key) && val instanceof ArrayList) { - ArrayList children = (ArrayList) val; - for (Object child : children) { - if(child instanceof Map) { - Map ret = findNode((Map) child, name, value); - if(ret != null) return ret; - } - } - } - } - return null; - } + private Resource findComponentWithProperty(final Resource resource, final String name, final String value) { + for (final Resource child : resource.getChildren()) { + if (!equals(child.getResourceType(), NT_UNSTRUCTURED) && equals(child.getValueMap().get(name, String.class), value)) { + return child; + } + } + + for (final Resource child : resource.getChildren()) { + final Resource result = findComponentWithProperty(child, name, value); + if (nonNull(result)) { + return result; + } + } + + return null; + } private static boolean equals(final Object obj, final Object other) { if (obj == other) { From 663c4c6481c83187ef9e78579cc44ee76e71fbb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Weso=C5=82owski?= Date: Mon, 12 Apr 2021 22:06:30 +0200 Subject: [PATCH 6/6] Make findComponentWithProperty static --- .../main/java/com/themecleanflex/models/ReferenceModel.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/themecleanflex/models/ReferenceModel.java b/core/src/main/java/com/themecleanflex/models/ReferenceModel.java index dee2740e..a06ce64b 100644 --- a/core/src/main/java/com/themecleanflex/models/ReferenceModel.java +++ b/core/src/main/java/com/themecleanflex/models/ReferenceModel.java @@ -631,8 +631,8 @@ private String generateReferenceJson() { } try { - final boolean isLevelZero = recursionStarter.get(); - recursionStarter.set(false); + final boolean isLevelZero = recursionStarter.get(); + recursionStarter.set(false); final Map result = modelFactory.exportModelForResource(referencedResource, PerConstants.JACKSON, Map.class, Collections.emptyMap()); final boolean looped = foundLoop.get(); @@ -710,7 +710,7 @@ private static boolean isJcrContentDescendant(final String path) { return contains(path, "/jcr:content/"); } - private Resource findComponentWithProperty(final Resource resource, final String name, final String value) { + private static Resource findComponentWithProperty(final Resource resource, final String name, final String value) { for (final Resource child : resource.getChildren()) { if (!equals(child.getResourceType(), NT_UNSTRUCTURED) && equals(child.getValueMap().get(name, String.class), value)) { return child;