updated documentation with the 2 points mentioned below at revision 1785128.
On 24/02/17 11:40, "Angela Schreiber" <[email protected]> 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" <[email protected]> wrote: > >>Changes look fine however one aspect might cause issue >> >>RestrictionImpl#hashCode -> PropertyValues#hashCode -> >>PropertyStateValue#hashCode >> >>==== >> private String getInternalString() { >> StringBuilder sb = new StringBuilder(); >> Iterator<String> 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 <[email protected]> >>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 >>> >>> >
