OK. Thanks for explaining. Just wanted to make sure I wasn't missing something.
Justin
On Sun, Mar 16, 2014 at 5:46 AM, Carsten Ziegeler <cziege...@apache.org> wrote:
> Hi Justin
>
> it's right we can get the same result without adding this method, however
> with adding this method we really make clear that there is a contract
> change.
> We don't need a refactoring of the Resource api, just an extension - which
> imho we should have done from the beginning.
>
> Carsten
>
>
> 2014-03-15 14:37 GMT+01:00 Justin Edelson <jus...@justinedelson.com>:
>
>> 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
>>
>
>
>
> --
> Carsten Ziegeler
> cziege...@apache.org
