[
https://issues.apache.org/jira/browse/SLING-2853?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13649346#comment-13649346
]
Alexander Klimetschek edited comment on SLING-2853 at 5/5/13 2:52 PM:
----------------------------------------------------------------------
Here's my review of the patch from the Star Wars day ;-)
Content Structure:
- my proposal, see below for discussion
(legend: + = node, - = property, [nodetype], # comment)
+ <name> [nt:unstructured] # but node type should be free
- sling:resourceType = "sling/collection"
- key = "value" # collection-level metadata
+ thumbnail # metadata with child resources/nodes
- ...
...
+ members [nt:unstructured] # optional?
- sling:resources = ["/one", "/two", "/three"]
+ member1 [nt:unstructured]
- sling:resource = "/one"
- key = "value" # reference-level metadata
...
+ member2
- sling:resource = "/three"
- key = "value"
...
- The node type on the collection node should not be restricted to allow
custom metadata having child nodes with any type. For example, a child
node of type nt:file would not work within a nt:unstructured. This
requires the create collection API to allow for a node type being passed.
Also we have a so-called replication mechanism for individual nodes that
depends on certain node types, so in some case you might want this type
and in others another one.
- I think the "members" node could be left out completely. Nesting of
collections isn't really something that makes sense. Leaving it out
has only the disadvantage that custom metadata that needs child resources
cannot use any of the "member1", "member2" ... node names, instead of
just "members" as an excluded name. For simple collections without any
metadata this would truly optimize the structure to the minimum.
But I am not fully decided on that.
- The properties should be namespaced, since they could overlap with custom
properties. I guess "sling:" is the namespace to use.
- I would prefer naming the property "path" different, like "sling:resource" or
"sling:reference". While the content of the property is a path now, it might
be something else in the future (like a http url, uuid, etc.), and it
actually addresses a resource or reference to a resource. "resource"
would also be in line with the getResources() API name.
- Similarly, "pathorder" is a suboptimal name. It's not camel-cased and
as above, "sling:resources" or "sling:references" sounds better. That it's
ordered doesn't have to be included in the name.
- Moreover, I think that multi-value property should define the whole
references list. So it would not be required that there is actually a
child resource (entry) being present (applies to getEntries()) to
return that resource in the list. The child resource would only be
needed in case there are additional properties set.
API:
- ResourceCollection: For the implementation reading that list (getEntries())
the point above would mean no need to iterate over the child resources at all.
There needs to be some way to get the metadata, if there is some:
+ get properties for one reference
ValueMap getProperties(String reference) or (Resource resource)
+ optional, to batch read everything:
Iterator<Entry> getEntries()
Entry = simple struct-like class with Resource (the referenced
resource) and a ValueMap or ModifiableValueMap
- ResourceCollection.getEntries()
I don't think if getEntries() separate from getResources() is really needed,
as this exposes too much of the inner content structure in the API.
As proposed above, I see an optimization here where there might not be
actual child resources for every single reference defined in the array
property.
- ResourceCollection.DEFAULT_SLING_RES_TYPE rename to simply RESOURCE_TYPE
The "default" and "sling" in it is misleading; it is *the* rt for the
ResourceCollection,
and I guess it's rather unlikely that someone extends the resource collection
rt,
let alone being a central feature to be pointed out.
- ResourceCollectionManager methods should IMHO all be consistently based on
passing Resource instead of path (makes e.g. the createCollection()
implementation
simpler in that it does not have to getResource() and check for existence)
- I suppose a common use case will be a getOrCreateCollection() that gets an
existing one or creates it if it doesn't exist, would provide that right away.
- ResourceCollectionManager.deleteCollection() - agree with Bertrand, not
needed.
A getResource() on the collection interface would also make getName() and
getPath()
there obsolete. Maybe one could have the ResourceCollection extend from
Resource?
- The ResourceCollectionManager is really only needed to create a new collection
(get and delete can be done via adaptTo and Resource directly). That somewhat
bloats API client code a bit, "only" getting the manager for creation. But
I don't know what is better - a static create method on ResourceCollection
would
break into the API and don't see other ways.
Do we have any precedence in Sling with code like this, creating content
structures on top of the resource API?
Implementation:
- ResourceCollectionManagerImpl.createCollection() could maybe use
ResourceUtil.normalize() on the final concatenated path (solved if using
Resource
instead of path as argument)
- ResourceCollectionManagerImpl.getCollection relies on adaptTo() in
getCollection()
It is good practice to not rely on the adaptTo mechanism inside a bundle
itself
(for the classes that it provides itself). This can quickly run into a
chicken-and-egg situation on startups/bundle activations, since the
AdapterFactory
of the bundle might get activated later than the bundle code being available,
and then the adaptTo() returns null (had this case already, tricky to find).
It's also a lot of intermediate steps through the whole adapter mechanism and
also
potentially allows another adapterfactory to jump in here and return
something else.
The "right" code to use instead is already inside the
ResourceCollectionAdapterFactory.
An AdapterFactory should only be seen as a helper on top of the bundle, not an
integral part of the bundle's inner workings.
- ResourceCollectionAdapterFactory adapts to a collection only if the "members"
child
is present. IMHO the resource type should be enough. If there are no members
yet, the
node could be missing. Any code reading content should be very liberal in
what it
expects, and empty content should always work. So all writing methods should
have
a step in them to create the members node if not present yet.
- Resource type: ResourceCollectionAdapterFactory should use
Resource.isResourceType() for
the type/super type check.
But ;-) I think we don't need to support super types as it's
very unclear yet how an extended collection resource type would work (another
collection
API taking over? what would be added?). Explicitly not supporting extension
feels
safer to me for now - if that need comes up, it can still be added. This
means:
+ adpterfactory: only match if rt == collection
+ create collection: force rt == collection (not allowing anything else to be
set
via the properties map); no need to check super type in the properties
was (Author: alexander.klimetschek):
Here's my review of the patch from the Star Wars day ;-)
Content Structure:
- my proposal, see below for discussion
(legend: + = node, - = property, [nodetype])
+ <name> [nt:unstructured] # but node type should be free
- sling:resourceType = "sling/collection"
- key = "value" # collection-level metadata
+ thumbnail # metadata with child resources/nodes
- ...
...
+ members [nt:unstructured] # optional?
- sling:resources = ["/one", "/two", "/three"]
+ member1 [nt:unstructured]
- sling:resource = "/one"
- key = "value" # reference-level metadata
...
+ member2
- sling:resource = "/three"
- key = "value"
...
- The node type on the collection node should not be restricted to allow
custom metadata having child nodes with any type. For example, a child
node of type nt:file would not work within a nt:unstructured. This
requires the create collection API to allow for a node type being passed.
Also we have a so-called replication mechanism for individual nodes that
depends on certain node types, so in some case you might want this type
and in others another one.
- I think the "members" node could be left out completely. Nesting of
collections isn't really something that makes sense. Leaving it out
has only the disadvantage that custom metadata that needs child resources
cannot use any of the "member1", "member2" ... node names, instead of
just "members" as an excluded name. For simple collections without any
metadata this would truly optimize the structure to the minimum.
But I am not fully decided on that.
- The properties should be namespaced, since they could overlap with custom
properties. I guess "sling:" is the namespace to use.
- I would prefer naming the property "path" different, like "sling:resource" or
"sling:reference". While the content of the property is a path now, it might
be something else in the future (like a http url, uuid, etc.), and it
actually addresses a resource or reference to a resource. "resource"
would also be in line with the getResources() API name.
- Similarly, "pathorder" is a suboptimal name. It's not camel-cased and
as above, "sling:resources" or "sling:references" sounds better. That it's
ordered doesn't have to be included in the name.
- Moreover, I think that multi-value property should define the whole
references list. So it would not be required that there is actually a
child resource (entry) being present (applies to getEntries()) to
return that resource in the list. The child resource would only be
needed in case there are additional properties set.
API:
- ResourceCollection: For the implementation reading that list (getEntries())
the point above would mean no need to iterate over the child resources at all.
There needs to be some way to get the metadata, if there is some:
+ get properties for one reference
ValueMap getProperties(String reference) or (Resource resource)
+ optional, to batch read everything:
Iterator<Entry> getEntries()
Entry = simple struct-like class with Resource (the referenced
resource) and a ValueMap or ModifiableValueMap
- ResourceCollection.getEntries()
I don't think if getEntries() separate from getResources() is really needed,
as this exposes too much of the inner content structure in the API.
As proposed above, I see an optimization here where there might not be
actual child resources for every single reference defined in the array
property.
- ResourceCollection.DEFAULT_SLING_RES_TYPE rename to simply RESOURCE_TYPE
The "default" and "sling" in it is misleading; it is *the* rt for the
ResourceCollection,
and I guess it's rather unlikely that someone extends the resource collection
rt,
let alone being a central feature to be pointed out.
- ResourceCollectionManager methods should IMHO all be consistently based on
passing Resource instead of path (makes e.g. the createCollection()
implementation
simpler in that it does not have to getResource() and check for existence)
- I suppose a common use case will be a getOrCreateCollection() that gets an
existing one or creates it if it doesn't exist, would provide that right away.
- ResourceCollectionManager.deleteCollection() - agree with Bertrand, not
needed.
A getResource() on the collection interface would also make getName() and
getPath()
there obsolete. Maybe one could have the ResourceCollection extend from
Resource?
- The ResourceCollectionManager is really only needed to create a new collection
(get and delete can be done via adaptTo and Resource directly). That somewhat
bloats API client code a bit, "only" getting the manager for creation. But
I don't know what is better - a static create method on ResourceCollection
would
break into the API and don't see other ways.
Do we have any precedence in Sling with code like this, creating content
structures on top of the resource API?
Implementation:
- ResourceCollectionManagerImpl.createCollection() could maybe use
ResourceUtil.normalize() on the final concatenated path (solved if using
Resource
instead of path as argument)
- ResourceCollectionManagerImpl.getCollection relies on adaptTo() in
getCollection()
It is good practice to not rely on the adaptTo mechanism inside a bundle
itself
(for the classes that it provides itself). This can quickly run into a
chicken-and-egg situation on startups/bundle activations, since the
AdapterFactory
of the bundle might get activated later than the bundle code being available,
and then the adaptTo() returns null (had this case already, tricky to find).
It's also a lot of intermediate steps through the whole adapter mechanism and
also
potentially allows another adapterfactory to jump in here and return
something else.
The "right" code to use instead is already inside the
ResourceCollectionAdapterFactory.
An AdapterFactory should only be seen as a helper on top of the bundle, not an
integral part of the bundle's inner workings.
- ResourceCollectionAdapterFactory adapts to a collection only if the "members"
child
is present. IMHO the resource type should be enough. If there are no members
yet, the
node could be missing. Any code reading content should be very liberal in
what it
expects, and empty content should always work. So all writing methods should
have
a step in them to create the members node if not present yet.
- Resource type: ResourceCollectionAdapterFactory should use
Resource.isResourceType() for
the type/super type check.
But ;-) I think we don't need to support super types as it's
very unclear yet how an extended collection resource type would work (another
collection
API taking over? what would be added?). Explicitly not supporting extension
feels
safer to me for now - if that need comes up, it can still be added. This
means:
+ adpterfactory: only match if rt == collection
+ create collection: force rt == collection (not allowing anything else to be
set
via the properties map); no need to check super type in the properties
> Add ResourceCollection to Sling
> -------------------------------
>
> Key: SLING-2853
> URL: https://issues.apache.org/jira/browse/SLING-2853
> Project: Sling
> Issue Type: New Feature
> Components: API
> Affects Versions: API 2.0.2
> Reporter: Amit Gupta
> Priority: Minor
> Attachments: collection.zip, resourcecollection.zip
>
>
> Creating a collection of resources has been a use case for a while and there
> has been no inherent support in SLING for the same.
> This proposal is to add a ResourceCollection interface and implementation
> that allows creation of collection of resources.
> Collection is a simple list of members, where each member contains path of
> resource it refers to. In future, we might need to store additional
> information with the member, hence following structure is proposed
> N: resourceCollection (nt:unstructured)
> + P: sling:resourceType
> + N : members (nt:unstructured)
> + N: member_res1 > nt:unstructured
> + P: path > string, reference to actual resource
> + N: member_res2 > nt:unstructured
> + P: path > string, reference to actual resource
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
