Hi Ian Thanks for trying to use HelperData.
On Mon, Sep 18, 2017 at 5:30 PM, Ian Boston <i...@tfd.co.uk> wrote: > Hi, > > On 18 September 2017 at 14:49, Julian Sedding <jsedd...@gmail.com> wrote: >> >> On Mon, Sep 18, 2017 at 2:58 PM, Ian Boston <i...@tfd.co.uk> wrote: >> > Hi, >> > >> > On 18 September 2017 at 13:51, Carsten Ziegeler <cziege...@apache.org> >> > 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 >> >> cziege...@apache.org >> > >> > > >
