Re: OAK-6575 - Support for external binary URLs.

[Julian Sedding]

Hi Ian

Sounds good. Feel free to apply my refactoring of the HelperData[0] if
that makes your patch simpler.

Regards
Julian

[0] 
https://github.com/apache/sling/compare/trunk...jsedding:refactor-jcr-helper-data


On Tue, Sep 19, 2017 at 12:41 PM, Ian Boston  wrote:
> Hi,
>
> On 19 September 2017 at 11:00, Julian Sedding  wrote:
>>
>> Hi Ian
>>
>> On Tue, Sep 19, 2017 at 10:42 AM, Ian Boston  wrote:
>
>
> 
>
>>
>> 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?
>>
>
> Yes, they can if they bind to the Oak API, which IMHO needs to be
> discouraged as it bypasses the Resource API and the JCR API, hence the
> suggestion to expose the URIConverter as an API.
>
> but, Carsten is correct.
>
> For the issue that triggered OAK-6575 to be fixed in Sling only requires a
> Resource->InputStream cast to ExternalizableInputStream pattern.
>
> In which case path 2 (HelperData is simplest).
>
> If so, at some later date other services can bind to the URIProvider API
> should they be required. No point in trying to solve everything in 1 patch.
>
> Best Regards
> Ian
>
>
>>
>> Regards
>> Julian
>>
>>
>

Re: OAK-6575 - Support for external binary URLs.

[Ian Boston]

Hi,

On 19 September 2017 at 11:00, Julian Sedding  wrote:

> Hi Ian
>
> On Tue, Sep 19, 2017 at 10:42 AM, Ian Boston  wrote:
>




> 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?
>
>
Yes, they can if they bind to the Oak API, which IMHO needs to be
discouraged as it bypasses the Resource API and the JCR API, hence the
suggestion to expose the URIConverter as an API.

but, Carsten is correct.

For the issue that triggered OAK-6575 to be fixed in Sling only requires a
Resource->InputStream cast to ExternalizableInputStream pattern.

In which case path 2 (HelperData is simplest).

If so, at some later date other services can bind to the URIProvider API
should they be required. No point in trying to solve everything in 1 patch.

Best Regards
Ian



> Regards
> Julian
>
>
>

Re: OAK-6575 - Support for external binary URLs.

[Julian Sedding]

Hi Ian

On Tue, Sep 19, 2017 at 10:42 AM, 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  wrote:
>>
>> Hi Ian
>>
>> Thanks for trying to use HelperData.
>>
>> On Mon, Sep 18, 2017 at 5:30 PM, Ian Boston  wrote:
>> > Hi,
>> >
>> > On 18 September 2017 at 14:49, Julian Sedding 
>> > wrote:
>> >>
>> >> On Mon, Sep 18, 2017 at 2:58 PM, Ian Boston  wrote:
>> >> > Hi,
>> >> >
>> >> > On 18 September 2017 at 13:51, Carsten Ziegeler
>> >> > 
>> >> > 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 U

Re: OAK-6575 - Support for external binary URLs.

[Carsten Ziegeler]

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  wrote:
> 
>> Hi Ian
>>
>> Thanks for trying to use HelperData.
>>
>> On Mon, Sep 18, 2017 at 5:30 PM, Ian Boston  wrote:
>>> Hi,
>>>
>>> On 18 September 2017 at 14:49, Julian Sedding 
>> wrote:

 On Mon, Sep 18, 2017 at 2:58 PM, Ian Boston  wrote:
> Hi,
>
> On 18 September 2017 at 13:51, Carsten Ziegeler >>
> 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. H

Re: OAK-6575 - Support for external binary URLs.

[Ian Boston]

Hi Julian,

(this time to the list as well, hit reply rather than reply all)

On 19 September 2017 at 08:51, Julian Sedding  wrote:

> Hi Ian
>
> Thanks for trying to use HelperData.
>
> On Mon, Sep 18, 2017 at 5:30 PM, Ian Boston  wrote:
> > Hi,
> >
> > On 18 September 2017 at 14:49, Julian Sedding 
> wrote:
> >>
> >> On Mon, Sep 18, 2017 at 2:58 PM, Ian Boston  wrote:
> >> > Hi,
> >> >
> >> > On 18 September 2017 at 13:51, Carsten Ziegeler  >
> >> > 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.

Re: OAK-6575 - Support for external binary URLs.

[Julian Sedding]

Sorry, forgot to include the link to the changes mentioned:
https://github.com/apache/sling/compare/trunk...jsedding:refactor-jcr-helper-data

Regards
Julian


On Tue, Sep 19, 2017 at 9:51 AM, Julian Sedding  wrote:
> Hi Ian
>
> Thanks for trying to use HelperData.
>
> On Mon, Sep 18, 2017 at 5:30 PM, Ian Boston  wrote:
>> Hi,
>>
>> On 18 September 2017 at 14:49, Julian Sedding  wrote:
>>>
>>> On Mon, Sep 18, 2017 at 2:58 PM, Ian Boston  wrote:
>>> > Hi,
>>> >
>>> > On 18 September 2017 at 13:51, Carsten Ziegeler 
>>> > 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

Re: OAK-6575 - Support for external binary URLs.

[Julian Sedding]

Hi Ian

Thanks for trying to use HelperData.

On Mon, Sep 18, 2017 at 5:30 PM, Ian Boston  wrote:
> Hi,
>
> On 18 September 2017 at 14:49, Julian Sedding  wrote:
>>
>> On Mon, Sep 18, 2017 at 2:58 PM, Ian Boston  wrote:
>> > Hi,
>> >
>> > On 18 September 2017 at 13:51, Carsten Ziegeler 
>> > 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: OAK-6575 - Support for external binary URLs.

[Ian Boston]

Hi,

On 18 September 2017 at 14:49, Julian Sedding  wrote:

> On Mon, Sep 18, 2017 at 2:58 PM, Ian Boston  wrote:
> > Hi,
> >
> > On 18 September 2017 at 13:51, Carsten Ziegeler 
> > 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
> >
> >
>

Re: OAK-6575 - Support for external binary URLs.

[Julian Sedding]

On Mon, Sep 18, 2017 at 2:58 PM, Ian Boston  wrote:
> Hi,
>
> On 18 September 2017 at 13:51, Carsten Ziegeler 
> 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.

Regards
Julian

>
> Best Regards
> Ian
>
>>
>>
>> Carsten
>>
>>
>>
>> --
>> Carsten Ziegeler
>> Adobe Research Switzerland
>> cziege...@apache.org
>
>

Re: OAK-6575 - Support for external binary URLs.

[Ian Boston]

Hi,

On 18 September 2017 at 14:17, Carsten Ziegeler 
wrote:

> Ian Boston wrote
> > Hi,
> >
> > On 18 September 2017 at 13:51, Carsten Ziegeler 
> > 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.
> >
> > 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.
> >
> > Adapting to the URIConverter from the ResourceResolver seems a lot
> cleaner,
> > assuming we believe the AdapterFactory pattern is a good pattern.
> >
> I think the adapter factory pattern is a good pattern *if* you need to
> add an
> adaption to a resource outside of the implementation providing that
> resource.
>
> If your code is in the same bundle, then adapter factory usually
> complicates things a little bit as its more expensive to invoke it (and
> there might be subtle service activation races). Now I guess both can be
> neglected, however I think the cleaner way is to not use an adapter
> factory when the resource and the adapter reside in the same bundle.
>
> I see your problem with the HelperData/State object though.
>
> Now, an AdapterFactory is not wrong, so we could first go with this and
> improve over time and see how we can move this into the implementation
> directly.
>


Ok,
btw, this patch can't be merged until there is a suitable Oak release,
which I assume would need to be a 1.5.x release, same as the current
version in Sling.

Best Regards
Ian


>
> Regards
> Carsten
>
>
>
> --
> Carsten Ziegeler
> Adobe Research Switzerland
> cziege...@apache.org
>

Re: OAK-6575 - Support for external binary URLs.

[Carsten Ziegeler]

Ian Boston wrote
> Hi,
> 
> On 18 September 2017 at 13:51, Carsten Ziegeler 
> 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.
> 
> 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.
> 
> Adapting to the URIConverter from the ResourceResolver seems a lot cleaner,
> assuming we believe the AdapterFactory pattern is a good pattern.
> 
I think the adapter factory pattern is a good pattern *if* you need to
add an
adaption to a resource outside of the implementation providing that
resource.

If your code is in the same bundle, then adapter factory usually
complicates things a little bit as its more expensive to invoke it (and
there might be subtle service activation races). Now I guess both can be
neglected, however I think the cleaner way is to not use an adapter
factory when the resource and the adapter reside in the same bundle.

I see your problem with the HelperData/State object though.

Now, an AdapterFactory is not wrong, so we could first go with this and
improve over time and see how we can move this into the implementation
directly.

Regards
Carsten

 

-- 
Carsten Ziegeler
Adobe Research Switzerland
cziege...@apache.org

Re: OAK-6575 - Support for external binary URLs.

[Ian Boston]

Hi,

On 18 September 2017 at 13:51, Carsten Ziegeler 
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.

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.

Adapting to the URIConverter from the ResourceResolver seems a lot cleaner,
assuming we believe the AdapterFactory pattern is a good pattern.

Best Regards
Ian


>
> Carsten
>
>
>
> --
> Carsten Ziegeler
> Adobe Research Switzerland
> cziege...@apache.org
>

Re: OAK-6575 - Support for external binary URLs.

[Ian Boston]

Hi,
I have just updated the patch as the AdapterFactory javadoc was wrong.
 It adapts from ResourceResolver -> URIConverter which is an internal
class, so is only active inside the bundle.
The doc said it adapted from Value  or Resource -> URI which is wrong.

The HelperData class could be used but would still need the URICoverter
class and would need patches to change its constructor and all calls to
that constructor. I think the AdapterFactory approach is cleaner.

Yes, the reason for using this adaption is because the OSGi Service
registry is not available in JcrNodeResource.

Best Regards
Ian

On 18 September 2017 at 13:20, 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.
>
> Regards
> Julian
>
>
> On Mon, Sep 18, 2017 at 1:12 PM, Ian Boston  wrote:
> > Hi,
> > Good catch, didn't implement the vital interface.
> > Put the API into o.a.s.api.resource (o.a.s.resource.api doesn't exist),
> and
> > added some doc. While writing that doc realised there was no way of
> getting
> > a URI for a private network context (if available) so added a method and
> > documented.
> > The component is lazy now.
> > Best Regards
> > Ian
> >
> >
> > On 18 September 2017 at 06:15, Carsten Ziegeler 
> > wrote:
> >
> >> Ian Boston wrote
> >> > Hi,
> >> > Here is an updated patch.
> >> >
> >> > https://github.com/apache/sling/compare/trunk...ieb:OAK-
> >> 6575-3-1?expand=1
> >> >
> >>
> >> Thanks for updating the patch.
> >>
> >> I think we should move ExternalisableInputStream to o.a.s.resource.api,
> >> so other resource providers could potentially use it as well.
> >>
> >> I guess JcrExternalisableInputStream should implement that interface :)
> >>
> >> It's usually not necessary to use "immediate=true" on a component. It
> >> prevents lazy instantiation and should only be used if there is a good
> >> reason for it.
> >>
> >> Regards
> >>
> >> Carsten
> >>
> >> --
> >> Carsten Ziegeler
> >> Adobe Research Switzerland
> >> cziege...@apache.org
> >>
>

Re: OAK-6575 - Support for external binary URLs.

[Carsten Ziegeler]

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.

Carsten

 

-- 
Carsten Ziegeler
Adobe Research Switzerland
cziege...@apache.org

Re: OAK-6575 - Support for external binary URLs.

[Julian Sedding]

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.

Regards
Julian


On Mon, Sep 18, 2017 at 1:12 PM, Ian Boston  wrote:
> Hi,
> Good catch, didn't implement the vital interface.
> Put the API into o.a.s.api.resource (o.a.s.resource.api doesn't exist), and
> added some doc. While writing that doc realised there was no way of getting
> a URI for a private network context (if available) so added a method and
> documented.
> The component is lazy now.
> Best Regards
> Ian
>
>
> On 18 September 2017 at 06:15, Carsten Ziegeler 
> wrote:
>
>> Ian Boston wrote
>> > Hi,
>> > Here is an updated patch.
>> >
>> > https://github.com/apache/sling/compare/trunk...ieb:OAK-
>> 6575-3-1?expand=1
>> >
>>
>> Thanks for updating the patch.
>>
>> I think we should move ExternalisableInputStream to o.a.s.resource.api,
>> so other resource providers could potentially use it as well.
>>
>> I guess JcrExternalisableInputStream should implement that interface :)
>>
>> It's usually not necessary to use "immediate=true" on a component. It
>> prevents lazy instantiation and should only be used if there is a good
>> reason for it.
>>
>> Regards
>>
>> Carsten
>>
>> --
>> Carsten Ziegeler
>> Adobe Research Switzerland
>> cziege...@apache.org
>>

Re: OAK-6575 - Support for external binary URLs.

[Ian Boston]

Hi,
Good catch, didn't implement the vital interface.
Put the API into o.a.s.api.resource (o.a.s.resource.api doesn't exist), and
added some doc. While writing that doc realised there was no way of getting
a URI for a private network context (if available) so added a method and
documented.
The component is lazy now.
Best Regards
Ian


On 18 September 2017 at 06:15, Carsten Ziegeler 
wrote:

> Ian Boston wrote
> > Hi,
> > Here is an updated patch.
> >
> > https://github.com/apache/sling/compare/trunk...ieb:OAK-
> 6575-3-1?expand=1
> >
>
> Thanks for updating the patch.
>
> I think we should move ExternalisableInputStream to o.a.s.resource.api,
> so other resource providers could potentially use it as well.
>
> I guess JcrExternalisableInputStream should implement that interface :)
>
> It's usually not necessary to use "immediate=true" on a component. It
> prevents lazy instantiation and should only be used if there is a good
> reason for it.
>
> Regards
>
> Carsten
>
> --
> Carsten Ziegeler
> Adobe Research Switzerland
> cziege...@apache.org
>

Re: OAK-6575 - Support for external binary URLs.

[Carsten Ziegeler]

Ian Boston wrote
> Hi,
> Here is an updated patch.
> 
> https://github.com/apache/sling/compare/trunk...ieb:OAK-6575-3-1?expand=1
> 

Thanks for updating the patch.

I think we should move ExternalisableInputStream to o.a.s.resource.api,
so other resource providers could potentially use it as well.

I guess JcrExternalisableInputStream should implement that interface :)

It's usually not necessary to use "immediate=true" on a component. It
prevents lazy instantiation and should only be used if there is a good
reason for it.

Regards

Carsten

-- 
Carsten Ziegeler
Adobe Research Switzerland
cziege...@apache.org

Re: OAK-6575 - Support for external binary URLs.

[Ian Boston]

Hi,

On 14 September 2017 at 06:02, Julian Reschke  wrote:

> On 2017-09-14 14:54, Ian Boston wrote:
>
>> Hi,
>>
>> On 14 Sep 2017 12:22 am, "Julian Reschke" > julian.resc...@gmx.de>> wrote:
>>
>> On 2017-09-14 01:29, Ian Boston wrote:
>>
>> Hi,
>> Here is an updated patch.
>>
>> https://github.com/apache/sling/compare/trunk...ieb:OAK-6575
>> -3-1?expand=1
>> > 5-3-1?expand=1>
>>
>> Best Regards
>> Ian
>>
>>
>> 1) I think it would be good to only redirect HEAD and GET (may
>> already be the case...)
>>
>>
>> This is already the case, but looking at the code, should it redirect for
>> HEAD, or should HEAD still be served by Sling ?  (I havent checked the
>> spec). Sling has all the information needed for a HEAD response, although
>> it might not be exactly the same as the HEAD response from the target of
>> the redirect. If the target of the redirect is CloudFront, then I would
>> expect it to respond with headers consistent with its behaviour.
>>
>
> Realistically, callers would be interested in media type and content
> length. For these, both should be correct, right? In which case I think not
> doing the redirect would be the simpler approach.
>
> I would also expect the target of the redirect to change, as the signature
>> in the query params will change on each fresh redirect. Is that going to
>> cause a problem ?
>>
>
> It the client keeps the URI and then does a GET, would it still work?
>

While the TTL is still valid, yes. After that no, but this is
implementation specific, and controlled by Oak (or the DS implementation
inside Oak)

In the case of CloudFront signed urls, the signature of the URL with a
canned policy. The policy is enforced by CloudFront and (afaik) allows both
HEAD and GET requests, but not any other. So a client could store the
redirect and use it later, provided the signature had not expired. The
client could not use the URL to perform a POST or anything else.

It is possible to sign a URL with custom policies, but Oak wont do this,
probably ever.

Best Regards
Ian




>
> ...
>>
>
> Best regards, Julian
>

Re: OAK-6575 - Support for external binary URLs.

[Julian Reschke]

On 2017-09-14 14:54, Ian Boston wrote:

Hi,

On 14 Sep 2017 12:22 am, "Julian Reschke" > wrote:


On 2017-09-14 01:29, Ian Boston wrote:

Hi,
Here is an updated patch.


https://github.com/apache/sling/compare/trunk...ieb:OAK-6575-3-1?expand=1



Best Regards
Ian


1) I think it would be good to only redirect HEAD and GET (may
already be the case...)


This is already the case, but looking at the code, should it redirect 
for HEAD, or should HEAD still be served by Sling ?  (I havent checked 
the spec). Sling has all the information needed for a HEAD response, 
although it might not be exactly the same as the HEAD response from the 
target of the redirect. If the target of the redirect is CloudFront, 
then I would expect it to respond with headers consistent with its 
behaviour.


Realistically, callers would be interested in media type and content 
length. For these, both should be correct, right? In which case I think 
not doing the redirect would be the simpler approach.


I would also expect the target of the redirect to change, as the 
signature in the query params will change on each fresh redirect. Is 
that going to cause a problem ?


It the client keeps the URI and then does a GET, would it still work?


...


Best regards, Julian

Re: OAK-6575 - Support for external binary URLs.

[Ian Boston]

Hi,

On 14 Sep 2017 12:22 am, "Julian Reschke"  wrote:

On 2017-09-14 01:29, Ian Boston wrote:

> Hi,
> Here is an updated patch.
>
> https://github.com/apache/sling/compare/trunk...ieb:OAK-6575-3-1?expand=1
>
> Best Regards
> Ian
>

1) I think it would be good to only redirect HEAD and GET (may already be
the case...)


This is already the case, but looking at the code, should it redirect for
HEAD, or should HEAD still be served by Sling ?  (I havent checked the
spec). Sling has all the information needed for a HEAD response, although
it might not be exactly the same as the HEAD response from the target of
the redirect. If the target of the redirect is CloudFront, then I would
expect it to respond with headers consistent with its behaviour.

I would also expect the target of the redirect to change, as the signature
in the query params will change on each fresh redirect. Is that going to
cause a problem ?

At the moment, it does redirect for HEAD.
https://github.com/apache/sling/compare/trunk...ieb:OAK-6575-3-1?expand=1#diff-1af2bf2ffe55daf032f4c7419a84576fR171



2) For robustness in case a redirect *does* happen for POST, HTTP status
307 seems to be more correct than the default 302.

Best regards, Julian


StreamRenderServlet implements SlingSafeMethodsServlet so doenst handl POST
requests.

https://github.com/ieb/sling/blob/a491430a8b30ec84ef66985f3af81a758eab6022/bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/helpers/StreamRendererServlet.java#L64

Best Regards
Ian

Re: OAK-6575 - Support for external binary URLs.

[Julian Reschke]

On 2017-09-14 01:29, Ian Boston wrote:

Hi,
Here is an updated patch.

https://github.com/apache/sling/compare/trunk...ieb:OAK-6575-3-1?expand=1

Best Regards
Ian


1) I think it would be good to only redirect HEAD and GET (may already 
be the case...)


2) For robustness in case a redirect *does* happen for POST, HTTP status 
307 seems to be more correct than the default 302.


Best regards, Julian

Re: OAK-6575 - Support for external binary URLs.

[Ian Boston]

Hi,
Here is an updated patch.

https://github.com/apache/sling/compare/trunk...ieb:OAK-6575-3-1?expand=1

Best Regards
Ian



On 12 September 2017 at 17:39, Carsten Ziegeler 
wrote:

> Ian Boston wrote
> > Hi,
> >
> > On 12 September 2017 at 17:09, Carsten Ziegeler  > > wrote:
> >
> > I think there are two parts to be discussed:
> > a) whether to directly code this into the get servlet
> > b) how to mark the resource as being handled this way
> >
> > I think a) makes sense, it's a central place and allows any resource
> > provider to use it. However, this is also the problem I see :) For
> each
> > and every resource the adaption is tried, punishing basically every
> get
> > request handled by this. This might be neglectable, but we should
> think
> > about it.
> >
> >
> > The cost of checking is the Resource is a binary resource should be
> > almost identical to the adaptTo(InputStream.class) which happens
> > already, If its done the same way everything will be in memory already
> > so that part probably wont add anything. This part is sub almost
> > certainly sub 1ms and doesn't cause and additional objects to be loaded
> > by Oak.
> >
> > Same goes for a Resource that can be adapted to an InputStream, upto the
> > point where the InputStream is created.
> > The cost of signing the a URL using a shared private key is sub 1ms on
> > my MBP. creating the private key object is expensive, but that is done
> > in the activate inside Oak.
> >
> > All of this needs to be checked on realistic hardware, not a
> > unrealistically fast laptop.
>
> Right, I agree this shouldn't pose a problem in reality and is more of a
> theoretical nature. However, it doubles the adaptTo calls (first URI,
> then InputStream). So this part is doubled. Again, I think it can be
> neglected but we should be aware of it.
>
> >
> >
> >
> > For b) I think its good to use an approach which does not require new
> > API. However, any resource could somehow implement adaption to a URI.
> > That might be a URI which can only be resolved by a custom URI
> handler
> > or it might be a URI which can't be resolved at all or it might be an
> > external URI in any form. For example the full URI to access this
> > resource. So if a redirect would happen to this URI, it would be an
> > endless loop.
> > So just adapting to a URI might not be enough to detect this case.
> >
> > I'm just throwing in a wild idea which I haven't thought fully
> through
> > yet to foster a discussion: what about we add an
> > ExternalizableInputStream to our API. It extends InputStream by a
> > getURI() method.
> > In the get servlet we adapt to InputStream as today. If it is an
> > instance of ExternalizableInputStream getURI is called, if not, the
> > input stream is used (if provided).
> >
> >
> > This would solve the potential performance problem of a) and gives a
> > clear opt-in for b).
> >
> >
> >
> > Agreed.
> > Sounds like the  org.apache.sling.jcr.resource bundle needs to use
> > URIProvider where it does the Resource -> InputStream adaptation, rather
> > than a new custom bundle.
> > Is that what you were thinking of ?
>
> Yes, right.
>
> >
> > If so, would ExternalizableInputStream be in the Resource API or
> > somewhere else ?
>
> I think somewhere in the resource API, not quiet sure where but we'll
> find a nice spot.
>
> Regards
> Carsten
>
> >
> >
> > Best Regards
> > Ian
> >
> >
> >
> >
> >
> > Regards
> > Carsten
> >
> > Ian Boston wrote
> > > Hi,
> > >
> > > Background:
> > > OAK-6575 adds support for a Oak DataStore to implement a service
> > > implementing  org.apache.jackrabbit.oak.api.conversion.URIProvider
> > which
> > > converts a JCR Value to a URI. The implementation in OAK-6575 is
> > for the
> > > Oak S3 DataStore to convert to a CloudFront signed url as detailed
> > in [1].
> > > The URI the URIProvider emits in this case expires quickly (<
> > 60s), only
> > > allows GET and is only issued to a JCR session that can read the
> > binary. It
> > > is intended to be used as a redirect, the flow being.
> > >
> > > Client > Sling -> GET 
> > >   <- redirect to CF sifgned url
> > > Client -> CF -> GET 
> > >
> > > To patch[2] has been discussed at length on Oak and now is close
> > to being
> > > acceptable.
> > >
> > > To make this work in Sling requires a patch to Sling, which needs
> > > discussion. The assumptions are that it should work seamlessly and
> not
> > > require any core component to depend on the Oak API. One patch is
> > [3]. It
> > > adds a new bundle that adds and AdapterFactory that adapts from
> > Resource to
> > > URI. It tries all URIProviders registered by Oak and if one
> > provides a URI.
> > >
> > > Inside the Get Servlets streaming helper, 

Re: OAK-6575 - Support for external binary URLs.

[Carsten Ziegeler]

Ian Boston wrote
> Hi,
> 
> On 12 September 2017 at 17:09, Carsten Ziegeler  > wrote:
> 
> I think there are two parts to be discussed:
> a) whether to directly code this into the get servlet
> b) how to mark the resource as being handled this way
> 
> I think a) makes sense, it's a central place and allows any resource
> provider to use it. However, this is also the problem I see :) For each
> and every resource the adaption is tried, punishing basically every get
> request handled by this. This might be neglectable, but we should think
> about it.
> 
> 
> The cost of checking is the Resource is a binary resource should be
> almost identical to the adaptTo(InputStream.class) which happens
> already, If its done the same way everything will be in memory already
> so that part probably wont add anything. This part is sub almost
> certainly sub 1ms and doesn't cause and additional objects to be loaded
> by Oak.
> 
> Same goes for a Resource that can be adapted to an InputStream, upto the
> point where the InputStream is created.
> The cost of signing the a URL using a shared private key is sub 1ms on
> my MBP. creating the private key object is expensive, but that is done
> in the activate inside Oak.
> 
> All of this needs to be checked on realistic hardware, not a
> unrealistically fast laptop.

Right, I agree this shouldn't pose a problem in reality and is more of a
theoretical nature. However, it doubles the adaptTo calls (first URI,
then InputStream). So this part is doubled. Again, I think it can be
neglected but we should be aware of it.

>  
> 
> 
> For b) I think its good to use an approach which does not require new
> API. However, any resource could somehow implement adaption to a URI.
> That might be a URI which can only be resolved by a custom URI handler
> or it might be a URI which can't be resolved at all or it might be an
> external URI in any form. For example the full URI to access this
> resource. So if a redirect would happen to this URI, it would be an
> endless loop.
> So just adapting to a URI might not be enough to detect this case.
> 
> I'm just throwing in a wild idea which I haven't thought fully through
> yet to foster a discussion: what about we add an
> ExternalizableInputStream to our API. It extends InputStream by a
> getURI() method.
> In the get servlet we adapt to InputStream as today. If it is an
> instance of ExternalizableInputStream getURI is called, if not, the
> input stream is used (if provided).
> 
> 
> This would solve the potential performance problem of a) and gives a
> clear opt-in for b).
> 
> 
> 
> Agreed. 
> Sounds like the  org.apache.sling.jcr.resource bundle needs to use
> URIProvider where it does the Resource -> InputStream adaptation, rather
> than a new custom bundle.
> Is that what you were thinking of ?

Yes, right.

> 
> If so, would ExternalizableInputStream be in the Resource API or
> somewhere else ?

I think somewhere in the resource API, not quiet sure where but we'll
find a nice spot.

Regards
Carsten

> 
> 
> Best Regards
> Ian
> 
> 
>  
> 
> 
> Regards
> Carsten
> 
> Ian Boston wrote
> > Hi,
> >
> > Background:
> > OAK-6575 adds support for a Oak DataStore to implement a service
> > implementing  org.apache.jackrabbit.oak.api.conversion.URIProvider
> which
> > converts a JCR Value to a URI. The implementation in OAK-6575 is
> for the
> > Oak S3 DataStore to convert to a CloudFront signed url as detailed
> in [1].
> > The URI the URIProvider emits in this case expires quickly (<
> 60s), only
> > allows GET and is only issued to a JCR session that can read the
> binary. It
> > is intended to be used as a redirect, the flow being.
> >
> > Client > Sling -> GET 
> >           <- redirect to CF sifgned url
> > Client -> CF -> GET 
> >
> > To patch[2] has been discussed at length on Oak and now is close
> to being
> > acceptable.
> >
> > To make this work in Sling requires a patch to Sling, which needs
> > discussion. The assumptions are that it should work seamlessly and not
> > require any core component to depend on the Oak API. One patch is
> [3]. It
> > adds a new bundle that adds and AdapterFactory that adapts from
> Resource to
> > URI. It tries all URIProviders registered by Oak and if one
> provides a URI.
> >
> > Inside the Get Servlets streaming helper, if a resource is
> adaptable to a
> > URI, that URI is used as a redirect rather than streaming the
> binaries.
> >
> > I think this patch needs discussion before being committed, hence this
> > thread.
> >
> > Best Regards
> > Ian
> >
> >
> >
> > 1
> >
> 
> http://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/PrivateContent.html
> 
> 

Re: OAK-6575 - Support for external binary URLs.

[Ian Boston]

Hi,

On 12 September 2017 at 17:09, Carsten Ziegeler 
wrote:

> I think there are two parts to be discussed:
> a) whether to directly code this into the get servlet
> b) how to mark the resource as being handled this way
>
> I think a) makes sense, it's a central place and allows any resource
> provider to use it. However, this is also the problem I see :) For each
> and every resource the adaption is tried, punishing basically every get
> request handled by this. This might be neglectable, but we should think
> about it.
>

The cost of checking is the Resource is a binary resource should be almost
identical to the adaptTo(InputStream.class) which happens already, If its
done the same way everything will be in memory already so that part
probably wont add anything. This part is sub almost certainly sub 1ms and
doesn't cause and additional objects to be loaded by Oak.

Same goes for a Resource that can be adapted to an InputStream, upto the
point where the InputStream is created.
The cost of signing the a URL using a shared private key is sub 1ms on my
MBP. creating the private key object is expensive, but that is done in the
activate inside Oak.

All of this needs to be checked on realistic hardware, not a
unrealistically fast laptop.


>
> For b) I think its good to use an approach which does not require new
> API. However, any resource could somehow implement adaption to a URI.
> That might be a URI which can only be resolved by a custom URI handler
> or it might be a URI which can't be resolved at all or it might be an
> external URI in any form. For example the full URI to access this
> resource. So if a redirect would happen to this URI, it would be an
> endless loop.
> So just adapting to a URI might not be enough to detect this case.
>
> I'm just throwing in a wild idea which I haven't thought fully through
> yet to foster a discussion: what about we add an
> ExternalizableInputStream to our API. It extends InputStream by a
> getURI() method.
> In the get servlet we adapt to InputStream as today. If it is an
> instance of ExternalizableInputStream getURI is called, if not, the
> input stream is used (if provided).


> This would solve the potential performance problem of a) and gives a
> clear opt-in for b).
>


Agreed.
Sounds like the  org.apache.sling.jcr.resource bundle needs to use
URIProvider where it does the Resource -> InputStream adaptation, rather
than a new custom bundle.
Is that what you were thinking of ?

If so, would ExternalizableInputStream be in the Resource API or somewhere
else ?


Best Regards
Ian




>
> Regards
> Carsten
>
> Ian Boston wrote
> > Hi,
> >
> > Background:
> > OAK-6575 adds support for a Oak DataStore to implement a service
> > implementing  org.apache.jackrabbit.oak.api.conversion.URIProvider which
> > converts a JCR Value to a URI. The implementation in OAK-6575 is for the
> > Oak S3 DataStore to convert to a CloudFront signed url as detailed in
> [1].
> > The URI the URIProvider emits in this case expires quickly (< 60s), only
> > allows GET and is only issued to a JCR session that can read the binary.
> It
> > is intended to be used as a redirect, the flow being.
> >
> > Client > Sling -> GET 
> >   <- redirect to CF sifgned url
> > Client -> CF -> GET 
> >
> > To patch[2] has been discussed at length on Oak and now is close to being
> > acceptable.
> >
> > To make this work in Sling requires a patch to Sling, which needs
> > discussion. The assumptions are that it should work seamlessly and not
> > require any core component to depend on the Oak API. One patch is [3]. It
> > adds a new bundle that adds and AdapterFactory that adapts from Resource
> to
> > URI. It tries all URIProviders registered by Oak and if one provides a
> URI.
> >
> > Inside the Get Servlets streaming helper, if a resource is adaptable to a
> > URI, that URI is used as a redirect rather than streaming the binaries.
> >
> > I think this patch needs discussion before being committed, hence this
> > thread.
> >
> > Best Regards
> > Ian
> >
> >
> >
> > 1
> > http://docs.aws.amazon.com/AmazonCloudFront/latest/
> DeveloperGuide/PrivateContent.html
> > 2
> > https://github.com/ieb/jackrabbit-oak/compare/trunk..
> .ieb:OAK-6575-3?expand=1
> > 3 https://github.com/apache/sling/compare/trunk...ieb:OAK-
> 6575-3?expand=1
> >
>
>
>
>
> --
> Carsten Ziegeler
> Adobe Research Switzerland
> cziege...@apache.org
>

Re: OAK-6575 - Support for external binary URLs.

[Carsten Ziegeler]

I think there are two parts to be discussed:
a) whether to directly code this into the get servlet
b) how to mark the resource as being handled this way

I think a) makes sense, it's a central place and allows any resource
provider to use it. However, this is also the problem I see :) For each
and every resource the adaption is tried, punishing basically every get
request handled by this. This might be neglectable, but we should think
about it.

For b) I think its good to use an approach which does not require new
API. However, any resource could somehow implement adaption to a URI.
That might be a URI which can only be resolved by a custom URI handler
or it might be a URI which can't be resolved at all or it might be an
external URI in any form. For example the full URI to access this
resource. So if a redirect would happen to this URI, it would be an
endless loop.
So just adapting to a URI might not be enough to detect this case.

I'm just throwing in a wild idea which I haven't thought fully through
yet to foster a discussion: what about we add an
ExternalizableInputStream to our API. It extends InputStream by a
getURI() method.
In the get servlet we adapt to InputStream as today. If it is an
instance of ExternalizableInputStream getURI is called, if not, the
input stream is used (if provided).

This would solve the potential performance problem of a) and gives a
clear opt-in for b).

Regards
Carsten

Ian Boston wrote
> Hi,
> 
> Background:
> OAK-6575 adds support for a Oak DataStore to implement a service
> implementing  org.apache.jackrabbit.oak.api.conversion.URIProvider which
> converts a JCR Value to a URI. The implementation in OAK-6575 is for the
> Oak S3 DataStore to convert to a CloudFront signed url as detailed in [1].
> The URI the URIProvider emits in this case expires quickly (< 60s), only
> allows GET and is only issued to a JCR session that can read the binary. It
> is intended to be used as a redirect, the flow being.
> 
> Client > Sling -> GET 
>   <- redirect to CF sifgned url
> Client -> CF -> GET 
> 
> To patch[2] has been discussed at length on Oak and now is close to being
> acceptable.
> 
> To make this work in Sling requires a patch to Sling, which needs
> discussion. The assumptions are that it should work seamlessly and not
> require any core component to depend on the Oak API. One patch is [3]. It
> adds a new bundle that adds and AdapterFactory that adapts from Resource to
> URI. It tries all URIProviders registered by Oak and if one provides a URI.
> 
> Inside the Get Servlets streaming helper, if a resource is adaptable to a
> URI, that URI is used as a redirect rather than streaming the binaries.
> 
> I think this patch needs discussion before being committed, hence this
> thread.
> 
> Best Regards
> Ian
> 
> 
> 
> 1
> http://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/PrivateContent.html
> 2
> https://github.com/ieb/jackrabbit-oak/compare/trunk...ieb:OAK-6575-3?expand=1
> 3 https://github.com/apache/sling/compare/trunk...ieb:OAK-6575-3?expand=1
> 


 

-- 
Carsten Ziegeler
Adobe Research Switzerland
cziege...@apache.org

OAK-6575 - Support for external binary URLs.

[Ian Boston]

Hi,

Background:
OAK-6575 adds support for a Oak DataStore to implement a service
implementing  org.apache.jackrabbit.oak.api.conversion.URIProvider which
converts a JCR Value to a URI. The implementation in OAK-6575 is for the
Oak S3 DataStore to convert to a CloudFront signed url as detailed in [1].
The URI the URIProvider emits in this case expires quickly (< 60s), only
allows GET and is only issued to a JCR session that can read the binary. It
is intended to be used as a redirect, the flow being.

Client > Sling -> GET 
  <- redirect to CF sifgned url
Client -> CF -> GET 

To patch[2] has been discussed at length on Oak and now is close to being
acceptable.

To make this work in Sling requires a patch to Sling, which needs
discussion. The assumptions are that it should work seamlessly and not
require any core component to depend on the Oak API. One patch is [3]. It
adds a new bundle that adds and AdapterFactory that adapts from Resource to
URI. It tries all URIProviders registered by Oak and if one provides a URI.

Inside the Get Servlets streaming helper, if a resource is adaptable to a
URI, that URI is used as a redirect rather than streaming the binaries.

I think this patch needs discussion before being committed, hence this
thread.

Best Regards
Ian



1
http://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/PrivateContent.html
2
https://github.com/ieb/jackrabbit-oak/compare/trunk...ieb:OAK-6575-3?expand=1
3 https://github.com/apache/sling/compare/trunk...ieb:OAK-6575-3?expand=1