Hi Ian On Tue, Sep 19, 2017 at 10:42 AM, Ian Boston <i...@tfd.co.uk> wrote: > Hi Julian, > > (this time to the list as well, hit reply rather than reply all) > > On 19 September 2017 at 08:51, Julian Sedding <jsedd...@gmail.com> wrote: >> >> 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. > > > > > > That is not what I meant. I meant 1 API and 1 Service implementation. > Hopefully this explanation will make more sense....
Yes, I misunderstood you here. Sorry for the confusion. > > Currently other bundles that want to do the conversion must do > ((ExternalizableInputStream)resource.adaptTo(InputStream.class)).getURI() or > ((ExternalizableInputStream)resource.adaptTo(InputStream.class)).getPrivateURI(), > and if they dont have Resource they are out of luck. > > > If URIConverter was a API then the URIConverter implementation in patch 1 > (renamed to URIConverterImpl) was exposed, the other bundles would be able > bind to the URIConveter service, and do uriConveter.getURI(jcrValue) or > uriConveter.getPrivateURI(jcrValue). > In patch 1 they would also be able todo > resourceResolver.adaptTo(URIConveter.class).getURI(jcrValue) or > resourceResolver.adaptTo(URIConveter.class).getPrivateURI(jcrValue) > > In patch 2, the URIConverter doesn't exist, so that can't be done. The > conversion is buried inside the JcrResource class. > > The use case here is for other endpoints that might be used by, for example, > workflow operations on worker VMs to get secure direct access to the > binary. > > I did not mean many implementations of URIConverter. As you point out, that > makes no sense. > > 1 API and 1 Service implementation of URIConverter only makes sense if we > think that other code needs to do JCRValue->URI and cant do > Resource->InputStream->URI. > > There is an argument that we should not create any new APIs with JCR API > parameters, in which case JCRValue->URI should not be entertained as an > option. With the upcoming changes in Jackrabbit we will already have the URIProvider service(s), which does JCR Value -> URI conversion. Interested parties can use it independent of Resource/ResourceResolver. Or am I missing something? Regards Julian > > Best Regards > Ian > > >> >> 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 >> >> > >> >> > >> > >> > > >
