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 <[email protected]> <[email protected]>: > 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
