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" <[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 >> >>
