I only propose to change getParent() on NonExistingResource. Right now we have the situation that getParent() would just return the wrong NonExistingResource! So what we could do is to rely on resolve() as a fallback in case the regular way does not succeed. That would not make things any worse than they currently are. For real resources the getParent() handling would not be touched at all.
But I know that even the solution of relying on RR.resolve(String) is not optimal, as the resource resolver mapping is only 100% functional in case you also consider the request (which you don't have at hand at that point in time). Maybe someone else has another good idea on how to solve this... Thanks, Konrad > On 10 Jun 2016, at 16:24, Rob Ryan <rr...@adobe.com> wrote: > > 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 >>>>>> >>>>> >>>>> >>> >> >
