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
