General Notes ------------------------------------------------------------------------------- - removed unused imports - replaced * imports - access modifiers within jackrabbits jcr-server in the past we had some discussions regarding public/protected access modifiers. i still prefer private implementation specific methods, since they allow the improve the implementation later on without running into compatibility issues with potential (mostly unknown) extensions. from my point of view an open source project should try to cover as much as possible by interfaces in order to provide a clear api. i prefer to review and discuss the api and the implementation (which is btw nowhere prefect) to making protected methods/fields. examples: DavResourceImpl#setJcrProperty and the recent suggestions how to handle PROPPATCH, the protected constructor with the DefaultIOManager :) ResourceFactoryImpl: ------------------------------------------------------------------------------- - perhaps we should rather create a new implementation and leave the original one for everyone that extended from it. - createResource(DavResourceLocator, DavServletRequest request, DavServletResponse) always builds a DavResourceImpl. However it should be possible to access a Version, VersionHistory as resource from a request -> missing impl. Note that DavResourceFactory#createResource(DavResourceLocator, DavSession) is mainly used from within the resources to build the collection and members. - createResource(DavResourceLocator, DavSession) > Method_Not_Allowed if item is not node -> not sure about this. the resource could represent a null resource representing a node that does not yet exists. Possible conflicts should then rather be detected by the repository. - getItem(DavSession, DavResourceLocator) > i would rather call this 'getNode' > missing check for required implementation of the DavSession? > i would catch the 'PathNotFoundException' which indicates the request to a 'null' resource within the method and let 'RepositoryException' indication some other error being thrown -> request/creation of resource fails. - private method createResourceForItem: i would rather name this one 'createResource' and pass the Node (or null). - last: i never liked the protected DavResourceImpl.setIsCollection' maybe we can have a separate constructor instead, that takes a flag (isCollection) if no Node exists. DavResourceImpl: ------------------------------------------------------------------------------- - public DavResourceImpl(DavResourceLocator locator, DavResourceFactory factory, DavSession session, ResourceConfig config, Item item) throws DavException > since the item being Node already has been asserted within ResourceFactoryImpl i would rather pass 'Node' In the 'jcr-server' this is different for there nodes represent collections whereas properties represent non-collection resources. > Perhaps should also throw IllegalArgumentException if locator or session or config is 'null'. thus, indicating that the call to the constructor was not correct (instead of 404 DavException) > i would leave the 'DavException' however for the check if the DavSession provided the proper impl. - instance fields: now that the DavResourceImpl definitely acts as base-class i think its important to mark fields final that may not be reset later on. - getJcrSession this method was private. is there a special reason why you need it protected? see general comments above. DeltaVResourceImpl ------------------------------------------------------------------------------- - initProperties: > should maybe do more than calling super.initProperties(). (in the second patch its removed, if i'm not mistaken). e.g. this one could add the 'supportedReportSet' to the properties. - initSupportedReports: > i think the minimal set of supported reports is only the DAV:expand-property report and not the 'locate-by-history' report. the latter may be available on any collection resource (adding check). > supportedReportSet property is never added to the available property-set (see above) > perhaps it would be better to change this method to 'getSupportedReportSet' and call it only when needed (upon Report/Propfind) instead of building the set in the constructor. - getOptionResponse: > ItemResourceConstants.VERSIONSTORAGE_PATH this is correct (the DAV:version-history-collection-set must point to some collection that may contain vhistories and /jcr:system/jcr:versionStorage is defined by JSR170). however, i would prefer not to add additional dependencies between the simple and the jcr-server. Actually i think this constant should be present in the o.a.j.JCRConstants.java where all the 'jcr:xyz' and 'nt:xyz' are present (commons package). > since the 'addWorkspace' is not supported and you did not include the 'DAV:workspace' properties (thus the complete workspace feature is missing), i would suggest to remove the check for DAV:workspace-collection-set. - getLocatorFromItemPath > rename to 'getLocatorFromNodePath' > could be private since not used outside of this class. - getLocatorFromItem > maybe 'getLocatorFromNode' would be a better name including changed signature to take Node instead of Item since in this implementation DavResources never represent javax.jcr.Property. - createResourceFromLocator > maybe not needed... - addHrefProperty(DavPropertyName, Node[], boolean) > i think, it would be better to replace this by 'getHrefProperty(DavPropertyName, Node[], boolean)' which returns the property instead of adding it directely to the prop-set. > there seems to be an error while building the href. it's a speciality of the original code, that Nodes are represented as collections and javax.jcr.Property objects aren't. in the 'simple' server this is different -> needs to check if the given node represents a collection or not. - getItemName(String) > is current used by VersionHistoryResourceImpl only. i would rather inline code with the vh-resource (or create a private method) and remove protected method from DeltaV-resource - isVersionControlled > this method is used by the VersionControledResourceImpl only therefore i'd prefer to move it to that class and make it private. - getReferenceResources > impl missing. proposed an impl. VersionControlledResourceImpl ------------------------------------------------------------------------------- - initProperties contains comments refering to the original implementation this method was copied from. - getVersionHistory: added check if factory really created a vh-resource VersionResourceImpl ------------------------------------------------------------------------------- - constructor > i don't think the distiction between the 'Version' node and its internals (like the frozen node) should be known from the outside (see ResourceFactoryImpl) > if a 'Version' in passed instead of 'Item' a single check for not being null would be sufficient. > 'setVersionNode' see below - 'setVersionNode': > see comments regarding final fields > what is the use case for this method? and what is the reason for making it protected? see also general statements regarding access modifiers below. > as stated with the resource-factory: i would rather not let the world know about the internal structure of the version. therefore i would pass the 'versionNode' to the super- constructor. TODO: all the particular logic, what version information is exposed and how is not adjusted to your original code yet. TODO: i'm not complete sure about whether and to which extend the version resource must expose its internal props (i.e. the nt:frozenNode in terms of JSR170)... maybe someone can shed some light on this? - 'getVersionNode' > why does 'getVersionNode' return Node and not Version? > i would prefer this method to be private (or remove it if not required). see also general statements regarding access modifiers below. - initProperties outdated comment refering to extra stuff from original code in 'jcr-server' impl... - getVersionHistory: added check if factory really created a vh-resource VersionHistoryResourceImpl ------------------------------------------------------------------------------- - getVersions added check if factory really created a v-resource