You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Alexander Klimetschek <ak...@adobe.com> on 2015/02/06 01:45:49 UTC
Re: Using adapters on Sling startup
Let me recap:
0. adaptTo was invented for jsp scripts
1. we are now using adaptTo() everywhere because of convenience and we don't want to depend on a service
2. we figure out it doesn't work in some startup cases
3. we add a new mock adapter service and depend on that in our code
Isn't it simpler to use a proper service in the first place then?
And fix those cases that are only available via AdapterManager-adaptTo() to also have a proper service equivalent?
Note that you can have this case within one bundle already, i.e. if you have code in service A that depends on adaptTo() of an AdapterManager B in the same bundle, with A being started before B. The rule from that for me was to never use adaptTo() for the stuff from within a bundle - which was just lazy and can be done with better OO design anyway.
Cheers,
Alex
> On 08.12.2014, at 03:00, Robert Munteanu <ro...@lmn.ro> wrote:
>
> Hi Bertrand,
>
> On Mon, Dec 8, 2014 at 12:37 PM, Bertrand Delacretaz
> <bd...@apache.org> wrote:
>> Hi Robert,
>>
>> On Mon, Dec 8, 2014 at 10:45 AM, Robert Munteanu <ro...@apache.org> wrote:
>>> ...I'd
>>> like to commit this today or tomorrow so any feedback is welcome....
>>
>> Looks good to me except the SLING-4217 patch is missing the
>> Adaption*.java classes.
>
> I continued the latest patches at [1], makes it easier to see what
> changes, including between iterations of the same patch. This includes
> the missing classes.
>
>>
>> Also, about the "TODO - is it possible to remove more than one factory
>> per service reference" comment - I'm not sure what the impact of this
>> is, is it just about being able to do something in an easier way, or
>> is there a risk of leaving unwanted things registered?
>
> There's a risk of leaving an unwanted 'Adaption' service registered,
> which would incorrectly signal that the AdapterFactory is still
> registered. Nonetheless, the AdapterManager would still function
> correctly, the Adaption service is just used for signalling consumers
> of adaptTo when a certain adaption is available.
>
> Note that in the latest iteration I simply log an ERROR in such a
> scenario to simplify the code ( see [2] ).
>
> [1]: https://reviews.apache.org/r/28751/diff/#
> [2]: https://reviews.apache.org/r/28751/diff/3/#chunk3.22
>
>
>
> --
> Sent from my (old) computer
Re: Using adapters on Sling startup
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 13.02.2015, at 01:55, Robert Munteanu <ro...@apache.org> wrote:
> Coming back to this , I think that there's a scenario where actually
> adaptTo makes a lot of sense outside of scripts - components which must
> be backed by the caller's session / resource resolver.
>
> When looking up an OSGi service you can't pass in your own resource
> resolver. So if you want to get a component backed by your own resource
> resolver, you either use resourceResolver.adaptTo(...) or add new API
> which takes a resource resolver as an argument.
Such a service is exactly what I am talking about - and I don't think it's a problem. In many cases you end up having more optional arguments to get this, say some
ThatService.getObject(ResourceResolver resolver, String option, ...)
method (and the adaptTo is only kept for the simple/default way). Here you need the service anyway.
> Sometimes you don't want
> to add new API just for getting a service.
You are adding a new API anyway for the object you adaptTo.
Cheers,
Alex
Re: Using adapters on Sling startup
Posted by Robert Munteanu <ro...@apache.org>.
On Fri, 2015-02-06 at 00:45 +0000, Alexander Klimetschek wrote:
> Let me recap:
> 0. adaptTo was invented for jsp scripts
> 1. we are now using adaptTo() everywhere because of convenience and we
> don't want to depend on a service
> 2. we figure out it doesn't work in some startup cases
> 3. we add a new mock adapter service and depend on that in our code
>
> Isn't it simpler to use a proper service in the first place then?
Coming back to this , I think that there's a scenario where actually
adaptTo makes a lot of sense outside of scripts - components which must
be backed by the caller's session / resource resolver.
When looking up an OSGi service you can't pass in your own resource
resolver. So if you want to get a component backed by your own resource
resolver, you either use resourceResolver.adaptTo(...) or add new API
which takes a resource resolver as an argument. Sometimes you don't want
to add new API just for getting a service.
Cheers,
Robert
Re: Using adapters on Sling startup
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 12.02.2015, at 15:15, Alexander Klimetschek <ak...@adobe.com> wrote:
> An adaptTo-only example is the sling ResourceCollectionManager that you can only retrieve via resourceResolver.adaptTo(), but there is no corresponding service. Should I create an issue?
Correcting: it is a service itself. You can _also_ get it via adaptTo, so all good.
Cheers,
Alex
Re: Using adapters on Sling startup
Posted by Alexander Klimetschek <ak...@adobe.com>.
I was just reminded of another problem: adaptTo (especially within service code) makes it hard to write unit tests. It's hard to mock.
An adaptTo-only example is the sling ResourceCollectionManager that you can only retrieve via resourceResolver.adaptTo(), but there is no corresponding service. Should I create an issue?
Cheers,
Alex
Re: Using adapters on Sling startup
Posted by Felix Meschberger <fm...@adobe.com>.
Hi
> Am 06.02.2015 um 01:45 schrieb Alexander Klimetschek <ak...@adobe.com>:
>
> Let me recap:
> 0. adaptTo was invented for jsp scripts
Not only, but also :-)
It was essentially adapted from Eclipse to get a different view of the same object. E.g. get the Node „view“ on a node-based Resource.
As such the intent was always for this functionality to be available once the system is up and running. This goes from request processing scripts or servlets to services required at run time — even background processing.
> 1. we are now using adaptTo() everywhere because of convenience and we don't want to depend on a service
That’s not necessarily bad. The problem is during startup and shutdown where the adaptTo dependencies cannot be validated by the system.
> 2. we figure out it doesn't work in some startup cases
You are safe to say, its not reliable during startup and shutdown of the system, right.
> 3. we add a new mock adapter service and depend on that in our code
Which I never liked in the first place :-)
>
> Isn't it simpler to use a proper service in the first place then?
Exactly, that was my proposal as well: adaptTo is a really good convenience. For a number of use cases it is just better to have a provider service available.
>
> And fix those cases that are only available via AdapterManager-adaptTo() to also have a proper service equivalent?
+1
>
> Note that you can have this case within one bundle already, i.e. if you have code in service A that depends on adaptTo() of an AdapterManager B in the same bundle, with A being started before B. The rule from that for me was to never use adaptTo() for the stuff from within a bundle - which was just lazy and can be done with better OO design anyway.
+1
Regards
Felix
>
> Cheers,
> Alex
>
>> On 08.12.2014, at 03:00, Robert Munteanu <ro...@lmn.ro> wrote:
>>
>> Hi Bertrand,
>>
>> On Mon, Dec 8, 2014 at 12:37 PM, Bertrand Delacretaz
>> <bd...@apache.org> wrote:
>>> Hi Robert,
>>>
>>> On Mon, Dec 8, 2014 at 10:45 AM, Robert Munteanu <ro...@apache.org> wrote:
>>>> ...I'd
>>>> like to commit this today or tomorrow so any feedback is welcome....
>>>
>>> Looks good to me except the SLING-4217 patch is missing the
>>> Adaption*.java classes.
>>
>> I continued the latest patches at [1], makes it easier to see what
>> changes, including between iterations of the same patch. This includes
>> the missing classes.
>>
>>>
>>> Also, about the "TODO - is it possible to remove more than one factory
>>> per service reference" comment - I'm not sure what the impact of this
>>> is, is it just about being able to do something in an easier way, or
>>> is there a risk of leaving unwanted things registered?
>>
>> There's a risk of leaving an unwanted 'Adaption' service registered,
>> which would incorrectly signal that the AdapterFactory is still
>> registered. Nonetheless, the AdapterManager would still function
>> correctly, the Adaption service is just used for signalling consumers
>> of adaptTo when a certain adaption is available.
>>
>> Note that in the latest iteration I simply log an ERROR in such a
>> scenario to simplify the code ( see [2] ).
>>
>> [1]: https://reviews.apache.org/r/28751/diff/#
>> [2]: https://reviews.apache.org/r/28751/diff/3/#chunk3.22
>>
>>
>>
>> --
>> Sent from my (old) computer
>
Re: Using adapters on Sling startup
Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi Alex,
On Fri, Feb 6, 2015 at 1:45 AM, Alexander Klimetschek
<ak...@adobe.com> wrote:
> ...0. adaptTo was invented for jsp scripts...
Not only.
Adapting a Resource to a Node in java code makes perfect sense if you
need to access the JCR level.
Adapting a Resource to a Tenant also makes sense as a way to find out
which Tenant it belongs to.
> 1. we are now using adaptTo() everywhere because of convenience and we don't want to depend on a service...
There's definitely some abuse of adaptTo around...a good rule might be
to use adaptTo only when processing an HTTP request. A Sling app is
not supposed to process HTTP requests before it's fully ready, so the
"adapter not found yet" issue shouldn't happen.
-Bertrand