rombert commented on a change in pull request #43:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/43#discussion_r604836317
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java
##########
@@ -1046,37 +1051,57 @@ public String getParentResourceType(final String
resourceType) {
public boolean isResourceType(final Resource resource, final String
resourceType) {
boolean result = false;
if ( resource != null && resourceType != null ) {
- // Check if the resource is of the given type. This method first
checks the
- // resource type of the resource, then its super resource type
and continues
- // to go up the resource super type hierarchy.
- if (ResourceTypeUtil.areResourceTypesEqual(resourceType,
resource.getResourceType(), factory.getSearchPath())) {
- result = true;
- } else {
- Set<String> superTypesChecked = new HashSet<>();
- String superType = this.getParentResourceType(resource);
- while (!result && superType != null) {
- if (ResourceTypeUtil.areResourceTypesEqual(resourceType,
superType, factory.getSearchPath())) {
- result = true;
- } else {
- superTypesChecked.add(superType);
- superType = this.getParentResourceType(superType);
- if (superType != null &&
superTypesChecked.contains(superType)) {
- throw new SlingException("Cyclic dependency for
resourceSuperType hierarchy detected on resource " + resource.getPath(), null);
- }
- }
- }
+
+ // Check if the result is already available from cache
+ String3Tupel key = new
String3Tupel(resource.getResourceType(),resource.getResourceSuperType(),
resourceType);
+ if (resourceTypeLookupCache.containsKey(key)) {
Review comment:
I would suggest a get and null check. This way we don't query the map
twice.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java
##########
@@ -1046,37 +1051,57 @@ public String getParentResourceType(final String
resourceType) {
public boolean isResourceType(final Resource resource, final String
resourceType) {
boolean result = false;
if ( resource != null && resourceType != null ) {
- // Check if the resource is of the given type. This method first
checks the
Review comment:
This inline comment seems to have been lost in the refactoring.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java
##########
@@ -1117,4 +1142,68 @@ public Resource move(final String srcAbsPath, final
String destAbsPath) throws P
}
return rsrc;
}
+
+
+
+ // A very simple tupel implementation which can be used as key in any map
+ public class String3Tupel {
+
+ @Override
+ public int hashCode() {
+ final int prime = 31;
Review comment:
I think you can use `java.util.Objects.hashCode` here.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java
##########
@@ -1117,4 +1142,68 @@ public Resource move(final String srcAbsPath, final
String destAbsPath) throws P
}
return rsrc;
}
+
+
+
+ // A very simple tupel implementation which can be used as key in any map
+ public class String3Tupel {
+
+ @Override
+ public int hashCode() {
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + getEnclosingInstance().hashCode();
+ result = prime * result + ((s1 == null) ? 0 : s1.hashCode());
+ result = prime * result + ((s2 == null) ? 0 : s2.hashCode());
+ result = prime * result + ((s3 == null) ? 0 : s3.hashCode());
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj)
Review comment:
I think you can use `java.util.Objects.equals` here.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java
##########
@@ -1117,4 +1142,68 @@ public Resource move(final String srcAbsPath, final
String destAbsPath) throws P
}
return rsrc;
}
+
+
+
+ // A very simple tupel implementation which can be used as key in any map
+ public class String3Tupel {
Review comment:
Nit: tuple, not tupel :-)
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java
##########
@@ -1117,4 +1142,68 @@ public Resource move(final String srcAbsPath, final
String destAbsPath) throws P
}
return rsrc;
}
+
+
+
+ // A very simple tupel implementation which can be used as key in any map
+ public class String3Tupel {
+
+ @Override
+ public int hashCode() {
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + getEnclosingInstance().hashCode();
+ result = prime * result + ((s1 == null) ? 0 : s1.hashCode());
+ result = prime * result + ((s2 == null) ? 0 : s2.hashCode());
+ result = prime * result + ((s3 == null) ? 0 : s3.hashCode());
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj)
+ return true;
+ if (obj == null)
+ return false;
+ if (getClass() != obj.getClass())
+ return false;
+ String3Tupel other = (String3Tupel) obj;
+ if (!getEnclosingInstance().equals(other.getEnclosingInstance()))
+ return false;
+ if (s1 == null) {
+ if (other.s1 != null)
+ return false;
+ } else if (!s1.equals(other.s1))
+ return false;
+ if (s2 == null) {
+ if (other.s2 != null)
+ return false;
+ } else if (!s2.equals(other.s2))
+ return false;
+ if (s3 == null) {
+ if (other.s3 != null)
+ return false;
+ } else if (!s3.equals(other.s3))
+ return false;
+ return true;
+ }
+
+ String s1;
Review comment:
Any reason to not make these fields private? Also, fields should go
before contructors, constructors before instance methods :-)
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java
##########
@@ -1117,4 +1142,68 @@ public Resource move(final String srcAbsPath, final
String destAbsPath) throws P
}
return rsrc;
}
+
+
+
+ // A very simple tupel implementation which can be used as key in any map
+ public class String3Tupel {
+
+ @Override
+ public int hashCode() {
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + getEnclosingInstance().hashCode();
Review comment:
Is this necessary? A tuple3 does not escape the scope of a single
resource resolver, right?
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java
##########
@@ -1117,4 +1142,68 @@ public Resource move(final String srcAbsPath, final
String destAbsPath) throws P
}
return rsrc;
}
+
+
+
+ // A very simple tupel implementation which can be used as key in any map
+ public class String3Tupel {
+
+ @Override
+ public int hashCode() {
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + getEnclosingInstance().hashCode();
+ result = prime * result + ((s1 == null) ? 0 : s1.hashCode());
+ result = prime * result + ((s2 == null) ? 0 : s2.hashCode());
+ result = prime * result + ((s3 == null) ? 0 : s3.hashCode());
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj)
+ return true;
+ if (obj == null)
+ return false;
+ if (getClass() != obj.getClass())
+ return false;
+ String3Tupel other = (String3Tupel) obj;
+ if (!getEnclosingInstance().equals(other.getEnclosingInstance()))
Review comment:
Same as above, is this needed?
##########
File path:
src/test/java/org/apache/sling/resourceresolver/impl/ResourceResolverImplTest.java
##########
@@ -521,6 +527,28 @@ public void testCloseWithStackTraceLogging() throws
Exception {
assertFalse(resolver.isResourceType(r, "h:p"));
}
+ @Test public void testIsResourceTypeCached() throws NoSuchFieldException,
SecurityException, IllegalArgumentException, IllegalAccessException {
Review comment:
Nit: maybe `throws Exception`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
