Using adapters on Sling startup

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Using adapters on Sling startup

Robert Munteanu-2
Hi,

I recently became aware of the following scenario ( and many thanks to
David Bosschaert for pointing out why it's failing ).

- Bundle A exports package com.foo and registers an adapter factory for
com.foo.Service
- Bundle B imports com.foo from Bundle A and calls
slingRequest.adaptTo(com.foo.Service.class)

When Sling starts up, it's perfectly legal for Bundle B to be STARTED
while Bundle A is just RESOLVED. This means that while com.foo.Service
does not have an AdapterFactory registered, bundle B will call
slingRequest.adaptTo(com.foo.Service.class), which will unexpectedly
return null.

The only 'solution' I see to this is documenting this scenario so that
it's less suprising to others.

I briefly considered using an OSGi manifest header to declare provided
and required adapter factories, but unfortunately you can't always know
what the klazz object will be at runtime when writing

    slingRequest.adaptTo(klazz)

Thoughts? Other ideas?

Thanks,

Robert

Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

justzzzz
Hi,
I thought I sent this earlier, but it appears not to have gotten
through (at least not according to markmail). Weird...

------

Hi,
One ongoing problem (which has been a bit exacerbated by Sling Models)
is when code depends upon the AdapterManager being able to handle a
particular adaptation. Right now, the only way to deal with this is by
listening for the AdapterManager events. But these events aren't
particularly easy to deal with; it would be easier if this was based
on the service registry.

What I would like to see is that the AdapterManager registers some
kind of marker service when an adaptation is available.

From the consuming side, it would look like this:

@Component
@Service
@Reference(referenceInterface=Adaptation.class,
target="(&(adaptable=com.myco.MyClass)(adaptable=org.apache.sling.api.Resource))")
public class Foo {

    public void doSomething(Resource resource) {
MyClass cfgMan =
resource.adaptTo(MyClass.class)
    }

}

WDYT?

Regards,
Justin

On Wed, Dec 3, 2014 at 3:11 PM, Robert Munteanu <[hidden email]> wrote:

> Hi,
>
> I recently became aware of the following scenario ( and many thanks to
> David Bosschaert for pointing out why it's failing ).
>
> - Bundle A exports package com.foo and registers an adapter factory for
> com.foo.Service
> - Bundle B imports com.foo from Bundle A and calls
> slingRequest.adaptTo(com.foo.Service.class)
>
> When Sling starts up, it's perfectly legal for Bundle B to be STARTED
> while Bundle A is just RESOLVED. This means that while com.foo.Service
> does not have an AdapterFactory registered, bundle B will call
> slingRequest.adaptTo(com.foo.Service.class), which will unexpectedly
> return null.
>
> The only 'solution' I see to this is documenting this scenario so that
> it's less suprising to others.
>
> I briefly considered using an OSGi manifest header to declare provided
> and required adapter factories, but unfortunately you can't always know
> what the klazz object will be at runtime when writing
>
>     slingRequest.adaptTo(klazz)
>
> Thoughts? Other ideas?
>
> Thanks,
>
> Robert
>
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Robert Munteanu-3
Hi Justin,

On Wed, Dec 3, 2014 at 10:19 PM, Justin Edelson
<[hidden email]> wrote:
> Hi,
> I thought I sent this earlier, but it appears not to have gotten
> through (at least not according to markmail). Weird...

It came through as a separate email with a new subject. Anyway ...

>
> ------
>
> Hi,
> One ongoing problem (which has been a bit exacerbated by Sling Models)
> is when code depends upon the AdapterManager being able to handle a
> particular adaptation. Right now, the only way to deal with this is by
> listening for the AdapterManager events. But these events aren't
> particularly easy to deal with; it would be easier if this was based
> on the service registry.
>
> What I would like to see is that the AdapterManager registers some
> kind of marker service when an adaptation is available.
>
> From the consuming side, it would look like this:
>
> @Component
> @Service
> @Reference(referenceInterface=Adaptation.class,
> target="(&(adaptable=com.myco.MyClass)(adaptable=org.apache.sling.api.Resource))")
> public class Foo {
>
>     public void doSomething(Resource resource) {
> MyClass cfgMan =
> resource.adaptTo(MyClass.class)
>     }
>
> }

That sounds like a workable way forward. The obvious advantage
compared to the current situation is that we can reliably wait for an
adaption to be available. The downsides IMO are:

- you have to know about this mechanism to use it
- it's a very slight abuse of the service registry, we don't actually
need or use that service anywhere

But for me the trade-offs in making the adaptable mechanism more
stable are worth it. I think you also mentioned in a separate
discussion that we can have a SCR annotation that generates this
reference, which is ideal, something like

    @RequiresAdapter(adaptable=Resource.class, adapter=MyClass.class)

( BTW, I think your example @Reference should have adapter=...MyClass,
not adaptable=...MyClass )

Cheers,

Robert

>
> WDYT?
>
> Regards,
> Justin
>
> On Wed, Dec 3, 2014 at 3:11 PM, Robert Munteanu <[hidden email]> wrote:
>> Hi,
>>
>> I recently became aware of the following scenario ( and many thanks to
>> David Bosschaert for pointing out why it's failing ).
>>
>> - Bundle A exports package com.foo and registers an adapter factory for
>> com.foo.Service
>> - Bundle B imports com.foo from Bundle A and calls
>> slingRequest.adaptTo(com.foo.Service.class)
>>
>> When Sling starts up, it's perfectly legal for Bundle B to be STARTED
>> while Bundle A is just RESOLVED. This means that while com.foo.Service
>> does not have an AdapterFactory registered, bundle B will call
>> slingRequest.adaptTo(com.foo.Service.class), which will unexpectedly
>> return null.
>>
>> The only 'solution' I see to this is documenting this scenario so that
>> it's less suprising to others.
>>
>> I briefly considered using an OSGi manifest header to declare provided
>> and required adapter factories, but unfortunately you can't always know
>> what the klazz object will be at runtime when writing
>>
>>     slingRequest.adaptTo(klazz)
>>
>> Thoughts? Other ideas?
>>
>> Thanks,
>>
>> Robert
>>



--
Sent from my (old) computer
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Bertrand Delacretaz
In reply to this post by Robert Munteanu-2
On Wed, Dec 3, 2014 at 9:11 PM, Robert Munteanu <[hidden email]> wrote:
> ...When Sling starts up, it's perfectly legal for Bundle B to be STARTED
> while Bundle A is just RESOLVED....

Would the OSGi Require-Capability help make sure that the required
adapters are available before starting bundle B?

-Bertrand
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Felix Meschberger-3
Hi

> Am 04.12.2014 um 15:00 schrieb Bertrand Delacretaz <[hidden email]>:
>
> On Wed, Dec 3, 2014 at 9:11 PM, Robert Munteanu <[hidden email]> wrote:
>> ...When Sling starts up, it's perfectly legal for Bundle B to be STARTED
>> while Bundle A is just RESOLVED....
>
> Would the OSGi Require-Capability help make sure that the required
> adapters are available before starting bundle B?

No, because Require-Capability is resolution time not runtime

Regards
Felix
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Bertrand Delacretaz
In reply to this post by Robert Munteanu-3
Hi,

On Thursday, December 4, 2014, Robert Munteanu <[hidden email]> wrote:
> ...
> @RequiresAdapter(adaptable=Resource.class, adapter=MyClass.class)
> ...

This looks good to me, although it would be even more obvious as

  @RequiresAdapter(from=Resource.class, to=MyClass.class)

The adaptable/adapter terminology has never been intuitive to me, for some
reason.

-Bertrand
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Robert Munteanu-3
On Thu, Dec 4, 2014 at 5:25 PM, Bertrand Delacretaz
<[hidden email]> wrote:

> Hi,
>
> On Thursday, December 4, 2014, Robert Munteanu <[hidden email]> wrote:
>> ...
>> @RequiresAdapter(adaptable=Resource.class, adapter=MyClass.class)
>> ...
>
> This looks good to me, although it would be even more obvious as
>
>   @RequiresAdapter(from=Resource.class, to=MyClass.class)
>
> The adaptable/adapter terminology has never been intuitive to me, for some
> reason.

+1

I wrote the exact same form initially, but reverted to the official
terminology to be consistent. But if others feel that from/to are
easier to digest, let's do that.

Robert

--
Sent from my (old) computer
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Robert Munteanu-2
In reply to this post by justzzzz
On Wed, 2014-12-03 at 15:19 -0500, Justin Edelson wrote:

> @Component
> @Service
> @Reference(referenceInterface=Adaptation.class,
> target="(&(adaptable=com.myco.MyClass)(adaptable=org.apache.sling.api.Resource))")
> public class Foo {
>
>     public void doSomething(Resource resource) {
> MyClass cfgMan =
> resource.adaptTo(MyClass.class)
>     }
>
> }

Actually the complete @Reference must be a bit more verbose, at least to
make the scr plugin happy:

@Reference(referenceInterface=Adaptation.class,target="(&(adaptable=com.myco.MyClass)(adapter=org.apache.sling.api.Resource))",name = "ignore", strategy = ReferenceStrategy.LOOKUP)

I took a quick stab at a patch ( org.apache.sling.adapter only, no SCR
annotations for now), and the changes are not major and contained to the
AdapterImpl, but testing ( both JUnit and manual ) is going to be a
drag.

Therefore before committing more time to this I'd like to know if there
are any concerns with the proposed solution or any ideas on how to make
it better.

Cheers,

Robert

Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Robert Munteanu-2
> I took a quick stab at a patch ( org.apache.sling.adapter only, no SCR
> annotations for now),

https://issues.apache.org/jira/browse/SLING-4217

Patch attached, a review would be appreciated. It works for my
specific scenario, but might not be 100% correct.

Thanks,

Robert
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Robert Munteanu-2
Any comments?

The final version of the diff is at [1] and I think it's correct. I'd
like to commit this today or tomorrow so any feedback is welcome.

Thanks,

Robert

[1]: https://reviews.apache.org/r/28751/

On Fri, Dec 5, 2014 at 12:01 AM, Robert Munteanu <[hidden email]> wrote:

>> I took a quick stab at a patch ( org.apache.sling.adapter only, no SCR
>> annotations for now),
>
> https://issues.apache.org/jira/browse/SLING-4217
>
> Patch attached, a review would be appreciated. It works for my
> specific scenario, but might not be 100% correct.
>
> Thanks,
>
> Robert
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Bertrand Delacretaz
Hi Robert,

On Mon, Dec 8, 2014 at 10:45 AM, Robert Munteanu <[hidden email]> 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.

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?

-Bertrand
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Felix Meschberger-3
In reply to this post by Bertrand Delacretaz
Hi

> Am 04.12.2014 um 16:25 schrieb Bertrand Delacretaz <[hidden email]>:
>
> Hi,
>
> On Thursday, December 4, 2014, Robert Munteanu <[hidden email]> wrote:
>> ...
>> @RequiresAdapter(adaptable=Resource.class, adapter=MyClass.class)
>> ...
>
> This looks good to me, although it would be even more obvious as
>
>  @RequiresAdapter(from=Resource.class, to=MyClass.class)
>
> The adaptable/adapter terminology has never been intuitive to me, for some
> reason.

It is not 100% intuitive, right, unfortunately. But I would refrain from adding not terminology to some existing stuff. We have adaptable and adapter and we should stick.

BTW: I may have missed sending message to the list. I have played with this idea an created https://reviews.apache.org/r/28758/ (I call it AdapterReference, though :-) )


Regards
Felix
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Robert Munteanu-3
In reply to this post by Bertrand Delacretaz
Hi Bertrand,

On Mon, Dec 8, 2014 at 12:37 PM, Bertrand Delacretaz
<[hidden email]> wrote:
> Hi Robert,
>
> On Mon, Dec 8, 2014 at 10:45 AM, Robert Munteanu <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Alexander Klimetschek-2
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 <[hidden email]> wrote:
>
> Hi Bertrand,
>
> On Mon, Dec 8, 2014 at 12:37 PM, Bertrand Delacretaz
> <[hidden email]> wrote:
>> Hi Robert,
>>
>> On Mon, Dec 8, 2014 at 10:45 AM, Robert Munteanu <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Bertrand Delacretaz
Hi Alex,

On Fri, Feb 6, 2015 at 1:45 AM, Alexander Klimetschek
<[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Felix Meschberger-3
In reply to this post by Alexander Klimetschek-2
Hi

> Am 06.02.2015 um 01:45 schrieb Alexander Klimetschek <[hidden email]>:
>
> 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 <[hidden email]> wrote:
>>
>> Hi Bertrand,
>>
>> On Mon, Dec 8, 2014 at 12:37 PM, Bertrand Delacretaz
>> <[hidden email]> wrote:
>>> Hi Robert,
>>>
>>> On Mon, Dec 8, 2014 at 10:45 AM, Robert Munteanu <[hidden email]> 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
>

Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Alexander Klimetschek-2
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
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Alexander Klimetschek-2
On 12.02.2015, at 15:15, Alexander Klimetschek <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Robert Munteanu-2
In reply to this post by Alexander Klimetschek-2
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

Reply | Threaded
Open this post in threaded view
|

Re: Using adapters on Sling startup

Alexander Klimetschek-2
On 13.02.2015, at 01:55, Robert Munteanu <[hidden email]> 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