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. Regardless here is a patch using HelperData. https://github.com/apache/sling/compare/trunk...ieb:OAK-6575-3-2?expand=1 15 files, And a patch using an Adapter. https://github.com/apache/sling/compare/trunk...ieb:OAK-6575-3-1?expand=1 8 files. 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. Best Regards Ian > > Regards > Julian > > > > > Best Regards > > Ian > > > >> > >> > >> Carsten > >> > >> > >> > >> -- > >> Carsten Ziegeler > >> Adobe Research Switzerland > >> cziege...@apache.org > > > > >
