I'm a little bit lost with the options. I think the only way we should provide atm is adapting a Resource to ExternalizableInputStream (or InputStream and then casting)
This should be flexible enough. Carsten Ian Boston 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.... > > 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 >>>>> >>>>> >>> >>> >> > -- Carsten Ziegeler Adobe Research Switzerland cziege...@apache.org
