Re: svn commit: r1452322 - in /sling/trunk/bundles: api/src/main/java/org/apache/sling/api/resource/ resourceresolver/ resourceresolver/src/main/java/org/apache/sling/resourceresolver/accessgate/ resourceresolver/src/main/java/org/apache/sling/resource...

[Felix Meschberger <fm...@adobe.com>]

Hi

Apart from some conceptual discussion which we might lead on the issue or on different threads in the list, here are some concrete comments to the commit:


Am 04.03.2013 um 15:23 schrieb <my...@apache.org> <my...@apache.org>:

> sling/trunk/bundles/api/src/main/java/org/apache/sling/api/resource/AccessGateException.java

If this exception is checked, it should probably extend from SecurityException.

>    sling/trunk/bundles/api/src/main/java/org/apache/sling/api/resource/ResourceAccessGate.java

JavaDoc missing ... particularly just from looking at the method, I don't understand what "sanitizeQuery" does. The name sounds strange ...

Where is this service implemented ?

>    sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/accessgate/
>    sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/accessgate/ResourceAccessGateHandler.java
>    sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/accessgate/ResourceAccessGateManager.java

Should these classes really be exported ? It looks wrong to export them, particularly because they are only used internally. 

Suggest to move them to impl.

ResourceAccessGateHandler should probably use Set<Operation> instead of List<Operation> to improve on "contains" and also because the indicated operations probably conceptually are sets of operations.

>    sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/accessgate/impl/
>    sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/accessgate/impl/AccessGateResourceDecorator.java

This class is confusing: It has a @Reference for ResourceAccessGateManager but no @Component but a constructor taking a ResourceAccessHateManagerTracker ...

Looks like the @Reference annotation is superfluous.

>    sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/accessgate/impl/AccessGateResourceWrapper.java

accessGatesForValues is unused -- what is it intended for ?

>    sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/accessgate/impl/ResourceAccessGateManagerImpl.java
>    

I have the impression that this does not need to be a service -- it does not even need to be a separate class: The singleton instance is created on first registration of a ResourceAccessGate and removed on unregistration of the last ResourceAccessGate. This singleton instance is then used by the singleton AccessGateResourceDecorator which is registered for the ResourceResolver to decorate resources.

Probably the AccessGateResourceDecorator could manage the ResourceAccessGate services itself and just register and unregister itself as a ResourceDecorator.


> sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/accessgate/impl/ResourceAccessGateManagerTracker.java

As per the above, this is not needed.

>    sling/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/accessgate/impl/ResourceAccessGateTracker.java
> Modified:
>    sling/trunk/bundles/resourceresolver/pom.xml

Regards
Felix