[jira] [Comment Edited] (OAK-1147) SecureNodeBuilder/SecureNodeState: Consider using 'TreePermission#canReadProperties'

["angela (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/OAK-1147?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13814026#comment-13814026 ] 

angela edited comment on OAK-1147 at 11/5/13 4:42 PM:
------------------------------------------------------

one more thing... which IMO makes it a bit inconsistent: #getPropertyCount checks for 'canReadAll(Properties)' and alternatively calculates the number of properties:

{code} 
            if (treePermission.canReadAll()) {
                propertyCount = state.getPropertyCount();
            } else {
                propertyCount = count(filter(
                        state.getProperties(),
                        new ReadablePropertyPredicate()));
            }
{code}

on the other hand #getProperties() first tries to optimize for all properties being visible and second tests if the node state itself is readable (in which case it reads the properties)... if not it returns an empty list.

{code}
        if (treePermission.canReadAll()) {
            return state.getProperties();
        } else if (treePermission.canRead()) { // TODO: check DENY_PROPERTIES?
            return filter(
                    state.getProperties(),
                    new ReadablePropertyPredicate());
        } else {
            return emptyList();
        }
{code}
thus, one method make the accessibility of the properties depending on the read-status of the tree itself. the other doesn't. from a JCR level point of view it doesn't make a difference (as Node#getProperties can only be called if you already have the Node at hand)... but on the OAK level it might result in odd behavior as the tree can exist or not.


was (Author: anchela):
one more thing... which IMO makes it a bit inconsistent: #getPropertyCount checks for 'canReadAll(Properties)' and alternatively calculates the number of properties:

{code} 
            if (treePermission.canReadProperties()) {
                propertyCount = state.getPropertyCount();
            } else {
                propertyCount = count(filter(
                        state.getProperties(),
                        new ReadablePropertyPredicate()));
            }
{code}

on the other hand #getProperties() first tries to optimize for all properties being visible and second tests if the node state itself is readable (in which case it reads the properties)... if not it returns an empty list.
thus, one method make the accessibility of the properties depending on the read-status of the tree itself. the other doesn't. from a JCR level point of view it doesn't make a difference (as Node#getProperties can only be called if you already have the Node at hand)... but on the OAK level it might result in odd behavior as the tree can exist or not.

> SecureNodeBuilder/SecureNodeState: Consider using 'TreePermission#canReadProperties'
> ------------------------------------------------------------------------------------
>
>                 Key: OAK-1147
>                 URL: https://issues.apache.org/jira/browse/OAK-1147
>             Project: Jackrabbit Oak
>          Issue Type: Improvement
>            Reporter: angela
>            Assignee: angela
>
> the methods #getProperties and #getPropertyCount have a shortcut for those cases where all properties are accessible.
> however, the current implemention requires SecurityContext#canReadAll to return true in order to enable the shortcut. without knowing the very details of the SecureNodeState/Builder i would have expected that #canReadAllProperties would be sufficient.
> [~jukkaz], do you remember what was the reason for using #canReadAll here? i changed it for test purpose and didn't see any difference... but that may also imply that we don't have enough or specific tests for this.



--
This message was sent by Atlassian JIRA
(v6.1#6144)