jackrabbit-oak-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Angela Schreiber <anch...@adobe.com>
Subject Re: svn commit: r1465664 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Date Wed, 10 Apr 2013 09:30:29 GMT
hi jukka


On 4/10/13 8:26 AM, Jukka Zitting wrote:
> Hi,
>
> On Tue, Apr 9, 2013 at 5:49 PM, Angela Schreiber<anchela@adobe.com>  wrote:
>>> On Tue, Apr 9, 2013 at 12:45 PM, Angela Schreiber<anchela@adobe.com>
>>> How about we change the order of the checks, so that the readStatus is
>>> pre-loaded only when child.getChildNodeCount() == 0? That way we could
>>> avoid unnecessarily evaluating permissions at ancestor nodes, but
>>> still avoid the cost of the SecureNodeState wrapper for most leaf
>>> nodes.
>>
>> sure, we can try that... in general i would prefer to get a better
>> picture on the overall impact first before starting to optimize
>> things and make the code complicated.
>
> The way I see it, the ability to avoid the SecureNodeState wrapper in
> cases like these is an essential feature of this design. Otherwise we
> could just as well keep the access checks in TreeImpl and avoid the
> extra layer of indirection.

ok... i didn't see it that way.
my point is that permission evaluation should only occur if the
corresponding tree or property is really being read. that's what
i keep saying about having random access to repository which IMO
must be really fast without having to evaluate access to all the
ancestors.

> Thus I'd really want to include this
> aspect in the SecureNodeState code right from the beginning.
>
> Here's my proposed change to wrapChildNodeEntry.apply():
>
>      SecureNodeState secureChild =
>              new SecureNodeState(SecureNodeState.this, name, child);
>      if (child.getChildNodeCount() == 0
>              &&  secureChild.getReadStatus().includes(ALLOW_ALL)) {
>          return input;
>      } else {
>          return new MemoryChildNodeEntry(name, secureChild);
>      }

as i stated before this currently doesn't work with the way the
readstatus is being calculated... it would need to check
for 'read + read-access-control' in order to be really sure that
ALL items including access control content can be read.

however, i most cases users will not be allowed to read ac content
but just regular content, in which case the ac-content needs to
be filtered out again. the biggest issue: we don't know if there
is any ac-content down in the hierarchy.

don't get me wrong: i am not against having the optimization in
the SecureNodeState but the permission content must first be adjusted
in order to allow for such an optimization... notably the read-status.

>> anyway. the point here is that i would prefer to get it to work
>> first and optimize once we know that it really works.
>
> Based on my above point of view, I wouldn't consider SecureNodeState
> to be working as designed if at least some getChildNode() calls didn't
> return the raw underlying NodeState instance. :-)

i can accept that... but as long as it doesn't work there IMO is no
need in having that extra complexity... that's why i replaced it
with a TODO comment.

to me the whole thing is still a prove of concept. there are too
many very fundamental things broken now (in particular that we can no
longer access Trees and Properties if their parent is not accessible)
or odd (the nodestate keeping an tree internally) that i am fully 
convinced that moving it down to the node state level really will work.

angela

> BR,
>
> Jukka Zitting

Mime
View raw message