Sorry, forgot to include the link to the changes mentioned: https://github.com/apache/sling/compare/trunk...jsedding:refactor-jcr-helper-data
Regards Julian On Tue, Sep 19, 2017 at 9:51 AM, Julian Sedding <[email protected]> wrote: > Hi Ian > > Thanks for trying to use HelperData. > > On Mon, Sep 18, 2017 at 5:30 PM, Ian Boston <[email protected]> wrote: >> Hi, >> >> On 18 September 2017 at 14:49, Julian Sedding <[email protected]> wrote: >>> >>> On Mon, Sep 18, 2017 at 2:58 PM, Ian Boston <[email protected]> wrote: >>> > Hi, >>> > >>> > On 18 September 2017 at 13:51, Carsten Ziegeler <[email protected]> >>> > wrote: >>> >> >>> >> Julian Sedding wrote >>> >> > Hi Ian >>> >> > >>> >> > Would it make sense to remove the AdapterFactory implementation and >>> >> > instead pass along the URIProvider service(s) using the >>> >> > o.a.s.jcr.resource.internal.HelperData object instead? According to >>> >> > its javadoc, it is "used to pass several services/data to the >>> >> > resource". >>> >> > >>> >> > I assume the AdapterFactory is used because you don't have access to >>> >> > the OSGi service registry in JcrNodeResource where you need the >>> >> > URIProvider. >>> >> > >>> >> >>> >> Sounds like a good change to me, right a AdapterFactory shouldn't be >>> >> needed. >>> > >>> > >>> > The patch is quite extensive and it would need care to ensure that the >>> > list >>> > that is passed is fully thread safe. So far this approach has only been >>> > used >>> > with an AtomicReference holding a single value. >>> >>> I don't see the problem, Java has pretty decent support for concurrent >>> collections. Another option would be to pass a BundleContext to >>> HelperData and retrieve required services on-the-fly. IMHO that would >>> improve encapsulation and clean up both JcrProviderState and >>> JcrProviderStateFactory. >>> >>> > >>> > If a larger patch is wanted, I can track down everywhere the the >>> > JcrProviderState is created, as this holds the HelperData for the >>> > session, >>> > although the createProviderState method is already deprecated, which is >>> > the >>> > only way to create JcrProviderState afaict. See the >>> > JcrProviderStateFactory. >>> >>> I doubt that the patch would be larger (in terms of LOC). It would >>> likely touch more existing classes. >>> >>> HelperData's constructor is called precisely once. So is >>> JcrProviderState's constructor. The non-private method >>> createProviderState is called once (and yes, it's the only way to >>> create a JcrProviderState). The method is not deprecated (it >>> suppresses the deprecation warning of loginAdministrative). >>> >>> > >>> > Adapting to the URIConverter from the ResourceResolver seems a lot >>> > cleaner, >>> > assuming we believe the AdapterFactory pattern is a good pattern. >>> >>> IMHO publicly registering an AdapterFactory for private use is a >>> violation of the principle of least surprise and should thus be >>> avoided. Or in other words, I think it makes the code unnecessarily >>> hard to understand. >> >> >> >> I disagree. Hard to understand or not depends on how familiar you are with >> the code already. >> >> As evidence, I tried for 4h to do what the HelperData does, and didn't >> discover the HelperData class, even though it was staring me in the face. >> I asked Carsten for ideas last week, he also missed HelperData which is why >> I used an AdapterFactory. >> Then I again, I am not familiar with this area of the code base, so not >> surprised as the context flows through several layers. >> >> I guess your right, if you know the code intimately already, then the >> HelperData mechanism makes perfect sense, if not then the widely used >> AdapterFactory pattern makes sense. > > I am sure we all differ in what we consider intuitive, which is > natural, I guess. FWIW, I was also unfamiliar with the code before > reviewing your changes, but happened to see HelperData. Go figure. > >> >> Regardless here is a patch using HelperData. >> >> https://github.com/apache/sling/compare/trunk...ieb:OAK-6575-3-2?expand=1 >> 15 files, > > Of which 4 are tests. > >> >> And a patch using an Adapter. >> https://github.com/apache/sling/compare/trunk...ieb:OAK-6575-3-1?expand=1 >> 8 files. > > I created a patch refactoring HelperData to make it easier (IMHO) to > extend with OSGi services (3 files + 5 test files). This should reduce > the number of files needed to be touched for your changes. > >> >> I think the HelperData patch does make it a lot less likely anything else >> will implement the ExternalizableInputStream given that whoever tries to >> will need to also get the Oak URIProvider services binding directly to the >> Oak API, which we need to discourage. The AdapterFactory patch would remove >> that requirement, if the URIConverter implemented an API. > > Fair enough. However, as you say the URIConverter would need to become > API first. Every implementation would need to provide a suitable > AdapterFactory and in the end use it internally to create an > ExternalizableInputStream. Each implementation could still only use > its own AdapterFactory, because the adaptation relies on > implementation-specific details. To me it feels more natural that an > implementation of ResourceProvider would pass around a "context" > object that satisfies such demands. Or do you suggest we should do > away with HelperData and create an AdapterFactory to adapt to > DynamicClassLoaderManager? While we're at it, we could adapt > ResourceResolver to any OSGi service... > > I stand by my opinion, I consider it cleaner to keep the > implementation details within the bundle. I'm only offering my > opinion, however, and will live with whatever decision you go with. > > Regards > Julian > >> >> Best Regards >> Ian >> >> >> >>> >>> >>> Regards >>> Julian >>> >>> > >>> > Best Regards >>> > Ian >>> > >>> >> >>> >> >>> >> Carsten >>> >> >>> >> >>> >> >>> >> -- >>> >> Carsten Ziegeler >>> >> Adobe Research Switzerland >>> >> [email protected] >>> > >>> > >> >>
