Regardless of what other problem may exist; involving resolve() in getParent() isn't a solution.
Resolve() operates on _URI_ paths. Not on resource paths. So proposing to involve resolve() in an implementation of getParent() doesn't make sense. Resolve() makes special considerations of some constructs: _foo_bar to mean foo:bar. But a resource path could legitimately have _foo_bar as a component. /* : star resource handling getParent() would likely be broken if it involved resolve() in such cases. -Rob On 6/10/16, 12:12 AM, "Konrad Windszus" <konra...@gmx.de> wrote: >There is one related problem with mapping: > >Consider the case you have a resource resolver mapping from "/content/" >to "/". In the underlying repository you have a resource in >"/content/existingParent". > >Now it might happen that a NonExistingResource is resolved for path >"/existingParent/nonExistingResource". Since this is pointing towards a >non-existing resource the mapping is not active (i.e. the path will not >contain "/content"). So you end up with a NonExistingResource with path >"/existingParent/nonExistingResource". >Now you call getParent() on this resource. > >You would expect to end up with an existing resource in >"/content/existingParent" (because that path does exist in the >repository). Unfortunately due to the logic in >AbstractResource.getParent() this just calls ResourceResolver.getParent() >which will not use ResourceResolver.resolve(...) but rather >ResourceResolver.getResource(...) internally. So the mapping is not >considered here. Therefore no resource is found at "/existingParent", >although it does exist at "/content/existingParent". > >This is unexpected and wrong in my opinion. So the NonExistingResource >should not rely on AbstractResource.getParent() at all, but rather use >its own implementation relying on ResourceResolver.resolve(...). WDYT? > >Thanks, >Konrad > > >> On 02 Jun 2016, at 14:48, Konrad Windszus <konra...@gmx.de> wrote: >> >> Thanks for the input. >> I created https://issues.apache.org/jira/browse/SLING-5757 for tracking >>that and I am going to propose a patch in there. >> >> What about SyntheticResources which are not NonExistingResources? If we >>would follow the same approach as for NonExistingResources the question >>is, with which resource type the non-existing parent resource should be >>created? Or should a SyntheticResource (which is not a >>NonExistingResource) return a NonExistingResource for getParent() >>instead in that case? >> >> Konrad >> >> >>> On 02 Jun 2016, at 13:47, Daniel Klco <dk...@apache.org> wrote: >>> >>> I would imagine that the only thing this would change is to make a >>>small >>> number of null checks irrelevant. >>> >>> +1 for making the behavior more consistent, however, the JavaDocs and >>> release notes should be explicit about this change. >>> >>> On Thu, Jun 2, 2016 at 4:45 AM, Georg Henzler <slin...@ghenzler.de> >>>wrote: >>> >>>> Hi Konrad, >>>> >>>> +1 for making the behaviour of NonExistingResource more consistent - I >>>> personally can't think of any places this would break existing code. >>>> >>>> Regards >>>> Georg >>>> >>>> >>>> >>>> On 2016-06-01 15:09, Konrad Windszus wrote: >>>> >>>>> Hi Robert, >>>>> thanks for your input. >>>>> >>>>> >>>>>> I am not sure whether this would confuse existing clients though... >>>>>> >>>>> >>>>> I am also a bit worried about that but the only example I could think >>>>> of is a code trying to create the parent nodes or collecting the >>>>> non-existing ones by checking getParent() for null. >>>>> >>>>> This would be pretty bad style IMHO therefore I would deliberately be >>>>> willing to break that code. I wonder what do others think about >>>>> changing the semantics of getParent() for NonExistingResource and >>>>> probably also SyntheticResource. >>>>> Konrad >>>>> >>>> >>>> >> >
