Re: Merging OAK-5784 into 1.6.1

[Angela Schreiber]

updated documentation with the 2 points mentioned below at revision
1785128.

On 24/02/17 11:40, "Angela Schreiber"  wrote:

>Hi Chetan
>
>Thanks a lot for your input!
>In fact I looked at the PropertyStateValue implementation and spotted the
>usage of the internal string representation. However, for the Restriction
>case I could not come up with a use case that would involve a binary
>value. The supported restrictions we are shipping are all of type STRING
>or NAME. As far as custom implementations are concerned I doubt that
>BINARY would make sense... but who knows... maybe it would be worth
>explicitly stating in the documentation at
>http://jackrabbit.apache.org/oak/docs/security/authorization/restriction.h
>t
>ml#pluggability that
>
>a) performance must be taken into consideration when writing custom impls
>b) the base impl of the Restriction interface relies on
>PropertyStateValue.hashCode and custom implementations may need to adjust
>that _if_ they contain binaries... ok... i don't think that would be
>sensible... but who knows :-)
>
>as far as PropertyState.hashCode is concerned: i vaguely remember that we
>discussed that in the early days of Oak and decided that there is no need
>for including the value because a given Tree object will never have
>multiple properties with the same name and thus decided to just use the
>name and type for performance reasons.
>
>as far as PropertyStateValue is concerned: while working on the
>improvement i struggled again with the fact that the implementation of
>this oak-api interface is located in spi.query package that looks
>pretty troublesome to me from a design point of view. maybe this is
>another indication that we should think about having an implementation
>with plugins.memory and deal with the binary topic there.
>
>wdyt?
>kind regards
>angela
>
>On 24/02/17 10:39, "Chetan Mehrotra"  wrote:
>
>>Changes look fine however one aspect might cause issue
>>
>>RestrictionImpl#hashCode -> PropertyValues#hashCode ->
>>PropertyStateValue#hashCode
>>
>>
>>private String getInternalString() {
>>StringBuilder sb = new StringBuilder();
>>Iterator iterator = getValue(Type.STRINGS).iterator();
>>while (iterator.hasNext()) {
>>sb.append(iterator.next());
>>if (iterator.hasNext()) {
>>sb.append(",");
>>}
>>}
>>return sb.toString();
>>}
>>
>>@Override
>>public int hashCode() {
>>return getType().tag() ^ getInternalString().hashCode();
>>}
>>
>>
>>Here it tries to get value as STRINGS which leads to
>>PropertyState#getValue(Type.STRINGS) which would lead to a Binary
>>getting coerced to String in Conversions#convert(Blob) which would
>>lead to load of whole binary. Now I am not sure if PropertyState in
>>RestrictionImpl is applicable for Binary property also
>>
>>Probably PropertyStateValue#hashCode should take care of Binary
>>properties and thats why PropertyState#hashCode does not take into
>>account the value
>>Chetan Mehrotra
>>
>>
>>On Fri, Feb 24, 2017 at 2:34 PM, Angela Schreiber 
>>wrote:
>>> hi oak-devs
>>>
>>> i would like to merge another improvement into the 1.6.1 branch:
>>> https://issues.apache.org/jira/browse/OAK-5784
>>>
>>> in addition to additional tests i run the AceCreationTest benchmark and
>>> attached the results to the issue.
>>> however, having some extra pair of eyes would be appreciated in order
>>>to
>>> limit the risk of regressions.
>>>
>>> thanks
>>> angela
>>>
>>>
>

Re: Merging OAK-5784 into 1.6.1

[Angela Schreiber]

Hi Chetan

Created OAK-5838  to keep
track of the topics discussed here.

Kind regards
Angela

On 24/02/17 11:56, "Chetan Mehrotra"  wrote:

>On Fri, Feb 24, 2017 at 4:10 PM, Angela Schreiber 
>wrote:
>> maybe this is
>> another indication that we should think about having an implementation
>> with plugins.memory and deal with the binary topic there.
>
>+1
>
>Then we can go with current fix (and also merge to 1.6) and later
>backport the change to 1.6 branch
>
>
>Chetan Mehrotra

Re: Merging OAK-5784 into 1.6.1

[Chetan Mehrotra]

On Fri, Feb 24, 2017 at 4:10 PM, Angela Schreiber  wrote:
> maybe this is
> another indication that we should think about having an implementation
> with plugins.memory and deal with the binary topic there.

+1

Then we can go with current fix (and also merge to 1.6) and later
backport the change to 1.6 branch


Chetan Mehrotra

Re: Merging OAK-5784 into 1.6.1

[Angela Schreiber]

Hi Chetan

Thanks a lot for your input!
In fact I looked at the PropertyStateValue implementation and spotted the
usage of the internal string representation. However, for the Restriction
case I could not come up with a use case that would involve a binary
value. The supported restrictions we are shipping are all of type STRING
or NAME. As far as custom implementations are concerned I doubt that
BINARY would make sense... but who knows... maybe it would be worth
explicitly stating in the documentation at
http://jackrabbit.apache.org/oak/docs/security/authorization/restriction.ht
ml#pluggability that

a) performance must be taken into consideration when writing custom impls
b) the base impl of the Restriction interface relies on
PropertyStateValue.hashCode and custom implementations may need to adjust
that _if_ they contain binaries... ok... i don't think that would be
sensible... but who knows :-)

as far as PropertyState.hashCode is concerned: i vaguely remember that we
discussed that in the early days of Oak and decided that there is no need
for including the value because a given Tree object will never have
multiple properties with the same name and thus decided to just use the
name and type for performance reasons.

as far as PropertyStateValue is concerned: while working on the
improvement i struggled again with the fact that the implementation of
this oak-api interface is located in spi.query package that looks
pretty troublesome to me from a design point of view. maybe this is
another indication that we should think about having an implementation
with plugins.memory and deal with the binary topic there.

wdyt?
kind regards
angela

On 24/02/17 10:39, "Chetan Mehrotra"  wrote:

>Changes look fine however one aspect might cause issue
>
>RestrictionImpl#hashCode -> PropertyValues#hashCode ->
>PropertyStateValue#hashCode
>
>
>private String getInternalString() {
>StringBuilder sb = new StringBuilder();
>Iterator iterator = getValue(Type.STRINGS).iterator();
>while (iterator.hasNext()) {
>sb.append(iterator.next());
>if (iterator.hasNext()) {
>sb.append(",");
>}
>}
>return sb.toString();
>}
>
>@Override
>public int hashCode() {
>return getType().tag() ^ getInternalString().hashCode();
>}
>
>
>Here it tries to get value as STRINGS which leads to
>PropertyState#getValue(Type.STRINGS) which would lead to a Binary
>getting coerced to String in Conversions#convert(Blob) which would
>lead to load of whole binary. Now I am not sure if PropertyState in
>RestrictionImpl is applicable for Binary property also
>
>Probably PropertyStateValue#hashCode should take care of Binary
>properties and thats why PropertyState#hashCode does not take into
>account the value
>Chetan Mehrotra
>
>
>On Fri, Feb 24, 2017 at 2:34 PM, Angela Schreiber 
>wrote:
>> hi oak-devs
>>
>> i would like to merge another improvement into the 1.6.1 branch:
>> https://issues.apache.org/jira/browse/OAK-5784
>>
>> in addition to additional tests i run the AceCreationTest benchmark and
>> attached the results to the issue.
>> however, having some extra pair of eyes would be appreciated in order to
>> limit the risk of regressions.
>>
>> thanks
>> angela
>>
>>

Re: Merging OAK-5784 into 1.6.1

[Chetan Mehrotra]

Changes look fine however one aspect might cause issue

RestrictionImpl#hashCode -> PropertyValues#hashCode ->
PropertyStateValue#hashCode


private String getInternalString() {
StringBuilder sb = new StringBuilder();
Iterator iterator = getValue(Type.STRINGS).iterator();
while (iterator.hasNext()) {
sb.append(iterator.next());
if (iterator.hasNext()) {
sb.append(",");
}
}
return sb.toString();
}

@Override
public int hashCode() {
return getType().tag() ^ getInternalString().hashCode();
}


Here it tries to get value as STRINGS which leads to
PropertyState#getValue(Type.STRINGS) which would lead to a Binary
getting coerced to String in Conversions#convert(Blob) which would
lead to load of whole binary. Now I am not sure if PropertyState in
RestrictionImpl is applicable for Binary property also

Probably PropertyStateValue#hashCode should take care of Binary
properties and thats why PropertyState#hashCode does not take into
account the value
Chetan Mehrotra


On Fri, Feb 24, 2017 at 2:34 PM, Angela Schreiber  wrote:
> hi oak-devs
>
> i would like to merge another improvement into the 1.6.1 branch:
> https://issues.apache.org/jira/browse/OAK-5784
>
> in addition to additional tests i run the AceCreationTest benchmark and
> attached the results to the issue.
> however, having some extra pair of eyes would be appreciated in order to
> limit the risk of regressions.
>
> thanks
> angela
>
>

Merging OAK-5784 into 1.6.1

[Angela Schreiber]

hi oak-devs

i would like to merge another improvement into the 1.6.1 branch:
https://issues.apache.org/jira/browse/OAK-5784

in addition to additional tests i run the AceCreationTest benchmark and
attached the results to the issue.
however, having some extra pair of eyes would be appreciated in order to
limit the risk of regressions.

thanks
angela