You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Ian Boston <ia...@gmail.com> on 2020/01/29 18:46:28 UTC
Redirect Provider
Hi,
I have been talking with Carsten and Robert offline about a better solution
to replace the URIProvider which to be frank, isn't really working for us.
For those that don't know, its used inside the Slng Default Get servlet to
allow that servlet to return a redirect instead of sending a stream of
bytes. Typically the redirect would be a to a service better suited to
streaming the bytes of a binary.
The problem with the URIProvider is the API only sees a Resource so can
determine the context of the request, the FQDN or any other information
about the request.
Initially I suggested adding a method to the URIProvider, but Carsten said
that would break implementations.
We discussed deprecating URIProvider so that anyone using it can still use
it, but adding a new provider (ie new interface) that can given a request
from which a provider implementation would return a status code, and
headers. If the status code is not -1 then the headers would added to the
response and the response returned.
Initially I suggested both request and response objects were give to the
provider. Carsten preferred not to give the response object as this could
allow he provider implementation to do too much.
ie
interface RedirectProvider {
/**
* @param request The request
* @param responseHeaders a list of headers to be filled by the provider
* and added to the response if the return is a http status
code.
* @returns -1 if no action to be taken, or a http status code if a
response is to
* created with that status code.
*/
int getRedirect(HttpServletRequest request, List<String[]> responseHeaders);
}
wdyt? Is this something Sling can consider. If so I would volunteer to do a
PR, but I would not want to invest the effort if there is no appetite.
Best Regards
Ian
Re: Redirect Provider
Posted by Robert Munteanu <ro...@apache.org>.
On Thu, 2020-01-30 at 09:52 +0100, Radu Cotescu wrote:
> Could we, at least for new APIs, stop using adaptTo? (I started
> running away from any physical location known by Sling committers)
I think that requires a separate discussion, as I don't see consensus
on that.
Thanks,
Robert
Re: Redirect Provider
Posted by Carsten Ziegeler <cz...@apache.org>.
Casting is bad, but as RedirectResolver is the result of adapting a
resource, the implementation has access to the resource already. So no
need for SlingHttpServletRequest or casting etc.
Regards
Carsten
On 31.01.2020 01:59, Ian Boston wrote:
> Hi,
> NO_REDIRECT, good point, added.
>
> I think HttpServletRequest is better as it is wider.
> If an implementation of a RedirectResolver requires SlingHttpServletRequest
> it can check for that and cast to it, returning NO_REDIRECT if the context
> isn't what it required.
>
> Best Regards
> Ian
>
>
> On Fri, 31 Jan 2020 at 08:49, Bertrand Delacretaz <bd...@apache.org>
> wrote:
>
>> Hi,
>>
>> On Fri, Jan 31, 2020 at 9:11 AM Ian Boston <ie...@tfd.co.uk> wrote:
>> ..
>>> 1 https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621
>> ...
>>
>> The RedirectResolver gets an HttpServletRequest, shouldn't that be a
>> SlingHttpServletRequest to be able to get at the Resource, selectors
>> etc?
>>
>> I understand the Resource can come from having used adaptTo before,
>> but other request attributes might influence the redirect and we have
>> them already parsed in SlingHttpServletRequest.
>>
>> And a nitpick, in RedirectResponse I would make -1 a constant, NO_REDIRECT.
>>
>> -Bertrand
>>
>
--
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: Redirect Provider
Posted by Robert Munteanu <ro...@apache.org>.
On Tue, 2020-02-04 at 10:14 +0100, Bertrand Delacretaz wrote:
> On Tue, Feb 4, 2020 at 9:54 AM Radu Cotescu <ra...@apache.org> wrote:
> > ...After Carsten’s detailed reasoning I guess it makes the most
> > sense to put this new
> > API in o.a.s.api, where the Resource and ResourceProvider APIs are
> > also defined...
>
> But ideally in a new package? To avoid bumping up versions of
> existing
> packages, which often has unfortunate ripple effects.
+1
Robert
Re: Redirect Provider
Posted by Ian Boston <ie...@tfd.co.uk>.
Hi,
After discussing internally I have updated the patch to use and interface,
that hopefully will unblock the -1. The PR and issue has been reopened.
Best Regards
Ian
On Fri, 14 Feb 2020 at 15:06, Ian Boston <ie...@tfd.co.uk> wrote:
> Hi,
> The API PR attracted a -1 which I understand and respect.
>
> I cant see of a way or revolving the -1 without making the API
> unsupportable long term, hence I have closed the PRs and closed SLING-9060
> as a Wont fix and will look for alternative ways of solving the problem.
> Best Regards
> Ian
>
> On Mon, 10 Feb 2020 at 16:28, Ian Boston <ie...@tfd.co.uk> wrote:
>
>> > Let me know whether that is enough or it needs expansions.
>>
>> +1, works.
>>
>> Interestingly it did need all occurrences of bundles pointing to SNAPSHOT
>> Sling API bundles to be updated, and for those bundles to depend on the
>> latest snapshot of the API. scripting.core and servlets.resolver had to be
>> fixed before the tests would pass.
>>
>> Thanks for the comments on the PRs. I have tried to address them with
>> more commits.
>> Best Regards
>> Ian
>>
>> On Sat, 8 Feb 2020 at 12:18, Robert Munteanu <ro...@apache.org> wrote:
>>
>>> Hi Ian,
>>>
>>> On Sat, 2020-02-08 at 09:48 +0000, Ian Boston wrote:
>>> > Hi,
>>> > The PRs have been opened, see the issue.
>>> > I will do complete implementations to verify it works as required in
>>> > AEM.
>>> > I would also like to insure no disruption to Sling, and since it's
>>> > been a
>>> > long time since I did this, how do run full CI/CD and integrate a new
>>> > API
>>> > in Sling ? (a pointer would be great).
>>> >
>>>
>>> I have added some instructions at
>>>
>>> https://sling.apache.org/contributing.html#testing
>>>
>>> Let me know whether that is enough or it needs expansions.
>>>
>>> Thanks,
>>> Robert
>>>
>>> > Best Regards
>>> > Ian
>>> >
>>> >
>>> > On Sat, 8 Feb 2020 at 09:23, Ian Boston <ie...@tfd.co.uk> wrote:
>>> >
>>> > > Hi,
>>> > > Opened [1] to track the work.
>>> > > Best Regards
>>> > > Ian
>>> > >
>>> > > 1 https://issues.apache.org/jira/browse/SLING-9060
>>> > >
>>> > > On Wed, 5 Feb 2020 at 22:12, Ian Boston <ie...@tfd.co.uk> wrote:
>>> > >
>>> > > > Hi,
>>> > > > Ok.
>>> > > >
>>> > > > o.a.s.api.redirect for the API, and a patch to the Get servlet to
>>> > > > use the
>>> > > > API.
>>> > > > Will do PRs shortly.
>>> > > >
>>> > > > Best Regards
>>> > > > Ian
>>> > > >
>>> > > >
>>> > > > On Tue, 4 Feb 2020 at 14:13, Bertrand Delacretaz <
>>> > > > bdelacretaz@apache.org>
>>> > > > wrote:
>>> > > >
>>> > > > > On Tue, Feb 4, 2020 at 1:58 PM Ian Boston <ie...@tfd.co.uk>
>>> > > > > wrote:
>>> > > > > > ...New package, agreed, no ripples.
>>> > > > > > o.a.s.api.response ? ...
>>> > > > >
>>> > > > > o.a.s.api.redirect is more specific and I think having packages
>>> > > > > with
>>> > > > > few interfaces is fine?
>>> > > > >
>>> > > > > The existing api.auth and api.security packages for example
>>> > > > > only have
>>> > > > > 3 items each.
>>> > > > >
>>> > > > > It has the benefit of OSGi package versioning being very
>>> > > > > specific.
>>> > > > >
>>> > > > > -Bertrand
>>> > > > >
>>>
>>>
Re: Redirect Provider
Posted by Ian Boston <ie...@tfd.co.uk>.
Hi,
The API PR attracted a -1 which I understand and respect.
I cant see of a way or revolving the -1 without making the API
unsupportable long term, hence I have closed the PRs and closed SLING-9060
as a Wont fix and will look for alternative ways of solving the problem.
Best Regards
Ian
On Mon, 10 Feb 2020 at 16:28, Ian Boston <ie...@tfd.co.uk> wrote:
> > Let me know whether that is enough or it needs expansions.
>
> +1, works.
>
> Interestingly it did need all occurrences of bundles pointing to SNAPSHOT
> Sling API bundles to be updated, and for those bundles to depend on the
> latest snapshot of the API. scripting.core and servlets.resolver had to be
> fixed before the tests would pass.
>
> Thanks for the comments on the PRs. I have tried to address them with more
> commits.
> Best Regards
> Ian
>
> On Sat, 8 Feb 2020 at 12:18, Robert Munteanu <ro...@apache.org> wrote:
>
>> Hi Ian,
>>
>> On Sat, 2020-02-08 at 09:48 +0000, Ian Boston wrote:
>> > Hi,
>> > The PRs have been opened, see the issue.
>> > I will do complete implementations to verify it works as required in
>> > AEM.
>> > I would also like to insure no disruption to Sling, and since it's
>> > been a
>> > long time since I did this, how do run full CI/CD and integrate a new
>> > API
>> > in Sling ? (a pointer would be great).
>> >
>>
>> I have added some instructions at
>>
>> https://sling.apache.org/contributing.html#testing
>>
>> Let me know whether that is enough or it needs expansions.
>>
>> Thanks,
>> Robert
>>
>> > Best Regards
>> > Ian
>> >
>> >
>> > On Sat, 8 Feb 2020 at 09:23, Ian Boston <ie...@tfd.co.uk> wrote:
>> >
>> > > Hi,
>> > > Opened [1] to track the work.
>> > > Best Regards
>> > > Ian
>> > >
>> > > 1 https://issues.apache.org/jira/browse/SLING-9060
>> > >
>> > > On Wed, 5 Feb 2020 at 22:12, Ian Boston <ie...@tfd.co.uk> wrote:
>> > >
>> > > > Hi,
>> > > > Ok.
>> > > >
>> > > > o.a.s.api.redirect for the API, and a patch to the Get servlet to
>> > > > use the
>> > > > API.
>> > > > Will do PRs shortly.
>> > > >
>> > > > Best Regards
>> > > > Ian
>> > > >
>> > > >
>> > > > On Tue, 4 Feb 2020 at 14:13, Bertrand Delacretaz <
>> > > > bdelacretaz@apache.org>
>> > > > wrote:
>> > > >
>> > > > > On Tue, Feb 4, 2020 at 1:58 PM Ian Boston <ie...@tfd.co.uk>
>> > > > > wrote:
>> > > > > > ...New package, agreed, no ripples.
>> > > > > > o.a.s.api.response ? ...
>> > > > >
>> > > > > o.a.s.api.redirect is more specific and I think having packages
>> > > > > with
>> > > > > few interfaces is fine?
>> > > > >
>> > > > > The existing api.auth and api.security packages for example
>> > > > > only have
>> > > > > 3 items each.
>> > > > >
>> > > > > It has the benefit of OSGi package versioning being very
>> > > > > specific.
>> > > > >
>> > > > > -Bertrand
>> > > > >
>>
>>
Re: Redirect Provider
Posted by Ian Boston <ie...@tfd.co.uk>.
> Let me know whether that is enough or it needs expansions.
+1, works.
Interestingly it did need all occurrences of bundles pointing to SNAPSHOT
Sling API bundles to be updated, and for those bundles to depend on the
latest snapshot of the API. scripting.core and servlets.resolver had to be
fixed before the tests would pass.
Thanks for the comments on the PRs. I have tried to address them with more
commits.
Best Regards
Ian
On Sat, 8 Feb 2020 at 12:18, Robert Munteanu <ro...@apache.org> wrote:
> Hi Ian,
>
> On Sat, 2020-02-08 at 09:48 +0000, Ian Boston wrote:
> > Hi,
> > The PRs have been opened, see the issue.
> > I will do complete implementations to verify it works as required in
> > AEM.
> > I would also like to insure no disruption to Sling, and since it's
> > been a
> > long time since I did this, how do run full CI/CD and integrate a new
> > API
> > in Sling ? (a pointer would be great).
> >
>
> I have added some instructions at
>
> https://sling.apache.org/contributing.html#testing
>
> Let me know whether that is enough or it needs expansions.
>
> Thanks,
> Robert
>
> > Best Regards
> > Ian
> >
> >
> > On Sat, 8 Feb 2020 at 09:23, Ian Boston <ie...@tfd.co.uk> wrote:
> >
> > > Hi,
> > > Opened [1] to track the work.
> > > Best Regards
> > > Ian
> > >
> > > 1 https://issues.apache.org/jira/browse/SLING-9060
> > >
> > > On Wed, 5 Feb 2020 at 22:12, Ian Boston <ie...@tfd.co.uk> wrote:
> > >
> > > > Hi,
> > > > Ok.
> > > >
> > > > o.a.s.api.redirect for the API, and a patch to the Get servlet to
> > > > use the
> > > > API.
> > > > Will do PRs shortly.
> > > >
> > > > Best Regards
> > > > Ian
> > > >
> > > >
> > > > On Tue, 4 Feb 2020 at 14:13, Bertrand Delacretaz <
> > > > bdelacretaz@apache.org>
> > > > wrote:
> > > >
> > > > > On Tue, Feb 4, 2020 at 1:58 PM Ian Boston <ie...@tfd.co.uk>
> > > > > wrote:
> > > > > > ...New package, agreed, no ripples.
> > > > > > o.a.s.api.response ? ...
> > > > >
> > > > > o.a.s.api.redirect is more specific and I think having packages
> > > > > with
> > > > > few interfaces is fine?
> > > > >
> > > > > The existing api.auth and api.security packages for example
> > > > > only have
> > > > > 3 items each.
> > > > >
> > > > > It has the benefit of OSGi package versioning being very
> > > > > specific.
> > > > >
> > > > > -Bertrand
> > > > >
>
>
Re: Redirect Provider
Posted by Robert Munteanu <ro...@apache.org>.
Hi Ian,
On Sat, 2020-02-08 at 09:48 +0000, Ian Boston wrote:
> Hi,
> The PRs have been opened, see the issue.
> I will do complete implementations to verify it works as required in
> AEM.
> I would also like to insure no disruption to Sling, and since it's
> been a
> long time since I did this, how do run full CI/CD and integrate a new
> API
> in Sling ? (a pointer would be great).
>
I have added some instructions at
https://sling.apache.org/contributing.html#testing
Let me know whether that is enough or it needs expansions.
Thanks,
Robert
> Best Regards
> Ian
>
>
> On Sat, 8 Feb 2020 at 09:23, Ian Boston <ie...@tfd.co.uk> wrote:
>
> > Hi,
> > Opened [1] to track the work.
> > Best Regards
> > Ian
> >
> > 1 https://issues.apache.org/jira/browse/SLING-9060
> >
> > On Wed, 5 Feb 2020 at 22:12, Ian Boston <ie...@tfd.co.uk> wrote:
> >
> > > Hi,
> > > Ok.
> > >
> > > o.a.s.api.redirect for the API, and a patch to the Get servlet to
> > > use the
> > > API.
> > > Will do PRs shortly.
> > >
> > > Best Regards
> > > Ian
> > >
> > >
> > > On Tue, 4 Feb 2020 at 14:13, Bertrand Delacretaz <
> > > bdelacretaz@apache.org>
> > > wrote:
> > >
> > > > On Tue, Feb 4, 2020 at 1:58 PM Ian Boston <ie...@tfd.co.uk>
> > > > wrote:
> > > > > ...New package, agreed, no ripples.
> > > > > o.a.s.api.response ? ...
> > > >
> > > > o.a.s.api.redirect is more specific and I think having packages
> > > > with
> > > > few interfaces is fine?
> > > >
> > > > The existing api.auth and api.security packages for example
> > > > only have
> > > > 3 items each.
> > > >
> > > > It has the benefit of OSGi package versioning being very
> > > > specific.
> > > >
> > > > -Bertrand
> > > >
Re: Redirect Provider
Posted by Ian Boston <ie...@tfd.co.uk>.
Hi,
The PRs have been opened, see the issue.
I will do complete implementations to verify it works as required in AEM.
I would also like to insure no disruption to Sling, and since it's been a
long time since I did this, how do run full CI/CD and integrate a new API
in Sling ? (a pointer would be great).
Best Regards
Ian
On Sat, 8 Feb 2020 at 09:23, Ian Boston <ie...@tfd.co.uk> wrote:
> Hi,
> Opened [1] to track the work.
> Best Regards
> Ian
>
> 1 https://issues.apache.org/jira/browse/SLING-9060
>
> On Wed, 5 Feb 2020 at 22:12, Ian Boston <ie...@tfd.co.uk> wrote:
>
>> Hi,
>> Ok.
>>
>> o.a.s.api.redirect for the API, and a patch to the Get servlet to use the
>> API.
>> Will do PRs shortly.
>>
>> Best Regards
>> Ian
>>
>>
>> On Tue, 4 Feb 2020 at 14:13, Bertrand Delacretaz <bd...@apache.org>
>> wrote:
>>
>>> On Tue, Feb 4, 2020 at 1:58 PM Ian Boston <ie...@tfd.co.uk> wrote:
>>> > ...New package, agreed, no ripples.
>>> > o.a.s.api.response ? ...
>>>
>>> o.a.s.api.redirect is more specific and I think having packages with
>>> few interfaces is fine?
>>>
>>> The existing api.auth and api.security packages for example only have
>>> 3 items each.
>>>
>>> It has the benefit of OSGi package versioning being very specific.
>>>
>>> -Bertrand
>>>
>>
Re: Redirect Provider
Posted by Ian Boston <ie...@tfd.co.uk>.
Hi,
Opened [1] to track the work.
Best Regards
Ian
1 https://issues.apache.org/jira/browse/SLING-9060
On Wed, 5 Feb 2020 at 22:12, Ian Boston <ie...@tfd.co.uk> wrote:
> Hi,
> Ok.
>
> o.a.s.api.redirect for the API, and a patch to the Get servlet to use the
> API.
> Will do PRs shortly.
>
> Best Regards
> Ian
>
>
> On Tue, 4 Feb 2020 at 14:13, Bertrand Delacretaz <bd...@apache.org>
> wrote:
>
>> On Tue, Feb 4, 2020 at 1:58 PM Ian Boston <ie...@tfd.co.uk> wrote:
>> > ...New package, agreed, no ripples.
>> > o.a.s.api.response ? ...
>>
>> o.a.s.api.redirect is more specific and I think having packages with
>> few interfaces is fine?
>>
>> The existing api.auth and api.security packages for example only have
>> 3 items each.
>>
>> It has the benefit of OSGi package versioning being very specific.
>>
>> -Bertrand
>>
>
Re: Redirect Provider
Posted by Ian Boston <ie...@tfd.co.uk>.
Hi,
Ok.
o.a.s.api.redirect for the API, and a patch to the Get servlet to use the
API.
Will do PRs shortly.
Best Regards
Ian
On Tue, 4 Feb 2020 at 14:13, Bertrand Delacretaz <bd...@apache.org>
wrote:
> On Tue, Feb 4, 2020 at 1:58 PM Ian Boston <ie...@tfd.co.uk> wrote:
> > ...New package, agreed, no ripples.
> > o.a.s.api.response ? ...
>
> o.a.s.api.redirect is more specific and I think having packages with
> few interfaces is fine?
>
> The existing api.auth and api.security packages for example only have
> 3 items each.
>
> It has the benefit of OSGi package versioning being very specific.
>
> -Bertrand
>
Re: Redirect Provider
Posted by Bertrand Delacretaz <bd...@apache.org>.
On Tue, Feb 4, 2020 at 1:58 PM Ian Boston <ie...@tfd.co.uk> wrote:
> ...New package, agreed, no ripples.
> o.a.s.api.response ? ...
o.a.s.api.redirect is more specific and I think having packages with
few interfaces is fine?
The existing api.auth and api.security packages for example only have
3 items each.
It has the benefit of OSGi package versioning being very specific.
-Bertrand
Re: Redirect Provider
Posted by Ian Boston <ie...@tfd.co.uk>.
Hi,
New package, agreed, no ripples.
o.a.s.api.response ?
request & resource already exist, but no response.
Best Regards
Ian
On Tue, 4 Feb 2020 at 10:14, Bertrand Delacretaz <bd...@apache.org>
wrote:
> On Tue, Feb 4, 2020 at 9:54 AM Radu Cotescu <ra...@apache.org> wrote:
> > ...After Carsten’s detailed reasoning I guess it makes the most sense to
> put this new
> > API in o.a.s.api, where the Resource and ResourceProvider APIs are also
> defined...
>
> But ideally in a new package? To avoid bumping up versions of existing
> packages, which often has unfortunate ripple effects.
>
> -Bertrand
>
Re: Redirect Provider
Posted by Bertrand Delacretaz <bd...@apache.org>.
On Tue, Feb 4, 2020 at 9:54 AM Radu Cotescu <ra...@apache.org> wrote:
> ...After Carsten’s detailed reasoning I guess it makes the most sense to put this new
> API in o.a.s.api, where the Resource and ResourceProvider APIs are also defined...
But ideally in a new package? To avoid bumping up versions of existing
packages, which often has unfortunate ripple effects.
-Bertrand
Re: Redirect Provider
Posted by Radu Cotescu <ra...@apache.org>.
Hi Ian,
> On 4 Feb 2020, at 09:24, Ian Boston <ie...@tfd.co.uk> wrote:
>
> Hi,
> Which bundle/package should the API be in ?
> Sling API ?
> Best Regards
> Ian
>
After Carsten’s detailed reasoning I guess it makes the most sense to put this new API in o.a.s.api, where the Resource and ResourceProvider APIs are also defined.
Regards,
Radu
Re: Redirect Provider
Posted by Ian Boston <ie...@tfd.co.uk>.
Hi,
Which bundle/package should the API be in ?
Sling API ?
Best Regards
Ian
On Fri, 31 Jan 2020 at 17:50, Carsten Ziegeler <cz...@apache.org> wrote:
> A general service does imho not sense. That service has no knowledge
> about where resources are coming from and how binaries a stored.
> A resource provider does know these things.
>
> This leaves us with two options: we allow multiple services to be
> registered. Each service than has to check if it knows about the
> resource implementation it gets and if it knows about it, it can handle
> it. This makes handling this case unnecessarily complex. It also brings
> us back to an ordering problem - what if that service is currently not
> registered, but the resource provider is etc.
>
> The other option is to let the resource provider implementation provide
> this object. The only interface we have for this, is adaptTo. But as
> pointed out, in this case it is super safe to call adaptTo as the
> resource (or the backing resource provider) can implement it.
>
> No need to ask N services to find out that none of them supports a
> resource. No fear of services being temporarily unavailable etc.
>
> AdaptTo makes both, implementation and usage much cleaner
>
> Carsten
>
> On 31.01.2020 04:15, Ian Boston wrote:
> > Hi,
> > I started with that service pattern, as you have suggested, but Carsten
> > asked we used a provider with the adaptTo pattern. He can explain. He
> does
> > that better. It made sense to me when he did.
> > Best Regards
> > Ian
> >
> > On Fri, 31 Jan 2020 at 12:02, Radu Cotescu <ra...@apache.org> wrote:
> >
> >> Hi Ian,
> >>
> >> I posted a comment to your gist [2] where I just changed the pattern to
> >> not use adaptTo. WDYT?
> >>
> >> Thanks,
> >> Radu
> >>
> >> [2] -
> >>
> https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621#gistcomment-3162233
> >> <
> >>
> https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621#gistcomment-3162233
> >>>
> >
>
> --
> --
> Carsten Ziegeler
> Adobe Research Switzerland
> cziegeler@apache.org
>
Re: Redirect Provider
Posted by Carsten Ziegeler <cz...@apache.org>.
A general service does imho not sense. That service has no knowledge
about where resources are coming from and how binaries a stored.
A resource provider does know these things.
This leaves us with two options: we allow multiple services to be
registered. Each service than has to check if it knows about the
resource implementation it gets and if it knows about it, it can handle
it. This makes handling this case unnecessarily complex. It also brings
us back to an ordering problem - what if that service is currently not
registered, but the resource provider is etc.
The other option is to let the resource provider implementation provide
this object. The only interface we have for this, is adaptTo. But as
pointed out, in this case it is super safe to call adaptTo as the
resource (or the backing resource provider) can implement it.
No need to ask N services to find out that none of them supports a
resource. No fear of services being temporarily unavailable etc.
AdaptTo makes both, implementation and usage much cleaner
Carsten
On 31.01.2020 04:15, Ian Boston wrote:
> Hi,
> I started with that service pattern, as you have suggested, but Carsten
> asked we used a provider with the adaptTo pattern. He can explain. He does
> that better. It made sense to me when he did.
> Best Regards
> Ian
>
> On Fri, 31 Jan 2020 at 12:02, Radu Cotescu <ra...@apache.org> wrote:
>
>> Hi Ian,
>>
>> I posted a comment to your gist [2] where I just changed the pattern to
>> not use adaptTo. WDYT?
>>
>> Thanks,
>> Radu
>>
>> [2] -
>> https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621#gistcomment-3162233
>> <
>> https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621#gistcomment-3162233
>>>
>
--
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: Redirect Provider
Posted by Ian Boston <ie...@tfd.co.uk>.
Hi,
I started with that service pattern, as you have suggested, but Carsten
asked we used a provider with the adaptTo pattern. He can explain. He does
that better. It made sense to me when he did.
Best Regards
Ian
On Fri, 31 Jan 2020 at 12:02, Radu Cotescu <ra...@apache.org> wrote:
> Hi Ian,
>
> I posted a comment to your gist [2] where I just changed the pattern to
> not use adaptTo. WDYT?
>
> Thanks,
> Radu
>
> [2] -
> https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621#gistcomment-3162233
> <
> https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621#gistcomment-3162233
> >
Re: Redirect Provider
Posted by Radu Cotescu <ra...@apache.org>.
Hi Ian,
I posted a comment to your gist [2] where I just changed the pattern to not use adaptTo. WDYT?
Thanks,
Radu
[2] - https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621#gistcomment-3162233 <https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621#gistcomment-3162233>
Re: Redirect Provider
Posted by Ian Boston <ie...@tfd.co.uk>.
Hi,
Good observations.
> Why not return RedirectResponse from
RedirectResolver#resolve(SlingHttpServletRequest)? This would obviate
the need for the magic NO_REDIRECT status code. Instead null could be
returned if no redirect should be performed.
Initially an interface was used, and hence passed in to avoid binding
implementations of the provider to the interface. Being passed in and not
returned means the provider will continue to work regardless of changes to
the RedirectResponse interface over time. Obviously methods will be
deprecated and ideally not removed for ages so providers dont have to be
patched.
That said, on reflection, I switched to using a non abstract, non final
class, so both consumer and provider are loosely bound to the class and
free to implement by extension. With a class, it could be returned and
implemented on either side, provided the sides agree. Then the question
comes down to, which side ? Should the consumer drive this or the
producer ? The consumer has to act on what is returned from the producer,
the producer suggesting what should be done. Hence, even with a class, I
think the consumer should extend the class and pass it as a param for the
producer propose how redirection should be.
> I am also wondering if it is sensible to encode the word "redirect"
into the class/interface names. Is a redirect really the only
use-case? Or would it be conceivable to use the same mechanism for
other responses that don't require a response body?
While the mechanism could be used for any non-body response, the intention
is only for redirection, hence I think RedirectResolver is better than
ResponseResolver or something wider.
Best Regards
Ian
On Fri, 31 Jan 2020 at 10:42, Julian Sedding <js...@gmail.com> wrote:
> Hi,
>
> Why not return RedirectResponse from
> RedirectResolver#resolve(SlingHttpServletRequest)? This would obviate
> the need for the magic NO_REDIRECT status code. Instead null could be
> returned if no redirect should be performed.
>
> I am also wondering if it is sensible to encode the word "redirect"
> into the class/interface names. Is a redirect really the only
> use-case? Or would it be conceivable to use the same mechanism for
> other responses that don't require a response body?
>
> Another consideration could be whether the same mechanism could
> (should?) be used to allow setting headers for responses that then
> continue through normal processing. (I know, the same can be achieved
> using a servlet filter.)
>
> No hard opinions, just soft thoughts ;)
>
> Regards
> Julian
>
> On Fri, Jan 31, 2020 at 11:00 AM Ian Boston <ie...@tfd.co.uk> wrote:
> >
> > Hi,
> > NO_REDIRECT, good point, added.
> >
> > I think HttpServletRequest is better as it is wider.
> > If an implementation of a RedirectResolver requires
> SlingHttpServletRequest
> > it can check for that and cast to it, returning NO_REDIRECT if the
> context
> > isn't what it required.
> >
> > Best Regards
> > Ian
> >
> >
> > On Fri, 31 Jan 2020 at 08:49, Bertrand Delacretaz <
> bdelacretaz@apache.org>
> > wrote:
> >
> > > Hi,
> > >
> > > On Fri, Jan 31, 2020 at 9:11 AM Ian Boston <ie...@tfd.co.uk> wrote:
> > > ..
> > > > 1 https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621
> > > ...
> > >
> > > The RedirectResolver gets an HttpServletRequest, shouldn't that be a
> > > SlingHttpServletRequest to be able to get at the Resource, selectors
> > > etc?
> > >
> > > I understand the Resource can come from having used adaptTo before,
> > > but other request attributes might influence the redirect and we have
> > > them already parsed in SlingHttpServletRequest.
> > >
> > > And a nitpick, in RedirectResponse I would make -1 a constant,
> NO_REDIRECT.
> > >
> > > -Bertrand
> > >
>
Re: Redirect Provider
Posted by Julian Sedding <js...@gmail.com>.
Hi,
Why not return RedirectResponse from
RedirectResolver#resolve(SlingHttpServletRequest)? This would obviate
the need for the magic NO_REDIRECT status code. Instead null could be
returned if no redirect should be performed.
I am also wondering if it is sensible to encode the word "redirect"
into the class/interface names. Is a redirect really the only
use-case? Or would it be conceivable to use the same mechanism for
other responses that don't require a response body?
Another consideration could be whether the same mechanism could
(should?) be used to allow setting headers for responses that then
continue through normal processing. (I know, the same can be achieved
using a servlet filter.)
No hard opinions, just soft thoughts ;)
Regards
Julian
On Fri, Jan 31, 2020 at 11:00 AM Ian Boston <ie...@tfd.co.uk> wrote:
>
> Hi,
> NO_REDIRECT, good point, added.
>
> I think HttpServletRequest is better as it is wider.
> If an implementation of a RedirectResolver requires SlingHttpServletRequest
> it can check for that and cast to it, returning NO_REDIRECT if the context
> isn't what it required.
>
> Best Regards
> Ian
>
>
> On Fri, 31 Jan 2020 at 08:49, Bertrand Delacretaz <bd...@apache.org>
> wrote:
>
> > Hi,
> >
> > On Fri, Jan 31, 2020 at 9:11 AM Ian Boston <ie...@tfd.co.uk> wrote:
> > ..
> > > 1 https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621
> > ...
> >
> > The RedirectResolver gets an HttpServletRequest, shouldn't that be a
> > SlingHttpServletRequest to be able to get at the Resource, selectors
> > etc?
> >
> > I understand the Resource can come from having used adaptTo before,
> > but other request attributes might influence the redirect and we have
> > them already parsed in SlingHttpServletRequest.
> >
> > And a nitpick, in RedirectResponse I would make -1 a constant, NO_REDIRECT.
> >
> > -Bertrand
> >
Re: Redirect Provider
Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi,
On Fri, Jan 31, 2020 at 11:00 AM Ian Boston <ie...@tfd.co.uk> wrote:
> ...If an implementation of a RedirectResolver requires SlingHttpServletRequest
> it can check for that and cast to it, returning NO_REDIRECT if the context
> isn't what it required...
Makes sense and that's probably going to be an infrequent case anyway, thanks!
-Bertrand
Re: Redirect Provider
Posted by Ian Boston <ie...@tfd.co.uk>.
Hi,
NO_REDIRECT, good point, added.
I think HttpServletRequest is better as it is wider.
If an implementation of a RedirectResolver requires SlingHttpServletRequest
it can check for that and cast to it, returning NO_REDIRECT if the context
isn't what it required.
Best Regards
Ian
On Fri, 31 Jan 2020 at 08:49, Bertrand Delacretaz <bd...@apache.org>
wrote:
> Hi,
>
> On Fri, Jan 31, 2020 at 9:11 AM Ian Boston <ie...@tfd.co.uk> wrote:
> ..
> > 1 https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621
> ...
>
> The RedirectResolver gets an HttpServletRequest, shouldn't that be a
> SlingHttpServletRequest to be able to get at the Resource, selectors
> etc?
>
> I understand the Resource can come from having used adaptTo before,
> but other request attributes might influence the redirect and we have
> them already parsed in SlingHttpServletRequest.
>
> And a nitpick, in RedirectResponse I would make -1 a constant, NO_REDIRECT.
>
> -Bertrand
>
Re: Redirect Provider
Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi,
On Fri, Jan 31, 2020 at 9:11 AM Ian Boston <ie...@tfd.co.uk> wrote:
..
> 1 https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621
...
The RedirectResolver gets an HttpServletRequest, shouldn't that be a
SlingHttpServletRequest to be able to get at the Resource, selectors
etc?
I understand the Resource can come from having used adaptTo before,
but other request attributes might influence the redirect and we have
them already parsed in SlingHttpServletRequest.
And a nitpick, in RedirectResponse I would make -1 a constant, NO_REDIRECT.
-Bertrand
Re: Redirect Provider
Posted by Ian Boston <ie...@tfd.co.uk>.
Hi,
Agreed. We should have the deprecation conversation in a separate thread.
No rush.
Here is a gist[1] of what is being proposed.
It has had some input already from Carsten and Robert.
On reflection we changed the signature of the provider to pass in an
implementation of a class (RedirectResponse).
I also changed that from an interface to a class to be extended by
implementations, so that when new methods are added to the RedirectResponse
in the future, no past implementation has to change.
If it was an interface, then all past implementations on the caller side
would have to change to avoid class cast issues.
Best Regards
Ian
1 https://gist.github.com/ieb/5f217e2c160afb7bb4098bca99896621
On Thu, 30 Jan 2020 at 14:17, Carsten Ziegeler <cz...@apache.org> wrote:
> We need a way for a resource provider to provide this object based on a
> resource. And adaptTo is exactly the mechanism for this. As this
> adaption is implemented by a provider, there is no startup ordering
> problem etc as some experience with more custom adaptions.
> So in this case, on the same resource a call to adaptTo will most likely
> always give the same result, either null or the object.
> Sounds perfectly valid to me.
>
> Carsten
>
> On 30.01.2020 00:52, Radu Cotescu wrote:
> > Could we, at least for new APIs, stop using adaptTo? (I started running
> away from any physical location known by Sling committers)
> >
> >> On 29 Jan 2020, at 21:40, Carsten Ziegeler <cz...@apache.org>
> wrote:
> >>
> >> A resource might be adaptable to RedirectProvider.
> >
> >
>
> --
> --
> Carsten Ziegeler
> Adobe Research Switzerland
> cziegeler@apache.org
>
Re: Redirect Provider
Posted by Carsten Ziegeler <cz...@apache.org>.
We need a way for a resource provider to provide this object based on a
resource. And adaptTo is exactly the mechanism for this. As this
adaption is implemented by a provider, there is no startup ordering
problem etc as some experience with more custom adaptions.
So in this case, on the same resource a call to adaptTo will most likely
always give the same result, either null or the object.
Sounds perfectly valid to me.
Carsten
On 30.01.2020 00:52, Radu Cotescu wrote:
> Could we, at least for new APIs, stop using adaptTo? (I started running away from any physical location known by Sling committers)
>
>> On 29 Jan 2020, at 21:40, Carsten Ziegeler <cz...@apache.org> wrote:
>>
>> A resource might be adaptable to RedirectProvider.
>
>
--
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: Redirect Provider
Posted by Radu Cotescu <ra...@apache.org>.
Could we, at least for new APIs, stop using adaptTo? (I started running away from any physical location known by Sling committers)
> On 29 Jan 2020, at 21:40, Carsten Ziegeler <cz...@apache.org> wrote:
>
> A resource might be adaptable to RedirectProvider.
Re: Redirect Provider
Posted by Carsten Ziegeler <cz...@apache.org>.
I think we should create two interfaces (names are not final):
RedirectProvider and RedirectResponse.
A resource might be adaptable to RedirectProvider. If it is the stream
get servlet uses it to do the redirect.
RedirectResponse is a stripped down version of the HttpServletResponse
just allowing to setHeaders, send a redirect or send an error.
RedirectProvider gets a single method
void redirect(HttpServletRequest request, RedirectResponse response);
This way we can evolve RedirectResponse if really required.
Both need to go into a new package.
Regards
Carsten
On 29.01.2020 10:46, Ian Boston wrote:
> Hi,
> I have been talking with Carsten and Robert offline about a better solution
> to replace the URIProvider which to be frank, isn't really working for us.
> For those that don't know, its used inside the Slng Default Get servlet to
> allow that servlet to return a redirect instead of sending a stream of
> bytes. Typically the redirect would be a to a service better suited to
> streaming the bytes of a binary.
>
> The problem with the URIProvider is the API only sees a Resource so can
> determine the context of the request, the FQDN or any other information
> about the request.
>
> Initially I suggested adding a method to the URIProvider, but Carsten said
> that would break implementations.
>
> We discussed deprecating URIProvider so that anyone using it can still use
> it, but adding a new provider (ie new interface) that can given a request
> from which a provider implementation would return a status code, and
> headers. If the status code is not -1 then the headers would added to the
> response and the response returned.
>
> Initially I suggested both request and response objects were give to the
> provider. Carsten preferred not to give the response object as this could
> allow he provider implementation to do too much.
>
> ie
> interface RedirectProvider {
>
> /**
> * @param request The request
> * @param responseHeaders a list of headers to be filled by the provider
> * and added to the response if the return is a http status
> code.
> * @returns -1 if no action to be taken, or a http status code if a
> response is to
> * created with that status code.
> */
> int getRedirect(HttpServletRequest request, List<String[]> responseHeaders);
>
> }
>
>
> wdyt? Is this something Sling can consider. If so I would volunteer to do a
> PR, but I would not want to invest the effort if there is no appetite.
>
> Best Regards
> Ian
>
--
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org