You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2022/06/10 12:46:41 UTC
[GitHub] [sling-org-apache-sling-servlets-resolver] karlpauls opened a new pull request, #29: SLING-11373: wrap resources returned from decorator to have them appe…
karlpauls opened a new pull request, #29:
URL: https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/29
…ar in the tree correctly
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-servlets-resolver] jsedding commented on a diff in pull request #29: SLING-11373: wrap resources returned from decorator to have them appe…
Posted by GitBox <gi...@apache.org>.
jsedding commented on code in PR #29:
URL: https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/29#discussion_r899934975
##########
src/main/java/org/apache/sling/servlets/resolver/internal/ScriptResourceResolver.java:
##########
@@ -173,14 +326,84 @@ public ScriptResourceResolver clone(Map<String, Object> o) throws LoginException
return ScriptResourceResolver.wrap(resolver.clone(o), providerSupplier);
}
- private class ScriptResourceResolverResourceWrapper extends ResourceWrapper {
+ private class ScriptResourceResolverResourceWrapper implements Resource {
+ private final Resource delegate;
public ScriptResourceResolverResourceWrapper(Resource resource) {
- super(resource);
+ this.delegate = resource;
+ }
+
+ Resource getDelegate() {
+ return delegate;
}
@Override
public ResourceResolver getResourceResolver() {
return ScriptResourceResolver.this;
}
+
+ @Override
+ public Resource getChild( final String relPath) {
+ return ScriptResourceResolver.this.getResource(this, relPath);
Review Comment:
It is actually correct. At least [`AbstractResource` implements it in the same spirit](https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/AbstractResource.java#L77).
However, I think we should just extend from `AbstractResource` to simplify and reduce risk.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-servlets-resolver] joerghoh commented on a diff in pull request #29: SLING-11373: wrap resources returned from decorator to have them appe…
Posted by GitBox <gi...@apache.org>.
joerghoh commented on code in PR #29:
URL: https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/29#discussion_r899919412
##########
src/main/java/org/apache/sling/servlets/resolver/internal/ScriptResourceResolver.java:
##########
@@ -173,14 +326,84 @@ public ScriptResourceResolver clone(Map<String, Object> o) throws LoginException
return ScriptResourceResolver.wrap(resolver.clone(o), providerSupplier);
}
- private class ScriptResourceResolverResourceWrapper extends ResourceWrapper {
+ private class ScriptResourceResolverResourceWrapper implements Resource {
+ private final Resource delegate;
public ScriptResourceResolverResourceWrapper(Resource resource) {
- super(resource);
+ this.delegate = resource;
+ }
+
+ Resource getDelegate() {
+ return delegate;
}
@Override
public ResourceResolver getResourceResolver() {
return ScriptResourceResolver.this;
}
+
+ @Override
+ public Resource getChild( final String relPath) {
+ return ScriptResourceResolver.this.getResource(this, relPath);
Review Comment:
This feels wrong. Shouldn't ```getChild``` be invoked here?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-servlets-resolver] jsedding commented on a diff in pull request #29: SLING-11373: wrap resources returned from decorator to have them appe…
Posted by GitBox <gi...@apache.org>.
jsedding commented on code in PR #29:
URL: https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/29#discussion_r899875485
##########
src/main/java/org/apache/sling/servlets/resolver/internal/ScriptResourceResolver.java:
##########
@@ -107,7 +108,7 @@ public Resource getResource(Resource base, @NotNull String path) {
public Iterator<Resource> listChildren(Resource parent) {
MergingServletResourceProvider provider = this.providerSupplier.get();
if (provider == null) {
- return super.listChildren(parent);
+ return resolver.listChildren(parent);
Review Comment:
To be consistent I think `getDelegate()` instead of `resolver` would be cleaner. Same as above.
##########
src/main/java/org/apache/sling/servlets/resolver/internal/ScriptResourceDecorator.java:
##########
@@ -61,7 +61,7 @@ public Resource decorate(Resource resource) {
script.getResourceMetadata().setResolutionPath(resolutionPath);
}
- return script;
+ return ScriptResourceResolver.wrap(resource.getResourceResolver(), () -> this.provider).wrap(script);
Review Comment:
Isn't this equivalent to `ScriptResourceResolver.wrap(resource.getResourceResolver(), () -> this.provider).getResource(path)`? If it is, you could remove the method `getResource(Resource resource, String path)` below, which seems to duplicate code.
##########
src/main/java/org/apache/sling/servlets/resolver/internal/ScriptResourceResolver.java:
##########
@@ -58,7 +59,7 @@ public Resource getResource(String scriptPath) {
MergingServletResourceProvider provider = this.providerSupplier.get();
if (provider == null) {
- return super.getResource(scriptPath);
+ return resolver.getResource(scriptPath);
Review Comment:
To be consistent I think `getDelegate()` instead of `resolver` would be cleaner.
##########
src/main/java/org/apache/sling/servlets/resolver/internal/ScriptResourceResolver.java:
##########
@@ -173,14 +326,84 @@ public ScriptResourceResolver clone(Map<String, Object> o) throws LoginException
return ScriptResourceResolver.wrap(resolver.clone(o), providerSupplier);
}
- private class ScriptResourceResolverResourceWrapper extends ResourceWrapper {
+ private class ScriptResourceResolverResourceWrapper implements Resource {
+ private final Resource delegate;
public ScriptResourceResolverResourceWrapper(Resource resource) {
- super(resource);
+ this.delegate = resource;
+ }
+
+ Resource getDelegate() {
+ return delegate;
}
@Override
public ResourceResolver getResourceResolver() {
return ScriptResourceResolver.this;
}
+
+ @Override
+ public Resource getChild( final String relPath) {
+ return ScriptResourceResolver.this.getResource(this, relPath);
+ }
+
+ @Override
+ public Iterable<Resource> getChildren() {
+ return ScriptResourceResolver.this.getChildren(this);
+ }
+
+ @Override
+ public String getName() {
+ return this.delegate.getName();
+ }
+
+ @Override
+ public Resource getParent() {
+ return ScriptResourceResolver.this.getParent(this);
+ }
+
+ @Override
+ public String getPath() {
+ return this.delegate.getPath();
+ }
+
+ @Override
+ public ResourceMetadata getResourceMetadata() {
+ return this.delegate.getResourceMetadata();
+ }
+
+ @Override
+ public String getResourceSuperType() {
+ return this.delegate.getResourceSuperType();
+ }
+
+ @Override
+ public String getResourceType() {
+ return this.delegate.getResourceType();
+ }
+
+ @Override
+ public ValueMap getValueMap() {
+ return this.delegate.getValueMap();
+ }
+
+ @Override
+ public boolean hasChildren() {
+ return listChildren().hasNext();
+ }
+
+ @Override
+ public boolean isResourceType(final String resourceType) {
+ return this.delegate.isResourceType(resourceType);
+ }
+
+ @Override
+ public Iterator<Resource> listChildren() {
+ return getChildren().iterator();
Review Comment:
How about `ScriptResourceResolver.this.listChildren(this)`? That saves the two calls to `getChildren()`and `ScriptResourceResolver.this.getChildren(this)` before eventually hitting `ScriptResourceResolver.this.listChildren(this)` anyway.
##########
src/main/java/org/apache/sling/servlets/resolver/internal/ScriptResourceResolver.java:
##########
@@ -173,14 +326,84 @@ public ScriptResourceResolver clone(Map<String, Object> o) throws LoginException
return ScriptResourceResolver.wrap(resolver.clone(o), providerSupplier);
}
- private class ScriptResourceResolverResourceWrapper extends ResourceWrapper {
+ private class ScriptResourceResolverResourceWrapper implements Resource {
Review Comment:
Maybe add a comment to indicate that extending from `ResourceWrapper` leads to undesired unwrapping and therefore implementing `Resource` directly prevents the unwrapping?
It may also be good to extend from `AbstractResource`. It is "strongly encouraged" according to the docs and it already takes care of delegating `getChildren()`, `listChildren()` and some other methods to the resource resolver (via `getResourceResolver()`, i.e. the wrapped one).
##########
src/main/java/org/apache/sling/servlets/resolver/internal/ScriptResourceResolver.java:
##########
@@ -142,8 +143,160 @@ public Iterator<Resource> listChildren(ResolveContext<Object> ctx, Resource pare
}
}
- private Resource wrap(Resource resource) {
- if (resource != null && !(resource.getResourceResolver() instanceof ScriptResourceResolver)) {
+ protected final ResourceResolver getDelegate() {
+ return resolver;
+ }
+
+ @Override
+ public void close() {
+ getDelegate().close();
+ }
+
+ @Override
+ public void commit() throws PersistenceException {
+ getDelegate().commit();
+ }
+
+ @Override
+ public Resource create(final Resource parent, final String name, final Map<String, Object> properties) throws PersistenceException {
+ return getDelegate().create(parent, name, properties);
+ }
+
+ @Override
+ public boolean orderBefore(Resource resource, String s, String s1) throws UnsupportedOperationException, PersistenceException, IllegalArgumentException {
+ return getDelegate().orderBefore(resource, s, s1);
+ }
+
+ @Override
+ public void delete( final Resource resource) throws PersistenceException {
+ getDelegate().delete(resource);
+ }
+
+ @Override
+ public Iterator<Resource> findResources(final String query, final String language) {
+ return getDelegate().findResources(query, language);
+ }
+
+ @Override
+ public Object getAttribute(final String name) {
+ return getDelegate().getAttribute(name);
+ }
+
+ @Override
+ public Iterator<String> getAttributeNames() {
+ return getDelegate().getAttributeNames();
+ }
+
+ @Override
+ public String getParentResourceType(final Resource resource) {
+ return getDelegate().getParentResourceType(resource);
+ }
+
+ @Override
+ public String getParentResourceType(final String resourceType) {
+ return getDelegate().getParentResourceType(resourceType);
+ }
+
+ @Override
+ public String[] getSearchPath() {
+ return getDelegate().getSearchPath();
+ }
+
+ @Override
+ public String getUserID() {
+ return getDelegate().getUserID();
+ }
+
+ @Override
+ public boolean hasChanges() {
+ return getDelegate().hasChanges();
+ }
+
+ @Override
+ public boolean hasChildren(final Resource resource) {
+ return listChildren(resource).hasNext();
+ }
+
+ @Override
+ public boolean isLive() {
+ return getDelegate().isLive();
+ }
+
+ @Override
+ public boolean isResourceType(final Resource resource, final String resourceType) {
+ return getDelegate().isResourceType(resource, resourceType);
+ }
+
+ @Override
+ public String map(final HttpServletRequest request, final String resourcePath) {
+ return getDelegate().map(request, resourcePath);
+ }
+
+ @Override
+ public String map(final String resourcePath) {
+ return getDelegate().map(resourcePath);
+ }
+
+ @Override
+ public Iterator<Map<String, Object>> queryResources(final String query, final String language) {
+ return getDelegate().queryResources(query, language);
+ }
+
+ @Override
+ public void refresh() {
+ getDelegate().refresh();
+ }
+
+ @Override
+ public Resource resolve(final String absPath) {
+ return getDelegate().resolve(absPath);
Review Comment:
Shouldn't the `resolve` methods wrap the resource? If you expect them never to get called you could also throw an `UnsupportedOperationException` to prevent bugs from creeping in.
##########
src/main/java/org/apache/sling/servlets/resolver/internal/ScriptResourceResolver.java:
##########
@@ -142,8 +143,160 @@ public Iterator<Resource> listChildren(ResolveContext<Object> ctx, Resource pare
}
}
- private Resource wrap(Resource resource) {
- if (resource != null && !(resource.getResourceResolver() instanceof ScriptResourceResolver)) {
+ protected final ResourceResolver getDelegate() {
+ return resolver;
+ }
+
+ @Override
+ public void close() {
+ getDelegate().close();
+ }
+
+ @Override
+ public void commit() throws PersistenceException {
+ getDelegate().commit();
+ }
+
+ @Override
+ public Resource create(final Resource parent, final String name, final Map<String, Object> properties) throws PersistenceException {
+ return getDelegate().create(parent, name, properties);
+ }
+
+ @Override
+ public boolean orderBefore(Resource resource, String s, String s1) throws UnsupportedOperationException, PersistenceException, IllegalArgumentException {
+ return getDelegate().orderBefore(resource, s, s1);
+ }
+
+ @Override
+ public void delete( final Resource resource) throws PersistenceException {
+ getDelegate().delete(resource);
+ }
+
+ @Override
+ public Iterator<Resource> findResources(final String query, final String language) {
+ return getDelegate().findResources(query, language);
+ }
+
+ @Override
+ public Object getAttribute(final String name) {
+ return getDelegate().getAttribute(name);
+ }
+
+ @Override
+ public Iterator<String> getAttributeNames() {
+ return getDelegate().getAttributeNames();
+ }
+
+ @Override
+ public String getParentResourceType(final Resource resource) {
+ return getDelegate().getParentResourceType(resource);
+ }
+
+ @Override
+ public String getParentResourceType(final String resourceType) {
+ return getDelegate().getParentResourceType(resourceType);
+ }
+
+ @Override
+ public String[] getSearchPath() {
+ return getDelegate().getSearchPath();
+ }
+
+ @Override
+ public String getUserID() {
+ return getDelegate().getUserID();
+ }
+
+ @Override
+ public boolean hasChanges() {
+ return getDelegate().hasChanges();
+ }
+
+ @Override
+ public boolean hasChildren(final Resource resource) {
+ return listChildren(resource).hasNext();
+ }
+
+ @Override
+ public boolean isLive() {
+ return getDelegate().isLive();
+ }
+
+ @Override
+ public boolean isResourceType(final Resource resource, final String resourceType) {
+ return getDelegate().isResourceType(resource, resourceType);
+ }
+
+ @Override
+ public String map(final HttpServletRequest request, final String resourcePath) {
+ return getDelegate().map(request, resourcePath);
+ }
+
+ @Override
+ public String map(final String resourcePath) {
+ return getDelegate().map(resourcePath);
+ }
+
+ @Override
+ public Iterator<Map<String, Object>> queryResources(final String query, final String language) {
+ return getDelegate().queryResources(query, language);
+ }
+
+ @Override
+ public void refresh() {
+ getDelegate().refresh();
+ }
+
+ @Override
+ public Resource resolve(final String absPath) {
+ return getDelegate().resolve(absPath);
+ }
+
+ @Override
+ @Deprecated
+ public Resource resolve(final HttpServletRequest request) {
+ return getDelegate().resolve(request);
+ }
+
+ @Override
+ public Resource resolve(final HttpServletRequest request, final String absPath) {
+ return getDelegate().resolve(request, absPath);
+ }
+
+ @Override
+ public void revert() {
+ getDelegate().revert();
+ }
+
+ @Override
+ public <AdapterType> AdapterType adaptTo(final Class<AdapterType> type) {
+ return getDelegate().adaptTo(type);
+ }
+
+ @Override
+ public Resource getParent(final Resource child) {
+ return getDelegate().getParent(child);
Review Comment:
Wrap the parent? Not sure, I guess it depends.
##########
src/main/java/org/apache/sling/servlets/resolver/internal/ScriptResourceResolver.java:
##########
@@ -173,14 +326,84 @@ public ScriptResourceResolver clone(Map<String, Object> o) throws LoginException
return ScriptResourceResolver.wrap(resolver.clone(o), providerSupplier);
}
- private class ScriptResourceResolverResourceWrapper extends ResourceWrapper {
+ private class ScriptResourceResolverResourceWrapper implements Resource {
+ private final Resource delegate;
public ScriptResourceResolverResourceWrapper(Resource resource) {
- super(resource);
+ this.delegate = resource;
+ }
+
+ Resource getDelegate() {
+ return delegate;
}
@Override
public ResourceResolver getResourceResolver() {
return ScriptResourceResolver.this;
}
+
+ @Override
+ public Resource getChild( final String relPath) {
+ return ScriptResourceResolver.this.getResource(this, relPath);
+ }
+
+ @Override
+ public Iterable<Resource> getChildren() {
+ return ScriptResourceResolver.this.getChildren(this);
+ }
+
+ @Override
+ public String getName() {
+ return this.delegate.getName();
Review Comment:
Either use `getDelegate()` consistently or if you consistently refer to the field, you may as well remove the method.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-servlets-resolver] karlpauls closed pull request #29: SLING-11373: wrap resources returned from decorator to have them appe…
Posted by GitBox <gi...@apache.org>.
karlpauls closed pull request #29: SLING-11373: wrap resources returned from decorator to have them appe…
URL: https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/29
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-servlets-resolver] cziegeler commented on pull request #29: SLING-11373: wrap resources returned from decorator to have them appe…
Posted by GitBox <gi...@apache.org>.
cziegeler commented on PR #29:
URL: https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/29#issuecomment-1152365688
it looks ok to me
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-servlets-resolver] sonarcloud[bot] commented on pull request #29: SLING-11373: wrap resources returned from decorator to have them appe…
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #29:
URL: https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/29#issuecomment-1152354059
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&metric=new_coverage&view=list) [12.3% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-servlets-resolver&pullRequest=29&metric=new_duplicated_lines_density&view=list)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org