You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Alexander Klimetschek <ak...@adobe.com> on 2014/02/28 22:54:54 UTC

Resource merger service not as designed

Hi,

looking at SLING-3420 [1] I noticed that the ResourceMergerService [2] fails to follow the intended design, and explains some of the misunderstandings around the /mnt/overlay servlet discussion and now the modifying resource merger discussion.

The ResourceMergerService must have this one central API, where the caller = application defines the merge paths!

    Resource merge(ResourceResolver resolver, String mergeRootPath, String relativePath, String... mergePaths)

Example values for the use case of the overlay resource provider:
    resolver = based on the current request (user)
    mergeRootPath = /mnt/overlay
    relativePath = <dynamic part>, for example: "projectX/content/ui/toolbar"
    mergePaths = /apps, /libs

This was in the original patch [3], sadly removed and later added back but completely missing the point.

Currently it is all tied to the sling search path internally, but this would just be ONE of different options the application might want to chose for it.

The specific /mnt/overlay ResourcerProvider would take the sling search path and expose resources based on calling the merger service with this as the mergePaths. (Currently there is a single implementation MergedResourceProviderFactory for both ResourceMergerService and the ResourceProvider service).

This is simply a proper separation of concerns.

Another application with different merge paths would be to merge resource type hierarchies: use the resource type and its super types. This could allow one to put UI dialogs in the resource types (say at ./dialog) and then be able to extend dialogs in sub resource types using the merger service.

Now to the writeable discussion: the ResourceMergerService must be read-only. It can never make a decision where to write. Given that the merge path is a list with N entries and could be anything, the application only knows where to write. A logic such as "if it's not in /libs, save to /apps" doesn't work. If there is another search path entry (and sling apps have more), how do you know to use /apps1 or /apps2? Or if you use custom merge paths such as resource type hierarchy - there is no single answer. And sometimes you want to overwrite something at /libs. Only the application or the app developer can know the intended target.

Actually, the design was for UI definitions or other developer created content only. Thus there is usually no application code that would write to it, it would be handcrafted by a framework developer (libs) or an application developer (apps or wherever necessary). Maybe an advanded UI that shows you the inheritance and where things come from and can be stored, and the user can pick where to write it. This might need additions to the service, but that's not clear yet, unless someone starts to build something like that.

Sorry, I missed to review that earlier when the original patch was submitted, but I didn't find the time and assumed it was the above way. But this needs to be fixed :)

[1] https://issues.apache.org/jira/browse/SLING-3420
[2] http://svn.apache.org/repos/asf/sling/trunk/contrib/extensions/resourcemerger/src/main/java/org/apache/sling/resourcemerger/api/ResourceMergerService.java
[3] https://github.com/gknob/sling-resourcemerger/commit/a3e1b78c87e54cd5c32a58627a4de0420229e1f9

Cheers,
Alex

Re: Resource merger service not as designed

Posted by Alexander Klimetschek <ak...@adobe.com>.
On 01.03.2014, at 01:56, Carsten Ziegeler <cz...@apache.org> wrote:

> It is not correct, that we added a merge() method which "completely misses
> the point".

No, you did not add that merge() method at all :)

It's basically now the getMergedResource(Resource) method, which is FIXED to the sling search path. And this is the part that is wrong - it reduces the merger service to one use case only and doesn't allow reuse.

> The service has just path handling methods, everything else is
> done consistently in the resource provider. And it is done exactly as has
> been specified / requested in the original issue.

Ok, yes, the getResourcePath() method is not as I assumed it and described it in my mail - it just gives information back on what currently would be the real resource path used.

I think this would be better (and IIRC was in Gilles' original patch) to expose the MergedResource interface and provide it with the necessary "insight" methods. I.e. ResourceMergerService would return MergedResource directly in its getMergedResource() (current API) or merge() (my proposal).

> I think adding a merge() method which allows to create resources that are
> not available via a resource resolver will create all kinds of problems.
> Even worse, it allows you to create a resource at any arbitrary path like
> /libs/hello/world and pass this on to other services, totally confusing the
> whole system.

No:
- the resulting "virtual" path (say /mnt/overlay) would not and should never be the same as any existing one (/libs)
- the merger service itself doesn't hook in automatically anywhere; applications have to call it explicitly, and know what they are doing, which is perfectly fine here

> Such a resource is never available via the typical resource resolution - so
> all the concepts of Sling like the REST api for getting resources and
> manipulating them does absolutely not apply. Or even traversing based on
> such a resource is not possible.

Why is traversing not possible? Was that feature removed? It worked fine in Gilles original implementation.

> Or in short, such a service method is bypassing all of Sling's concepts.

I don't understand. The purpose is a generic merge mechanism. Sling search paths are just one way of doing this.

The main purpose is the "protocol" to merge resources, i.e. to patch something (prefer one over the other, delete one resource or property, replace property values, etc.). What list of resources to merge is a separate concern - left to the application.

Also, don't assume that the ResourceMerger service itself already is a ResourceProvider. One can implement a resource provider using the merger service (as done for /mnt/overlay) but it doesn't have to be that way. (Although it will likely be the typical case to do it via a resource provider, and then having multiple ones, say /mnt/overlay, /mnt/component-dialogs, ....).

> If an application really wants to create fake resources for whatever
> reason, the application is free to provide such a service. But I'm
> reluctant to add anything like that to Sling.

If you think this doesn't belong into Sling, then ok, let's move the resource merger API outside Sling.

Cheers,
Alex

Re: Resource merger service not as designed

Posted by Alexander Klimetschek <ak...@adobe.com>.
On 03.03.2014, at 23:05, Carsten Ziegeler <cz...@apache.org> wrote:

> We should not return fake resources from a service which are not available
> using the same path (the resource returns) via the resoruce resolver.

If it's provided by a resource provider, that will be the case. Although I don't think it's a big issue. The resource tree implicitly provided by the service is consistent.

Maybe we should just make it clear that you should normally not use the ResourceMergerService directly, but only inside a ResourceProvider. And application code would only use it via the mounted provider, say /mnt/overlay, never directly. The service is not intended to be an alternative to the resource resolver, in no way.

> Assume, you get a resource from the service, do a getParent() what resource
> does it return, especially if that doesn't exist?

This should be clearly defined by the merging logic.

> Or even if that resource
> exists in the resource tree, you get back a "real" resource, do a
> listChildren and the resource returned by the service is not in this list.

The design was (when I discussed it with Gilles) to always return a merged resource and never a real one directly. In order to handle proper tree traversal (with everything merged properly, including children and ancestors) this is a strict requirement anyway IIRC.

> Finally, this mechanism bypasses all other Sling mechanisms like resource
> decorators, the access security gate, feature flags (and there might be
> more).

Yes (if you just look at the merger; and this is perfectly fine, I don't see why you want to mix different concerns here), no (if it is used via resource provider).

> If you provide a generic service which gets a mount path, search paths etc.
> as parameters, this is open to any value. First allowing all values, and
> then checking them within the service and potentially rejecting values does
> not seem like a good idea. For example, what if the client passes in
> /libs/bla, the current resource resolver has no access to this resource and
> therefore it can be used as it is a non existing path? There are all kinds
> of problems.

You would of course only use it properly. The same argument can be made for ResourceProviders etc. - I can implement them by using an admin resource resolver and thus exposing the entire existing tree. That doesn't make sense either.

> Maybe if you describe your exact use case, it would help to understand why
> you need this.

I think I explained it enough in this thread and before. It's about extending or overlaying things by specifying a delta/patch, not by copying content. This is a generic principle that can be done on the resource level instead of implementing it again and again for different application cases. This concept is not restricted to the sling search path.

Cheers,
Alex

Re: Resource merger service not as designed

Posted by Carsten Ziegeler <cz...@apache.org>.
We should not return fake resources from a service which are not available
using the same path (the resource returns) via the resoruce resolver.
Assume, you get a resource from the service, do a getParent() what resource
does it return, especially if that doesn't exist? Or even if that resource
exists in the resource tree, you get back a "real" resource, do a
listChildren and the resource returned by the service is not in this list.
Finally, this mechanism bypasses all other Sling mechanisms like resource
decorators, the access security gate, feature flags (and there might be
more).

If you provide a generic service which gets a mount path, search paths etc.
as parameters, this is open to any value. First allowing all values, and
then checking them within the service and potentially rejecting values does
not seem like a good idea. For example, what if the client passes in
/libs/bla, the current resource resolver has no access to this resource and
therefore it can be used as it is a non existing path? There are all kinds
of problems.

Maybe if you describe your exact use case, it would help to understand why
you need this.

Regards
Carsten


2014-03-03 23:23 GMT+01:00 Alexander Klimetschek <ak...@adobe.com>:

> On 02.03.2014, at 23:05, Carsten Ziegeler <cz...@apache.org> wrote:
>
> > public MergeData merge(String[] paths)
>
> But why not make it implement Resource then (as it already does)?
>
> > Having a method which gets a relative path together with the search paths
>
> The concept of "root path" plus "relative path" plus "list of merge paths"
> is generic. The relative path logic can be very different from the one used
> for the search paths today (this might the cause of some confusion).
>
> Cheers,
> Alex




-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Resource merger service not as designed

Posted by Alexander Klimetschek <ak...@adobe.com>.
On 02.03.2014, at 23:05, Carsten Ziegeler <cz...@apache.org> wrote:

> public MergeData merge(String[] paths)

But why not make it implement Resource then (as it already does)?

> Having a method which gets a relative path together with the search paths

The concept of "root path" plus "relative path" plus "list of merge paths" is generic. The relative path logic can be very different from the one used for the search paths today (this might the cause of some confusion).

Cheers,
Alex

Re: Resource merger service not as designed

Posted by Carsten Ziegeler <cz...@apache.org>.
As a compromise we could provide a method that returns some data structure
like:

public interface MergeData {
     String[] getMergedPaths();
     ValueMap getMergedValueMap();
}


with a method

public MergeData merge(String[] paths)

Having a method which gets a relative path together with the search paths
as suggested is not required as such resources are delivered by the
resource provider.

Regards
Carsten


2014-03-01 10:56 GMT+01:00 Carsten Ziegeler <cz...@apache.org>:

> Ok, let's please not put all different topics into a single mail and lets
> stick to the facts.
>
> As noted in the issues about supporting the ModifyingResourceProvider - I
> reverted this and added a simple path utility method which gives full
> control to the application and does no modifications. So there is zero
> modification logic in the bundle.
>
> It is not correct, that we added a merge() method which "completely misses
> the point". The service has just path handling methods, everything else is
> done consistently in the resource provider. And it is done exactly as has
> been specified / requested in the original issue.
>
> I think adding a merge() method which allows to create resources that are
> not available via a resource resolver will create all kinds of problems.
> Even worse, it allows you to create a resource at any arbitrary path like
> /libs/hello/world and pass this on to other services, totally confusing the
> whole system.
> Such a resource is never available via the typical resource resolution -
> so all the concepts of Sling like the REST api for getting resources and
> manipulating them does absolutely not apply. Or even traversing based on
> such a resource is not possible.
>
> Or in short, such a service method is bypassing all of Sling's concepts.
>
> For the real use cases mentioned so far, the resource provider approach
> works perfectly - especially as this is using Sling concepts and not adding
> additional logic on top. The idea with supporting the
> ModifyingResourceProvider might be a little bit over the top and is now
> discarded anyway.
>
> If an application really wants to create fake resources for whatever
> reason, the application is free to provide such a service. But I'm
> reluctant to add anything like that to Sling.
>
> Carsten
>
>
>
>
> 2014-02-28 23:56 GMT+01:00 Gilles Knobloch <gi...@gmail.com>:
>
> Hi,
>>
>> I would also +1 for having a method that does:
>>   Resource merge(String... paths);
>>
>> Since I'm using it, I already discovered some use cases where I'd like to
>> be able to have it, like shared content between two applications, one
>> being
>> the framework on which the other is based, where you want to be able to
>> merge content without having to redefine everything in the downstream
>> application.
>> This method would give the flexibility to merge any resources.
>>
>> The resource resolver should then be based on the service by calling the
>> merge method with the list of paths resolved by the resource resolver.
>>
>> Regards,
>> Gilles
>>
>>
>> 2014-02-28 23:05 GMT+01:00 Alexander Klimetschek <ak...@adobe.com>:
>>
>> > I created https://issues.apache.org/jira/browse/SLING-3423
>> >
>> > Cheers,
>> > Alex
>> >
>> > On 28.02.2014, at 13:54, Alexander Klimetschek <ak...@adobe.com>
>> wrote:
>> >
>> > > Hi,
>> > >
>> > > looking at SLING-3420 [1] I noticed that the ResourceMergerService [2]
>> > fails to follow the intended design, and explains some of the
>> > misunderstandings around the /mnt/overlay servlet discussion and now the
>> > modifying resource merger discussion.
>> > >
>> > > The ResourceMergerService must have this one central API, where the
>> > caller = application defines the merge paths!
>> > >
>> > >    Resource merge(ResourceResolver resolver, String mergeRootPath,
>> > String relativePath, String... mergePaths)
>> > >
>> > > Example values for the use case of the overlay resource provider:
>> > >    resolver = based on the current request (user)
>> > >    mergeRootPath = /mnt/overlay
>> > >    relativePath = <dynamic part>, for example:
>> > "projectX/content/ui/toolbar"
>> > >    mergePaths = /apps, /libs
>> > >
>> > > This was in the original patch [3], sadly removed and later added back
>> > but completely missing the point.
>> > >
>> > > Currently it is all tied to the sling search path internally, but this
>> > would just be ONE of different options the application might want to
>> chose
>> > for it.
>> > >
>> > > The specific /mnt/overlay ResourcerProvider would take the sling
>> search
>> > path and expose resources based on calling the merger service with this
>> as
>> > the mergePaths. (Currently there is a single implementation
>> > MergedResourceProviderFactory for both ResourceMergerService and the
>> > ResourceProvider service).
>> > >
>> > > This is simply a proper separation of concerns.
>> > >
>> > > Another application with different merge paths would be to merge
>> > resource type hierarchies: use the resource type and its super types.
>> This
>> > could allow one to put UI dialogs in the resource types (say at
>> ./dialog)
>> > and then be able to extend dialogs in sub resource types using the
>> merger
>> > service.
>> > >
>> > > Now to the writeable discussion: the ResourceMergerService must be
>> > read-only. It can never make a decision where to write. Given that the
>> > merge path is a list with N entries and could be anything, the
>> application
>> > only knows where to write. A logic such as "if it's not in /libs, save
>> to
>> > /apps" doesn't work. If there is another search path entry (and sling
>> apps
>> > have more), how do you know to use /apps1 or /apps2? Or if you use
>> custom
>> > merge paths such as resource type hierarchy - there is no single answer.
>> > And sometimes you want to overwrite something at /libs. Only the
>> > application or the app developer can know the intended target.
>> > >
>> > > Actually, the design was for UI definitions or other developer created
>> > content only. Thus there is usually no application code that would
>> write to
>> > it, it would be handcrafted by a framework developer (libs) or an
>> > application developer (apps or wherever necessary). Maybe an advanded UI
>> > that shows you the inheritance and where things come from and can be
>> > stored, and the user can pick where to write it. This might need
>> additions
>> > to the service, but that's not clear yet, unless someone starts to build
>> > something like that.
>> > >
>> > > Sorry, I missed to review that earlier when the original patch was
>> > submitted, but I didn't find the time and assumed it was the above way.
>> But
>> > this needs to be fixed :)
>> > >
>> > > [1] https://issues.apache.org/jira/browse/SLING-3420
>> > > [2]
>> >
>> http://svn.apache.org/repos/asf/sling/trunk/contrib/extensions/resourcemerger/src/main/java/org/apache/sling/resourcemerger/api/ResourceMergerService.java
>> > > [3]
>> >
>> https://github.com/gknob/sling-resourcemerger/commit/a3e1b78c87e54cd5c32a58627a4de0420229e1f9
>> > >
>> > > Cheers,
>> > > Alex
>> >
>> >
>>
>
>
>
> --
> Carsten Ziegeler
> cziegeler@apache.org
>



-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Resource merger service not as designed

Posted by Carsten Ziegeler <cz...@apache.org>.
Ok, let's please not put all different topics into a single mail and lets
stick to the facts.

As noted in the issues about supporting the ModifyingResourceProvider - I
reverted this and added a simple path utility method which gives full
control to the application and does no modifications. So there is zero
modification logic in the bundle.

It is not correct, that we added a merge() method which "completely misses
the point". The service has just path handling methods, everything else is
done consistently in the resource provider. And it is done exactly as has
been specified / requested in the original issue.

I think adding a merge() method which allows to create resources that are
not available via a resource resolver will create all kinds of problems.
Even worse, it allows you to create a resource at any arbitrary path like
/libs/hello/world and pass this on to other services, totally confusing the
whole system.
Such a resource is never available via the typical resource resolution - so
all the concepts of Sling like the REST api for getting resources and
manipulating them does absolutely not apply. Or even traversing based on
such a resource is not possible.

Or in short, such a service method is bypassing all of Sling's concepts.

For the real use cases mentioned so far, the resource provider approach
works perfectly - especially as this is using Sling concepts and not adding
additional logic on top. The idea with supporting the
ModifyingResourceProvider might be a little bit over the top and is now
discarded anyway.

If an application really wants to create fake resources for whatever
reason, the application is free to provide such a service. But I'm
reluctant to add anything like that to Sling.

Carsten




2014-02-28 23:56 GMT+01:00 Gilles Knobloch <gi...@gmail.com>:

> Hi,
>
> I would also +1 for having a method that does:
>   Resource merge(String... paths);
>
> Since I'm using it, I already discovered some use cases where I'd like to
> be able to have it, like shared content between two applications, one being
> the framework on which the other is based, where you want to be able to
> merge content without having to redefine everything in the downstream
> application.
> This method would give the flexibility to merge any resources.
>
> The resource resolver should then be based on the service by calling the
> merge method with the list of paths resolved by the resource resolver.
>
> Regards,
> Gilles
>
>
> 2014-02-28 23:05 GMT+01:00 Alexander Klimetschek <ak...@adobe.com>:
>
> > I created https://issues.apache.org/jira/browse/SLING-3423
> >
> > Cheers,
> > Alex
> >
> > On 28.02.2014, at 13:54, Alexander Klimetschek <ak...@adobe.com>
> wrote:
> >
> > > Hi,
> > >
> > > looking at SLING-3420 [1] I noticed that the ResourceMergerService [2]
> > fails to follow the intended design, and explains some of the
> > misunderstandings around the /mnt/overlay servlet discussion and now the
> > modifying resource merger discussion.
> > >
> > > The ResourceMergerService must have this one central API, where the
> > caller = application defines the merge paths!
> > >
> > >    Resource merge(ResourceResolver resolver, String mergeRootPath,
> > String relativePath, String... mergePaths)
> > >
> > > Example values for the use case of the overlay resource provider:
> > >    resolver = based on the current request (user)
> > >    mergeRootPath = /mnt/overlay
> > >    relativePath = <dynamic part>, for example:
> > "projectX/content/ui/toolbar"
> > >    mergePaths = /apps, /libs
> > >
> > > This was in the original patch [3], sadly removed and later added back
> > but completely missing the point.
> > >
> > > Currently it is all tied to the sling search path internally, but this
> > would just be ONE of different options the application might want to
> chose
> > for it.
> > >
> > > The specific /mnt/overlay ResourcerProvider would take the sling search
> > path and expose resources based on calling the merger service with this
> as
> > the mergePaths. (Currently there is a single implementation
> > MergedResourceProviderFactory for both ResourceMergerService and the
> > ResourceProvider service).
> > >
> > > This is simply a proper separation of concerns.
> > >
> > > Another application with different merge paths would be to merge
> > resource type hierarchies: use the resource type and its super types.
> This
> > could allow one to put UI dialogs in the resource types (say at ./dialog)
> > and then be able to extend dialogs in sub resource types using the merger
> > service.
> > >
> > > Now to the writeable discussion: the ResourceMergerService must be
> > read-only. It can never make a decision where to write. Given that the
> > merge path is a list with N entries and could be anything, the
> application
> > only knows where to write. A logic such as "if it's not in /libs, save to
> > /apps" doesn't work. If there is another search path entry (and sling
> apps
> > have more), how do you know to use /apps1 or /apps2? Or if you use custom
> > merge paths such as resource type hierarchy - there is no single answer.
> > And sometimes you want to overwrite something at /libs. Only the
> > application or the app developer can know the intended target.
> > >
> > > Actually, the design was for UI definitions or other developer created
> > content only. Thus there is usually no application code that would write
> to
> > it, it would be handcrafted by a framework developer (libs) or an
> > application developer (apps or wherever necessary). Maybe an advanded UI
> > that shows you the inheritance and where things come from and can be
> > stored, and the user can pick where to write it. This might need
> additions
> > to the service, but that's not clear yet, unless someone starts to build
> > something like that.
> > >
> > > Sorry, I missed to review that earlier when the original patch was
> > submitted, but I didn't find the time and assumed it was the above way.
> But
> > this needs to be fixed :)
> > >
> > > [1] https://issues.apache.org/jira/browse/SLING-3420
> > > [2]
> >
> http://svn.apache.org/repos/asf/sling/trunk/contrib/extensions/resourcemerger/src/main/java/org/apache/sling/resourcemerger/api/ResourceMergerService.java
> > > [3]
> >
> https://github.com/gknob/sling-resourcemerger/commit/a3e1b78c87e54cd5c32a58627a4de0420229e1f9
> > >
> > > Cheers,
> > > Alex
> >
> >
>



-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Resource merger service not as designed

Posted by Gilles Knobloch <gi...@gmail.com>.
Hi,

I would also +1 for having a method that does:
  Resource merge(String... paths);

Since I'm using it, I already discovered some use cases where I'd like to
be able to have it, like shared content between two applications, one being
the framework on which the other is based, where you want to be able to
merge content without having to redefine everything in the downstream
application.
This method would give the flexibility to merge any resources.

The resource resolver should then be based on the service by calling the
merge method with the list of paths resolved by the resource resolver.

Regards,
Gilles


2014-02-28 23:05 GMT+01:00 Alexander Klimetschek <ak...@adobe.com>:

> I created https://issues.apache.org/jira/browse/SLING-3423
>
> Cheers,
> Alex
>
> On 28.02.2014, at 13:54, Alexander Klimetschek <ak...@adobe.com> wrote:
>
> > Hi,
> >
> > looking at SLING-3420 [1] I noticed that the ResourceMergerService [2]
> fails to follow the intended design, and explains some of the
> misunderstandings around the /mnt/overlay servlet discussion and now the
> modifying resource merger discussion.
> >
> > The ResourceMergerService must have this one central API, where the
> caller = application defines the merge paths!
> >
> >    Resource merge(ResourceResolver resolver, String mergeRootPath,
> String relativePath, String... mergePaths)
> >
> > Example values for the use case of the overlay resource provider:
> >    resolver = based on the current request (user)
> >    mergeRootPath = /mnt/overlay
> >    relativePath = <dynamic part>, for example:
> "projectX/content/ui/toolbar"
> >    mergePaths = /apps, /libs
> >
> > This was in the original patch [3], sadly removed and later added back
> but completely missing the point.
> >
> > Currently it is all tied to the sling search path internally, but this
> would just be ONE of different options the application might want to chose
> for it.
> >
> > The specific /mnt/overlay ResourcerProvider would take the sling search
> path and expose resources based on calling the merger service with this as
> the mergePaths. (Currently there is a single implementation
> MergedResourceProviderFactory for both ResourceMergerService and the
> ResourceProvider service).
> >
> > This is simply a proper separation of concerns.
> >
> > Another application with different merge paths would be to merge
> resource type hierarchies: use the resource type and its super types. This
> could allow one to put UI dialogs in the resource types (say at ./dialog)
> and then be able to extend dialogs in sub resource types using the merger
> service.
> >
> > Now to the writeable discussion: the ResourceMergerService must be
> read-only. It can never make a decision where to write. Given that the
> merge path is a list with N entries and could be anything, the application
> only knows where to write. A logic such as "if it's not in /libs, save to
> /apps" doesn't work. If there is another search path entry (and sling apps
> have more), how do you know to use /apps1 or /apps2? Or if you use custom
> merge paths such as resource type hierarchy - there is no single answer.
> And sometimes you want to overwrite something at /libs. Only the
> application or the app developer can know the intended target.
> >
> > Actually, the design was for UI definitions or other developer created
> content only. Thus there is usually no application code that would write to
> it, it would be handcrafted by a framework developer (libs) or an
> application developer (apps or wherever necessary). Maybe an advanded UI
> that shows you the inheritance and where things come from and can be
> stored, and the user can pick where to write it. This might need additions
> to the service, but that's not clear yet, unless someone starts to build
> something like that.
> >
> > Sorry, I missed to review that earlier when the original patch was
> submitted, but I didn't find the time and assumed it was the above way. But
> this needs to be fixed :)
> >
> > [1] https://issues.apache.org/jira/browse/SLING-3420
> > [2]
> http://svn.apache.org/repos/asf/sling/trunk/contrib/extensions/resourcemerger/src/main/java/org/apache/sling/resourcemerger/api/ResourceMergerService.java
> > [3]
> https://github.com/gknob/sling-resourcemerger/commit/a3e1b78c87e54cd5c32a58627a4de0420229e1f9
> >
> > Cheers,
> > Alex
>
>

Re: Resource merger service not as designed

Posted by Alexander Klimetschek <ak...@adobe.com>.
I created https://issues.apache.org/jira/browse/SLING-3423

Cheers,
Alex

On 28.02.2014, at 13:54, Alexander Klimetschek <ak...@adobe.com> wrote:

> Hi,
> 
> looking at SLING-3420 [1] I noticed that the ResourceMergerService [2] fails to follow the intended design, and explains some of the misunderstandings around the /mnt/overlay servlet discussion and now the modifying resource merger discussion.
> 
> The ResourceMergerService must have this one central API, where the caller = application defines the merge paths!
> 
>    Resource merge(ResourceResolver resolver, String mergeRootPath, String relativePath, String... mergePaths)
> 
> Example values for the use case of the overlay resource provider:
>    resolver = based on the current request (user)
>    mergeRootPath = /mnt/overlay
>    relativePath = <dynamic part>, for example: "projectX/content/ui/toolbar"
>    mergePaths = /apps, /libs
> 
> This was in the original patch [3], sadly removed and later added back but completely missing the point.
> 
> Currently it is all tied to the sling search path internally, but this would just be ONE of different options the application might want to chose for it.
> 
> The specific /mnt/overlay ResourcerProvider would take the sling search path and expose resources based on calling the merger service with this as the mergePaths. (Currently there is a single implementation MergedResourceProviderFactory for both ResourceMergerService and the ResourceProvider service).
> 
> This is simply a proper separation of concerns.
> 
> Another application with different merge paths would be to merge resource type hierarchies: use the resource type and its super types. This could allow one to put UI dialogs in the resource types (say at ./dialog) and then be able to extend dialogs in sub resource types using the merger service.
> 
> Now to the writeable discussion: the ResourceMergerService must be read-only. It can never make a decision where to write. Given that the merge path is a list with N entries and could be anything, the application only knows where to write. A logic such as "if it's not in /libs, save to /apps" doesn't work. If there is another search path entry (and sling apps have more), how do you know to use /apps1 or /apps2? Or if you use custom merge paths such as resource type hierarchy - there is no single answer. And sometimes you want to overwrite something at /libs. Only the application or the app developer can know the intended target.
> 
> Actually, the design was for UI definitions or other developer created content only. Thus there is usually no application code that would write to it, it would be handcrafted by a framework developer (libs) or an application developer (apps or wherever necessary). Maybe an advanded UI that shows you the inheritance and where things come from and can be stored, and the user can pick where to write it. This might need additions to the service, but that's not clear yet, unless someone starts to build something like that.
> 
> Sorry, I missed to review that earlier when the original patch was submitted, but I didn't find the time and assumed it was the above way. But this needs to be fixed :)
> 
> [1] https://issues.apache.org/jira/browse/SLING-3420
> [2] http://svn.apache.org/repos/asf/sling/trunk/contrib/extensions/resourcemerger/src/main/java/org/apache/sling/resourcemerger/api/ResourceMergerService.java
> [3] https://github.com/gknob/sling-resourcemerger/commit/a3e1b78c87e54cd5c32a58627a4de0420229e1f9
> 
> Cheers,
> Alex