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.... 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. 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 > >> > > >> > > > > > >
