Hi Carsten,
I don't understand the coupling between the addition of
Resource.getValueMap() and having the Mongo, Merged, and JMX
ResourceProviders start using DeepReadValueMapDecorator.
IIOW, couldn't we get the same effect *without* changing the Resource
API? That has an impact on downstream users and I'd suggest putting it
in the category of "things to address the next time we do a
refactoring of the Resource API".
Regards,
Justin
On Sat, Mar 15, 2014 at 8:57 AM, Carsten Ziegeler <cziege...@apache.org> wrote:
> Exactly, if you have a look at the implementation or read my description,
> you see that the jcr implementation did not change at all. So, for all jcr
> backed resources nothing has changed and the deep read is done there.
> Implementations are free to implement the deep read themselves or can use
> the provided support.
>
> Although, as mentioned in the beginning, the jcr implementation is wrong as
> it does not use the resource tree, so a getValueMap().get("content/title")
> might return something different than a
> getChild("content").getValueMap().get("title"). But this has always been
> the case, and as the jcr implementation is very complicated with all the
> encoding stuff it contains, I think we shouldn't touch this.
>
> Carsten
>
>
> 2014-03-14 20:28 GMT+01:00 Alexander Klimetschek <aklim...@adobe.com>:
>
>> IIUC, a deep read is now done generically by fetching sub resources first
>> and each value map will from now on only read their "local" values.
>>
>> I think this introduces overhead for a lot of common operations. For every
>> descendant node you now have to read and instantiate a resource and value
>> map. So far with the JCR property map this was left to the JCR repo.
>>
>> IMO we should not change much here and simply support deep reads whereever
>> possible in the resource implemention (and the missing part here is in the
>> /mnt/overlay resource merger AFAIU).
>>
>> Cheers,
>> Alex
>>
>> On 14.03.2014, at 08:45, Carsten Ziegeler <cziege...@apache.org> wrote:
>>
>> > I just found out that we have to change our idea a little bit, we can't
>> > demand that Resource#adaptTo(ValueMap.class) always returns a value map.
>> > The nice exception is the JcrPropertyResource...
>> >
>> > Regards
>> > Carsten
>> >
>> >
>> > 2014-03-14 15:08 GMT+01:00 Carsten Ziegeler <cziege...@apache.org>:
>> >
>> >> Hi Felix,
>> >>
>> >> yes, the changes I just committed in fact deprecate
>> >> ResourceUtil.getValueMap() and let that method call
>> resource.getValueMap()
>> >> (if resource is not null of course).
>> >>
>> >> The utility code is an implementation of the value map which supports
>> the
>> >> deep read methods. See the new DeepReadValueMapDecorator class.
>> >>
>> >> Existing implementations which currently return a value map simply need
>> to
>> >> wrap their value map with this new decorator. Of course they are free to
>> >> implement the deep reads by themselves.
>> >>
>> >> I wanted to change the jcr resource provider, but that would introduce
>> >> incompatibilities to a lot of wired path encoding stuff in the
>> >> implementation. So it's safer to leave it as is.
>> >>
>> >> Carsten
>> >>
>> >>
>> >> 2014-03-14 14:58 GMT+01:00 Felix Meschberger <fmesc...@adobe.com>:
>> >>
>> >> Hi
>> >>>
>> >>> Am 14.03.2014 um 11:42 schrieb Carsten Ziegeler <cziege...@apache.org
>> >:
>> >>>
>> >>>> The only other option I see is to make ValueMap a first class citizen
>> >>> and:
>> >>>
>> >>> I think this makes sense independently of deep reads or not -- and
>> maybe
>> >>> we should have done this right when we introduced the ValueMap...
>> >>>
>> >>>> a) add a getValueMap() method to Resource - default implementation in
>> >>>> AbstractResource does the same as ResourceUtil does today
>> >>>
>> >>> In essence ?
>> >>>
>> >>>> ValueMap AbstractResource.getValueMap() {
>> >>>> return ResourceUtil.getValueMap(this);
>> >>>> }
>> >>>
>> >>> or rather copy the ResourceUtil.getValueMap(Resource) method to
>> >>> AbstractResource.getValueMap(), implement the deep-read logic there
>> and do
>> >>>
>> >>>> ValueMap ResourceUtil.getValueMap(Resource r) {
>> >>>> return r.getValueMap();
>> >>>> }
>> >>>
>> >>> while at the same time deprecating ResourceUtil.getValueMap(Resource) ?
>> >>>
>> >>>
>> >>>> b) clearly state that deep gets are allowed for reading from those
>> maps
>> >>>
>> >>> That would go to the JavaDoc of Resource.getValueMap() along with the
>> >>> requirement to never return null.
>> >>>
>> >>>> c) provide utility code in AbstractResource (or maybe somewhere else)
>> so
>> >>>> implementations do not need to copy the same code over and over again
>> >>>
>> >>> What kind of utility code ?
>> >>>
>> >>> What would existing implementations do ?
>> >>>
>> >>> I would assume the JCR-based one would have to be touched to not
>> support
>> >>> deep reads itself, others will benefit. Right ?
>> >>>
>> >>> Regards
>> >>> Felix
>> >>>
>> >>>>
>> >>>> This would mean:
>> >>>> a) current code does not need to change, regardless whether
>> >>> ResourceUtil or
>> >>>> adaptTo is used
>> >>>> b) a value map is always provided
>> >>>> c) all value maps support deep reads
>> >>>> d) no code duplication necessary
>> >>>>
>> >>>> Carsten
>> >>>>
>> >>>>
>> >>>> 2014-03-14 11:37 GMT+01:00 Carsten Ziegeler <cziege...@apache.org>:
>> >>>>
>> >>>>> The idea behind ResourceUtil.getValueMap is that it never returns
>> null
>> >>>>> while resource.adaptTo(ValueMap) can.
>> >>>>> That's the whole point of this utility method.
>> >>>>>
>> >>>>> I assume people who do resource.adaptTo(ValueMap) never check for
>> null
>> >>>>> which is bound to fail
>> >>>>>
>> >>>>> Carsten
>> >>>>>
>> >>>>>
>> >>>>> 2014-03-14 11:30 GMT+01:00 Amit.. Gupta. <amitg...@adobe.com>:
>> >>>>>
>> >>>>>> FWIW, there are lots of calls to resource.adaptTo(ValueMap) in
>> >>> rendering
>> >>>>>> code.
>> >>>>>> +1
>> >>>>>>
>> >>>>>>
>> >>>>>
>> >>>>>
>> >>>>> --
>> >>>>> Carsten Ziegeler
>> >>>>> cziege...@apache.org
>> >>>>>
>> >>>>
>> >>>>
>> >>>>
>> >>>> --
>> >>>> Carsten Ziegeler
>> >>>> cziege...@apache.org
>> >>>
>> >>>
>> >>
>> >>
>> >> --
>> >> Carsten Ziegeler
>> >> cziege...@apache.org
>> >>
>> >
>> >
>> >
>> > --
>> > Carsten Ziegeler
>> > cziege...@apache.org
>>
>>
>
>
> --
> Carsten Ziegeler
> cziege...@apache.org
