Workspace related functionality

[Carsten Ziegeler <cz...@apache.org>]

Hi,

as recently discussed, I changed the implementation of the (jcr)
resource resolver (and factory) and the script resolver a little bit
with respect to workspace handling.

The script resolver now relies completly on the resource resolver for
workspace related resolving.

The resource resolver can be used with the {workspace}:[path} syntax ti
resolve resources in a particalur workpace. If the current user is now
allowed to login into that workspace, this is treated as a resource not
found.

The commons auth and the login mechanism there does not support a
workspace property anymore. This is now more or less handled
transparently behind the sense.

I hope I did not break something - but there might be new bugs with the
changes (although this should only affect workspace related resolving) :)

There is currently one thing which I think we have to change: if you
resolve a resource with a workspace prefix, the resource resolver
returned by resource.getResourceResolver() resolves resources in that
workspace first. (Internally a resource resolver for each workspace is
created - on demand. And this internal resource resolver is returned for
such a resource).
I think we should change this and always return the "real" resource
resolver.

WDYT?

Carsten
-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Workspace related functionality

[Justin Edelson <ju...@gmail.com>]

On May 25, 2010, at 2:38 AM, Carsten Ziegeler <cz...@apache.org>  
wrote:

> Justin Edelson  wrote
>
>>  Is there a
>> reason why request.getResourceResolver().adaptTo(Session.class)
>> *shouldn't* return the requested session?
> Good question :) Actually I'm not sure whether this is a good idea or
> not. I'm rarely using directly the JCR API - except for modifications
> the Sling API is sufficient.
>
Yeah, but modifications are important :) The session is also needed  
for some access control operations.

> Now, we could add some kind of a flag inside the resource resolver
> implementation which returns the "correct" session on adaptTo. It  
> would
> be great if we could make this as simple as possible :)
> So the question is when to set this flag - one easy solution would  
> be to
> set it if resolve(Request, String) is called. This should work in the
> known cases and avoids additional interfaces and/or thread locals.
I like this. I'm traveling today, so I won't be able to prototype/test  
this today. But it is promising. Especially since I realized my  
ThreadLocal idea wouldn't work anyway.

Justin

> Regards
> Carsten
> -- 
> Carsten Ziegeler
> cziegeler@apache.org

Re: Workspace related functionality

[Carsten Ziegeler <cz...@apache.org>]

Justin Edelson  wrote

> I think we need to decide which method call (or series of calls) returns
> a session logged into the requested workspace. The call used in the post
> servlet and elsewhere is
> 
> request.getResourceResolver().adaptTo(Session.class)
> 
> If we can say that this method should always return a session to the
> requested workspace, then we just need to make that happen. 
Hmm, yes, that would be a solution as well.

> So maybe...
> 
> * create a ThreadLocal in SlingHttpServletRequest to get the current
> request.
> 
> * add code to JcrResourceResolver's adaptTo() method which figures out
> if there's a current request and returns the right session.
> 
> Alternatively, we could create a new interface "RequestedSession" which
> extends Session and then look for that class in adaptTo(), i.e.
> 
> request.getResourceResolver().adaptTo(Session.class) -> session in
> default workspace
> request.getResourceResolver().adaptTo(RequestedSession.class) -> session
> in non-default workspace (if specified in request attribute)
> 
> But this creates an extra dependency upon the Sling API. Is there a
> reason why request.getResourceResolver().adaptTo(Session.class)
> *shouldn't* return the requested session?
Good question :) Actually I'm not sure whether this is a good idea or
not. I'm rarely using directly the JCR API - except for modifications
the Sling API is sufficient.

Now, we could add some kind of a flag inside the resource resolver
implementation which returns the "correct" session on adaptTo. It would
be great if we could make this as simple as possible :)
So the question is when to set this flag - one easy solution would be to
set it if resolve(Request, String) is called. This should work in the
known cases and avoids additional interfaces and/or thread locals.

Regards
Carsten
-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Workspace related functionality

[Justin Edelson <ju...@gmail.com>]

On 5/24/10 12:47 PM, Carsten Ziegeler wrote:
> Justin Edelson  wrote
>> I just found a serious problem with this implementation - it is no
>> longer possible to create nodes via the post servlet as ModifyOperation
>> for a non-default workspace.
>>
>> Specifically, I'm looking at the ModifyOperation, line 96:
>> Session session = request.getResourceResolver().adaptTo(Session.class);
>>
>> This could be switched to:
>> request.getResource().getResourceResolver().adaptTo(Session.class);
>>
>> But it can't be switched to:
>> request.getResource().adaptTo(Node.class).getSession();
>>
>> because the resource is a synthetic resource.
>>
>> I'm not sure what we should do. Thoughts?
>>
> Hi,
> 
> actually I'm not sure either :) It seems that this whole workspace thing
> is more complicated than we (I) thought and that we already made
> assumptions here and there how it might work.
> 
> I think the script resolver is now fine :) (it's not 100% tested but in
> general it should be fine...)
> 
> Event handling for resources and the resource resolver supporting the
> {wspName}:{path} notation looks pretty good for me as well.
> 
> Now, I see two possible solutions:
> 
> a) we tweak my latest changes and allow again to provide the workspace
> name in the authentication info (as I left most of the stuff in, this is
> fairly easy to do). But we need to think about what this actually means
> in terms of how the returned resource resolver behaves (adaptTo,
> resolving resources without a workspace prefix etc.)
> 
> b) Continue the path of having a "global" resource resolver which always
> resolves against the default workspace unless a prefix is given. In this
> case we run into problems as mentioned above and have to fix all
> occurrances here and there.
> 
> While I would really prefer b) I fear the better and more compatible
> solution is a).
I think we need to decide which method call (or series of calls) returns
a session logged into the requested workspace. The call used in the post
servlet and elsewhere is

request.getResourceResolver().adaptTo(Session.class)

If we can say that this method should always return a session to the
requested workspace, then we just need to make that happen. So maybe...

* create a ThreadLocal in SlingHttpServletRequest to get the current
request.

* add code to JcrResourceResolver's adaptTo() method which figures out
if there's a current request and returns the right session.

Alternatively, we could create a new interface "RequestedSession" which
extends Session and then look for that class in adaptTo(), i.e.

request.getResourceResolver().adaptTo(Session.class) -> session in
default workspace
request.getResourceResolver().adaptTo(RequestedSession.class) -> session
in non-default workspace (if specified in request attribute)

But this creates an extra dependency upon the Sling API. Is there a
reason why request.getResourceResolver().adaptTo(Session.class)
*shouldn't* return the requested session?

Justin


> 
> WDYT?
> Carsten

Re: Workspace related functionality

[Carsten Ziegeler <cz...@apache.org>]

Justin Edelson  wrote
> I just found a serious problem with this implementation - it is no
> longer possible to create nodes via the post servlet as ModifyOperation
> for a non-default workspace.
> 
> Specifically, I'm looking at the ModifyOperation, line 96:
> Session session = request.getResourceResolver().adaptTo(Session.class);
> 
> This could be switched to:
> request.getResource().getResourceResolver().adaptTo(Session.class);
> 
> But it can't be switched to:
> request.getResource().adaptTo(Node.class).getSession();
> 
> because the resource is a synthetic resource.
> 
> I'm not sure what we should do. Thoughts?
> 
Hi,

actually I'm not sure either :) It seems that this whole workspace thing
is more complicated than we (I) thought and that we already made
assumptions here and there how it might work.

I think the script resolver is now fine :) (it's not 100% tested but in
general it should be fine...)

Event handling for resources and the resource resolver supporting the
{wspName}:{path} notation looks pretty good for me as well.

Now, I see two possible solutions:

a) we tweak my latest changes and allow again to provide the workspace
name in the authentication info (as I left most of the stuff in, this is
fairly easy to do). But we need to think about what this actually means
in terms of how the returned resource resolver behaves (adaptTo,
resolving resources without a workspace prefix etc.)

b) Continue the path of having a "global" resource resolver which always
resolves against the default workspace unless a prefix is given. In this
case we run into problems as mentioned above and have to fix all
occurrances here and there.

While I would really prefer b) I fear the better and more compatible
solution is a).

WDYT?
Carsten
-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Workspace related functionality

[Justin Edelson <ju...@gmail.com>]

I just found a serious problem with this implementation - it is no
longer possible to create nodes via the post servlet as ModifyOperation
for a non-default workspace.

Specifically, I'm looking at the ModifyOperation, line 96:
Session session = request.getResourceResolver().adaptTo(Session.class);

This could be switched to:
request.getResource().getResourceResolver().adaptTo(Session.class);

But it can't be switched to:
request.getResource().adaptTo(Node.class).getSession();

because the resource is a synthetic resource.

I'm not sure what we should do. Thoughts?

Justin


On 5/23/10 9:42 AM, Carsten Ziegeler wrote:
> Hi,
> 
> as recently discussed, I changed the implementation of the (jcr)
> resource resolver (and factory) and the script resolver a little bit
> with respect to workspace handling.
> 
> The script resolver now relies completly on the resource resolver for
> workspace related resolving.
> 
> The resource resolver can be used with the {workspace}:[path} syntax ti
> resolve resources in a particalur workpace. If the current user is now
> allowed to login into that workspace, this is treated as a resource not
> found.
> 
> The commons auth and the login mechanism there does not support a
> workspace property anymore. This is now more or less handled
> transparently behind the sense.
> 
> I hope I did not break something - but there might be new bugs with the
> changes (although this should only affect workspace related resolving) :)
> 
> There is currently one thing which I think we have to change: if you
> resolve a resource with a workspace prefix, the resource resolver
> returned by resource.getResourceResolver() resolves resources in that
> workspace first. (Internally a resource resolver for each workspace is
> created - on demand. And this internal resource resolver is returned for
> such a resource).
> I think we should change this and always return the "real" resource
> resolver.
> 
> WDYT?
> 
> Carsten

Re: Workspace related functionality

[Justin Edelson <ju...@gmail.com>]

On May 23, 2010, at 10:44 AM, Carsten Ziegeler <cz...@apache.org>  
wrote:
>
>> Finally, in terms of returning the real resource resolver, this is  
>> OK with
>> me, but should be done soon.
> Yes, I first wanted to get some feedback before doing so, but I'll do
> this rsn.
>
I meant soon as in the next week or so, i.e. Before the next release.

Justin

>> As I mentioned before, one of my needs is to
>> get access to a session in the request-specific workspace. First I  
>> did:
>> session = request.getResourceResolver().adaptTo(Session.class)
>>
>> then I switched to:
>> session = request.getResource().getResourceResolver().adaptTo 
>> (Session.class)
>>
>> now I will need to:
>> session = request.getResource().adaptTo(Node.class).getSession()
>>
> Yes, that's true - but in the end I think this is the better way, even
> if this requires more method calls :)
>
> Regards
> Carsten
> -- 
> Carsten Ziegeler
> cziegeler@apache.org

Re: Workspace related functionality

[Carsten Ziegeler <cz...@apache.org>]

Hi,

Justin Edelson  wrote
> Carsten-
> I've only had a few hours to use the new implementation, so I may have more
> comments, but my initial comments are:
> 
> * It should be noted somewhere that the default workspace is always used for
> authentication.
Yepp.

> 
> * We no longer have an *obvious* place to set the workspace for a request.
> When workspace selection was integrated with auth, it was clear (I thought)
> that you needed an AuthenticationHandler or an
> AuthenticationInfoPostProcessor. But now, workspace selection is detached
> from auth, yet auth is the only place within Sling you can effectively do
> workspace selection. I had originally thought you could use a Filter, but
> Sling Filter processing occurs after resource resolution, so you either have
> to use a Filter external to Sling or use the auth extension points
> anyway.  I would really like my "Selected workspace X because of Y" to be
> loggable in RequestProgressTracker, but this won't work if I use a "regular"
> Filter. Perhaps we need a new type of Filter that would get invoked before
> resource resolution.
Ok, I see your point, maybe adding a new type of filters is really a
good thing.

> 
> * At first glance, it appears to me that you've removed the "fallback to the
> default workspace" logic we had in the script resolver. But I don't actually
> use that (and I don't think we have integration tests for it), so I'm not
> sure. Removing this fallback is OK with me (Bertrand may feel differently),
> but it should be noted in SLING-1446.
I think I left it in, at least I wanted to :)

> Finally, in terms of returning the real resource resolver, this is OK with
> me, but should be done soon. 
Yes, I first wanted to get some feedback before doing so, but I'll do
this rsn.

> As I mentioned before, one of my needs is to
> get access to a session in the request-specific workspace. First I did:
> session = request.getResourceResolver().adaptTo(Session.class)
> 
> then I switched to:
> session = request.getResource().getResourceResolver().adaptTo(Session.class)
> 
> now I will need to:
> session = request.getResource().adaptTo(Node.class).getSession()
> 
Yes, that's true - but in the end I think this is the better way, even
if this requires more method calls :)

Regards
Carsten
-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Workspace related functionality

[Justin Edelson <ju...@justinedelson.com>]

Carsten-
I've only had a few hours to use the new implementation, so I may have more
comments, but my initial comments are:

* It should be noted somewhere that the default workspace is always used for
authentication.

* We no longer have an *obvious* place to set the workspace for a request.
When workspace selection was integrated with auth, it was clear (I thought)
that you needed an AuthenticationHandler or an
AuthenticationInfoPostProcessor. But now, workspace selection is detached
from auth, yet auth is the only place within Sling you can effectively do
workspace selection. I had originally thought you could use a Filter, but
Sling Filter processing occurs after resource resolution, so you either have
to use a Filter external to Sling or use the auth extension points
anyway.  I would really like my "Selected workspace X because of Y" to be
loggable in RequestProgressTracker, but this won't work if I use a "regular"
Filter. Perhaps we need a new type of Filter that would get invoked before
resource resolution.

* At first glance, it appears to me that you've removed the "fallback to the
default workspace" logic we had in the script resolver. But I don't actually
use that (and I don't think we have integration tests for it), so I'm not
sure. Removing this fallback is OK with me (Bertrand may feel differently),
but it should be noted in SLING-1446.

Finally, in terms of returning the real resource resolver, this is OK with
me, but should be done soon. As I mentioned before, one of my needs is to
get access to a session in the request-specific workspace. First I did:
session = request.getResourceResolver().adaptTo(Session.class)

then I switched to:
session = request.getResource().getResourceResolver().adaptTo(Session.class)

now I will need to:
session = request.getResource().adaptTo(Node.class).getSession()

Justin

On Sun, May 23, 2010 at 9:42 AM, Carsten Ziegeler <cz...@apache.org>wrote:

> Hi,
>
> as recently discussed, I changed the implementation of the (jcr)
> resource resolver (and factory) and the script resolver a little bit
> with respect to workspace handling.
>
> The script resolver now relies completly on the resource resolver for
> workspace related resolving.
>
> The resource resolver can be used with the {workspace}:[path} syntax ti
> resolve resources in a particalur workpace. If the current user is now
> allowed to login into that workspace, this is treated as a resource not
> found.
>
> The commons auth and the login mechanism there does not support a
> workspace property anymore. This is now more or less handled
> transparently behind the sense.
>
> I hope I did not break something - but there might be new bugs with the
> changes (although this should only affect workspace related resolving) :)
>
> There is currently one thing which I think we have to change: if you
> resolve a resource with a workspace prefix, the resource resolver
> returned by resource.getResourceResolver() resolves resources in that
> workspace first. (Internally a resource resolver for each workspace is
> created - on demand. And this internal resource resolver is returned for
> such a resource).
> I think we should change this and always return the "real" resource
> resolver.
>
> WDYT?
>
> Carsten
> --
> Carsten Ziegeler
> cziegeler@apache.org
>