You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Felix Meschberger <fm...@adobe.com> on 2014/07/01 09:07:33 UTC
adaptTo and results ....
Hi
There currently are two issues floating around dealing with the question of returning more information than just null from the Adaptable.adaptTo(Class) method: https://issues.apache.org/jira/browse/SLING-3714 and https://issues.apache.org/jira/browse/SLING-3709. I think these requests warrant some discussion on the list.
Background: adaptTo can be implemented by Adaptable implementations themselves or, by extending from SlingAdaptable, they may defer to an AdapterMananager. AdapterManager itself is extended by AdapterFactory services. All these interfaces define an adaptTo method. All these methods return null if adaption is not possible and don't declare or document to throw an exception.
While not explicitly documented as such, the intention is and was that adaptTo never throws on the grounds that adaption may fail which is considered a valid result and thus exceptions are not to be expected and handled.
Hence all implementations of the methods generally catch-and-log-but-don't-throw. Interestingly SlingAdaptable.adaptTo and AdapterManagerImpl.getAdapter don't catch — so any RuntimeException thrown from an AdapterFactory would be forwarded.
Having said this there are options available:
(1) Add support for a new Result<?> class. We would probably implement this in the AdapterManager.getAdapter implementation explicitly handling this case because it would entail catching the adaptTo/getAdapter calls to get the exception (the Result.getError should return Throwable probably not Error)
Use would be limited to new AdapterFactory implementations throwing RuntimeExcpetion. For Sling Models this would be the case.
(2) Add a new adaptToOrThrow method, which is declared to throw a RuntimeException and never return null: Either it can adapt or it throws. This would require a new interface Adaptable2 (probably) to not break existing Adaptable implementations. The SlingAdaptable base class would implement the new method of course, probably something like this:
> SlingAdaptable implements Adaptable2 {
> …
> public <AdapterType> AdapterType adaptToOrThrow(Class<AdapterType> type) {
> AdapterType result = this.adaptTo(type);
> if (result != null) {
> return result;
> }
> throw new CannotAdaptException(…);
> }
> }
>
Use is problematic because you would have to know whether you can call the new method: So instead of an null check you now have an instanceof check … Except for the Resource interface which would be extended to extend from Adaptable2 as well.
(3) Document, that Adaptable.adaptTo may throw a RuntimeException.
The problem here is, that this may conceptually break existing callers of Adaptable.adaptTo which don't expect an exception at all — presumably this is a minor nuisance because technically a RuntimeException may always be thrown.
Regards
Felix
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
That would be solved by just stating that RuntimeExceptions are allowed as alternative to returning null for all AdapterFactories (i.e. no API change necessary) and making sure that those exceptions are either being caught within the AdapterManagerImpl or just propagated to the caller.
On 01 Jul 2014, at 13:08, Carsten Ziegeler <cz...@apache.org> wrote:
> Ok, this would solve the throw if adaption is not possible case, what about
> the validation use case?
>
> Carsten
>
>
> 2014-07-01 12:50 GMT+02:00 Bertrand Delacretaz <bd...@apache.org>:
>
>> On Tue, Jul 1, 2014 at 12:38 PM, Stefan Egli <st...@apache.org>
>> wrote:
>>> I like the idea too, but I guess it's merely a question of taste as to
>>> which of the following two options is nicer:
>>> * Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
>>> * Foo f = someObject.adaptToUnchecked(Foo.class);
>>
>> The big difference is that the first variant requires no API changes
>> and only requires code changes in AdapterManagerImpl (I think -
>> haven't looked in full detail ;-)
>>
>> -Bertrand
>>
>
>
>
> --
> Carsten Ziegeler
> Adobe Research Switzerland
> cziegeler@apache.org
Re: adaptTo and results ....
Posted by Carsten Ziegeler <cz...@apache.org>.
Ok, this would solve the throw if adaption is not possible case, what about
the validation use case?
Carsten
2014-07-01 12:50 GMT+02:00 Bertrand Delacretaz <bd...@apache.org>:
> On Tue, Jul 1, 2014 at 12:38 PM, Stefan Egli <st...@apache.org>
> wrote:
> > I like the idea too, but I guess it's merely a question of taste as to
> > which of the following two options is nicer:
> > * Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
> > * Foo f = someObject.adaptToUnchecked(Foo.class);
>
> The big difference is that the first variant requires no API changes
> and only requires code changes in AdapterManagerImpl (I think -
> haven't looked in full detail ;-)
>
> -Bertrand
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: adaptTo and results ....
Posted by Bertrand Delacretaz <bd...@apache.org>.
On Tue, Jul 1, 2014 at 12:38 PM, Stefan Egli <st...@apache.org> wrote:
> I like the idea too, but I guess it's merely a question of taste as to
> which of the following two options is nicer:
> * Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
> * Foo f = someObject.adaptToUnchecked(Foo.class);
The big difference is that the first variant requires no API changes
and only requires code changes in AdapterManagerImpl (I think -
haven't looked in full detail ;-)
-Bertrand
Re: adaptTo and results ....
Posted by Stefan Egli <st...@apache.org>.
I like the idea too, but I guess it's merely a question of taste as to
which of the following two options is nicer:
* Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
* Foo f = someObject.adaptToUnchecked(Foo.class);
Cheers,
Stefan
On 7/1/14 11:57 AM, "Konrad Windszus" <ko...@gmx.de> wrote:
>I like that approach. It is backwards-compatible and allows the
>developers to decide whether they want to check for null or to rely on
>exceptions.
>The AdapterManagerImpl indeed would need to deal with such a
>parametrisation and in addition the javadocs would need to be adjusted to
>make it clear that AdapterFactories may throw RuntimeExceptions.
>Those exceptions should be caught by the AdapterManagerImpl when the
>RequireAdapter was not requested and in the other case just passed along.
>
>
>On 01 Jul 2014, at 09:44, Bertrand Delacretaz <bd...@apache.org>
>wrote:
>
>> On Tue, Jul 1, 2014 at 9:41 AM, Bertrand Delacretaz
>> <bd...@apache.org> wrote:
>>> ...how about this:
>>>
>>> Foo f = someObject.adaptTo(RequireAdapter.for(Foo.class));
>>
>> Actually, rereading SLING-3714, this can be made simpler with generics
>>
>> Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
>>
>> where RequireAdapter causes AdapterManagerImpl to wrap the adapters to
>> throw an exception if adaption returns null.
>>
>> -Bertrand
>
Re: adaptTo and results ....
Posted by Felix Meschberger <fm...@adobe.com>.
Hi
Am 03.07.2014 um 12:29 schrieb Konrad Windszus <ko...@gmx.de>:
> The AdapterFactory should clearly state, which RuntimeExceptions are thrown under which condition. You know in most of the cases, which AdapterFactory is responsible for your adaptTo-method (or you should be able to find out with the web console)
I see. How about thus adding an annotation for the AdapterFactory to declare the exceptions thrown so the web console could expose this information as well — in the same way as there is the annotations to declare the adapters and adaptables.
> Handling in some cases is more than simple logging. The AEM6 WCMDeveloperModeFilter is a good example for another error treatment (catching the error within a servlet filter and then exposing via the Web UI).
Good point
Regards
Felix
>
> On 03 Jul 2014, at 12:19, Felix Meschberger <fm...@adobe.com> wrote:
>
>> Hi
>>
>> Am 03.07.2014 um 11:10 schrieb Konrad Windszus <ko...@gmx.de>:
>>
>>>
>>> On 03 Jul 2014, at 10:50, Alexander Klimetschek <ak...@adobe.com> wrote:
>>>
>>>>
>>>> I guess it would make sense to have adapterfactories et. al. to work like this:
>>>> a) if it is not of the desired type, i.e. cannot semantically be adapted, return null
>>
>> example: resource.adaptTo(Node.class) for a resource not backed by a JCR Node instance.
>>
>>>> b) if it fails due to some actual exception, throw a runtimexception
>>
>> example: resource.adaptTo(Comment.class) when the required data to setup the Comment instance cannot be read from persistence or the data is inconsistent and thus a consistent Comment instance cannot be provided.
>>
>>>
>>> I would be fine with that approach. So the only change is a clarification in the Javadocs that adaptTo in fact may throw a RuntimeException (if the AdapterFactory has thrown an exception) and also that AdapterFactory may throw a RuntimeException.
>>
>> The question always remains: Do you expect the caller to handle this exception in some way or another ? Also, what exception can be expected by the client (you don't want to catch RuntimeException, do you ?) ? and what does it mean ?
>>
>> If handling just is catching and logging, there is no use in throwing in the first place — better immediately log and return some decent value that client can cope with, which in the case of adaptTo is just null (as documented). Plus: the boiler plate to catch and log is more complicated and convoluted than the boiler plate for the null check.
>>
>> Regards
>> Felix
>>
>>
>>> As Felix Meschberger already pointed out, neither the SlingAdaptable nor the AdapterManager currently catch any exceptions so that would work already with existing code and Sling Models could start right away throwing RuntimeExceptions for validation purposes.
>>>
>>>>
>>>> But not sure if that will work.
>>>>
>>>> Cheers,
>>>> Alex
>>>
>>
>
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
The AdapterFactory should clearly state, which RuntimeExceptions are thrown under which condition. You know in most of the cases, which AdapterFactory is responsible for your adaptTo-method (or you should be able to find out with the web console)
Handling in some cases is more than simple logging. The AEM6 WCMDeveloperModeFilter is a good example for another error treatment (catching the error within a servlet filter and then exposing via the Web UI).
On 03 Jul 2014, at 12:19, Felix Meschberger <fm...@adobe.com> wrote:
> Hi
>
> Am 03.07.2014 um 11:10 schrieb Konrad Windszus <ko...@gmx.de>:
>
>>
>> On 03 Jul 2014, at 10:50, Alexander Klimetschek <ak...@adobe.com> wrote:
>>
>>>
>>> I guess it would make sense to have adapterfactories et. al. to work like this:
>>> a) if it is not of the desired type, i.e. cannot semantically be adapted, return null
>
> example: resource.adaptTo(Node.class) for a resource not backed by a JCR Node instance.
>
>>> b) if it fails due to some actual exception, throw a runtimexception
>
> example: resource.adaptTo(Comment.class) when the required data to setup the Comment instance cannot be read from persistence or the data is inconsistent and thus a consistent Comment instance cannot be provided.
>
>>
>> I would be fine with that approach. So the only change is a clarification in the Javadocs that adaptTo in fact may throw a RuntimeException (if the AdapterFactory has thrown an exception) and also that AdapterFactory may throw a RuntimeException.
>
> The question always remains: Do you expect the caller to handle this exception in some way or another ? Also, what exception can be expected by the client (you don't want to catch RuntimeException, do you ?) ? and what does it mean ?
>
> If handling just is catching and logging, there is no use in throwing in the first place — better immediately log and return some decent value that client can cope with, which in the case of adaptTo is just null (as documented). Plus: the boiler plate to catch and log is more complicated and convoluted than the boiler plate for the null check.
>
> Regards
> Felix
>
>
>> As Felix Meschberger already pointed out, neither the SlingAdaptable nor the AdapterManager currently catch any exceptions so that would work already with existing code and Sling Models could start right away throwing RuntimeExceptions for validation purposes.
>>
>>>
>>> But not sure if that will work.
>>>
>>> Cheers,
>>> Alex
>>
>
Re: adaptTo and results ....
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 03.07.2014, at 12:19, Felix Meschberger <fm...@adobe.com> wrote:
>>> I guess it would make sense to have adapterfactories et. al. to work like this:
>>> a) if it is not of the desired type, i.e. cannot semantically be adapted, return null
>
> example: resource.adaptTo(Node.class) for a resource not backed by a JCR Node instance.
>
>>> b) if it fails due to some actual exception, throw a runtimexception
>
> example: resource.adaptTo(Comment.class) when the required data to setup the Comment instance cannot be read from persistence or the data is inconsistent and thus a consistent Comment instance cannot be provided.
This is on the edge - could also be seen as a). To me b) would be if while creating/reading that Comment instance, some unexpected JCR RepositoryException is thrown (i.e. network failure).
> If handling just is catching and logging, there is no use in throwing in the first place — better immediately log and return some decent value that client can cope with, which in the case of adaptTo is just null (as documented). Plus: the boiler plate to catch and log is more complicated and convoluted than the boiler plate for the null check.
If it's a runtime exception, you don't have to catch it immediately. In a request context, the sling request execution would catch & log that exception for you, thus by default the request would fail, which makes sense in case of an unexpected error like b). But if you want, you could catch it too (i.e. would need to be documented what kind of RuntimeException) and do things like retry etc. (although generally b) is something you can't do much about and almost always would make the whole request or operation fail).
Cheers,
Alex
Re: adaptTo and results ....
Posted by Felix Meschberger <fm...@adobe.com>.
Hi
Am 03.07.2014 um 11:10 schrieb Konrad Windszus <ko...@gmx.de>:
>
> On 03 Jul 2014, at 10:50, Alexander Klimetschek <ak...@adobe.com> wrote:
>
>>
>> I guess it would make sense to have adapterfactories et. al. to work like this:
>> a) if it is not of the desired type, i.e. cannot semantically be adapted, return null
example: resource.adaptTo(Node.class) for a resource not backed by a JCR Node instance.
>> b) if it fails due to some actual exception, throw a runtimexception
example: resource.adaptTo(Comment.class) when the required data to setup the Comment instance cannot be read from persistence or the data is inconsistent and thus a consistent Comment instance cannot be provided.
>
> I would be fine with that approach. So the only change is a clarification in the Javadocs that adaptTo in fact may throw a RuntimeException (if the AdapterFactory has thrown an exception) and also that AdapterFactory may throw a RuntimeException.
The question always remains: Do you expect the caller to handle this exception in some way or another ? Also, what exception can be expected by the client (you don't want to catch RuntimeException, do you ?) ? and what does it mean ?
If handling just is catching and logging, there is no use in throwing in the first place — better immediately log and return some decent value that client can cope with, which in the case of adaptTo is just null (as documented). Plus: the boiler plate to catch and log is more complicated and convoluted than the boiler plate for the null check.
Regards
Felix
> As Felix Meschberger already pointed out, neither the SlingAdaptable nor the AdapterManager currently catch any exceptions so that would work already with existing code and Sling Models could start right away throwing RuntimeExceptions for validation purposes.
>
>>
>> But not sure if that will work.
>>
>> Cheers,
>> Alex
>
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
On 03 Jul 2014, at 10:50, Alexander Klimetschek <ak...@adobe.com> wrote:
>
> I guess it would make sense to have adapterfactories et. al. to work like this:
> a) if it is not of the desired type, i.e. cannot semantically be adapted, return null
> b) if it fails due to some actual exception, throw a runtimexception
I would be fine with that approach. So the only change is a clarification in the Javadocs that adaptTo in fact may throw a RuntimeException (if the AdapterFactory has thrown an exception) and also that AdapterFactory may throw a RuntimeException.
As Felix Meschberger already pointed out, neither the SlingAdaptable nor the AdapterManager currently catch any exceptions so that would work already with existing code and Sling Models could start right away throwing RuntimeExceptions for validation purposes.
>
> But not sure if that will work.
>
> Cheers,
> Alex
RE: adaptTo and results ....
Posted by Stefan Seifert <ss...@pro-vision.de>.
>* My original suggestion of using a Result interface. This requires
>more verbose code on the caller side -- the caller needs to check a
>success flag -- but allows for fine-grained information (which would
>be appropriate for a validation use case).
+1
https://issues.apache.org/jira/browse/SLING-3714
and following alex hint it is still possible to use separate static method to convert this result object adaption call into a single-line "adaptOrThrow" method with throws a runtime exception, without having to build this into the adaptto API. for projects that use this a lot.
stefan
Re: adaptTo and results ....
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 07.07.2014, at 18:14, Justin Edelson <ju...@justinedelson.com> wrote:
> I found a more concrete example in the AEM codebase (so apologies to
> the non-Adobe people on this thread who will just have to take my word
> for it). The adapter factory which adapts Resources into Scene7 "set"
> objects makes a number of validations before returning a non-null
> result:
> 1) Is the Resource an Asset?
> 2) Does the Asset represet a Scene7 set? (which is done by looking at
> a property)
> 3) Does the requested set class correspond to the set type of the Asset?
Generally, I happen to know that this code is still evolving and I wouldn't see it as a good example ;)
More specifically, this should really just look at a resource type and adapt or not. Nothing specific here compared to all the normal adaptto cases that look at node or resource type to map to a specific class.
Cheers,
Alex
Re: adaptTo and results ....
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 07.07.2014, at 18:42, Carsten Ziegeler <cz...@apache.org> wrote:
> This doesn't really convince me - the same argument would hold true for
> every API where the exception (cause) is logged, but the method just gives
> back true/false,object/null. Even for APIs throwing an exception it might
> be hard to get a meaningful message to developer. So this isn't done for
> other APIs, why should we do it differently for adaptTo?
> In addition, if you have a lot of client code using the adapter pattern,
> then you end up in converting the exception to a meaningful message in
> various places.
>
> It would be so easy to let the adapter factory do a meaningful log
> statement and there are tools/apis to pick up this log message and display
> it to the dev without requiring the developer to go to the log
Ack.
Cheers,
Alex
Re: adaptTo and results ....
Posted by Justin Edelson <ju...@justinedelson.com>.
You're missing my point. How would your Sightly script indicate that a
RuntimeException should be thrown in the first place? Or are you
suggesting that Sightly assume that this is always the case?
On Mon, Jul 28, 2014 at 12:07 PM, Konrad Windszus <ko...@gmx.de> wrote:
> In my regard in this case a RuntimeException would be fine. That would be propagated correctly to the script level.
> So whenever a model class has the model annotation and something went wrong during the injection throwing a runtime exception would be correctly propagated and no other option would be tried (even when using Sightlies Use Extension)
>
> On 28 Jul 2014, at 18:03, Justin Edelson <ju...@justinedelson.com> wrote:
>
>> Hi Konrad,
>> In this case, I don't see how any of the options in this thread would
>> actually help because the code which calls adaptTo() is not under your
>> control. So there would be no way for the caller (i.e. your Sightly
>> script) to indicate that such an exception should be thrown.
>>
>> Regards,
>> Justin
>>
>> On Mon, Jul 28, 2014 at 11:44 AM, Konrad Windszus <ko...@gmx.de> wrote:
>>> I just came up with another example from CQ:
>>>
>>> In Sightly you can instantiate a model via the use API [1].
>>> Since that logic will first try to adapt from Resource then from Request and as fallback tries to instantiate the class leveraging the default constructor, you won’t get any exceptions in case required properties cannot be injected (and the default constructor is available).
>>>
>>> In most of the cases instantiating the class via the default constructor is not the right thing to do, because if the class is annotated with @Model and instanciation fails that should be propagated to the user. In this case it defers the error message to an NPE being thrown whenever someone is trying to access the field (which was not instantiated because the object was not created with Sling Models but as a regular POJO with no injections at all).
>>> That takes quite some time to figure out during development, that actually Sling Models cannot really instantiate the class and therefore the Sightly Use Extension will instantiate it as simple Pojo.
>>> That would not have happened, if Sling Models would be allowed to throw exceptions in case the instanciation was not successfull!
>>>
>>> [1] - http://docs.adobe.com/docs/en/aem/6-0/develop/sightly/use-api-in-java.html#Alternatives%20to%20WCMUse
>>>
>>> On 07 Jul 2014, at 18:42, Carsten Ziegeler <cz...@apache.org> wrote:
>>>
>>>> 2014-07-07 18:29 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
>>>>
>>>>> Provide a meaningful error message to the author or at least to the
>>>>> developer (leveraging the WCMDeveloperMode). By meaningful I don’t talk
>>>>> about something hidden within the logs.
>>>>>
>>>>
>>>> This doesn't really convince me - the same argument would hold true for
>>>> every API where the exception (cause) is logged, but the method just gives
>>>> back true/false,object/null. Even for APIs throwing an exception it might
>>>> be hard to get a meaningful message to developer. So this isn't done for
>>>> other APIs, why should we do it differently for adaptTo?
>>>> In addition, if you have a lot of client code using the adapter pattern,
>>>> then you end up in converting the exception to a meaningful message in
>>>> various places.
>>>>
>>>> It would be so easy to let the adapter factory do a meaningful log
>>>> statement and there are tools/apis to pick up this log message and display
>>>> it to the dev without requiring the developer to go to the log
>>>>
>>>> Carsten
>>>>
>>>>
>>>>
>>>>> Konrad
>>>>>
>>>>> On 07 Jul 2014, at 18:27, Carsten Ziegeler <cz...@apache.org> wrote:
>>>>>
>>>>>> 2014-07-07 18:14 GMT+02:00 Justin Edelson <ju...@justinedelson.com>:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>>
>>>>>>> I found a more concrete example in the AEM codebase (so apologies to
>>>>>>> the non-Adobe people on this thread who will just have to take my word
>>>>>>> for it). The adapter factory which adapts Resources into Scene7 "set"
>>>>>>> objects makes a number of validations before returning a non-null
>>>>>>> result:
>>>>>>> 1) Is the Resource an Asset?
>>>>>>> 2) Does the Asset represet a Scene7 set? (which is done by looking at
>>>>>>> a property)
>>>>>>> 3) Does the requested set class correspond to the set type of the Asset?
>>>>>>>
>>>>>>>
>>>>>> But again, what different action would a client take depending on the
>>>>> error
>>>>>> condition 1, 2 or 3?
>>>>>>
>>>>>> Carsten
>>>>>>
>>>>>>
>>>>>>> Regards,
>>>>>>> Justin
>>>>>>>
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Alex
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Carsten Ziegeler
>>>>>> Adobe Research Switzerland
>>>>>> cziegeler@apache.org
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Carsten Ziegeler
>>>> Adobe Research Switzerland
>>>> cziegeler@apache.org
>>>
>
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
In my regard in this case a RuntimeException would be fine. That would be propagated correctly to the script level.
So whenever a model class has the model annotation and something went wrong during the injection throwing a runtime exception would be correctly propagated and no other option would be tried (even when using Sightlies Use Extension)
On 28 Jul 2014, at 18:03, Justin Edelson <ju...@justinedelson.com> wrote:
> Hi Konrad,
> In this case, I don't see how any of the options in this thread would
> actually help because the code which calls adaptTo() is not under your
> control. So there would be no way for the caller (i.e. your Sightly
> script) to indicate that such an exception should be thrown.
>
> Regards,
> Justin
>
> On Mon, Jul 28, 2014 at 11:44 AM, Konrad Windszus <ko...@gmx.de> wrote:
>> I just came up with another example from CQ:
>>
>> In Sightly you can instantiate a model via the use API [1].
>> Since that logic will first try to adapt from Resource then from Request and as fallback tries to instantiate the class leveraging the default constructor, you won’t get any exceptions in case required properties cannot be injected (and the default constructor is available).
>>
>> In most of the cases instantiating the class via the default constructor is not the right thing to do, because if the class is annotated with @Model and instanciation fails that should be propagated to the user. In this case it defers the error message to an NPE being thrown whenever someone is trying to access the field (which was not instantiated because the object was not created with Sling Models but as a regular POJO with no injections at all).
>> That takes quite some time to figure out during development, that actually Sling Models cannot really instantiate the class and therefore the Sightly Use Extension will instantiate it as simple Pojo.
>> That would not have happened, if Sling Models would be allowed to throw exceptions in case the instanciation was not successfull!
>>
>> [1] - http://docs.adobe.com/docs/en/aem/6-0/develop/sightly/use-api-in-java.html#Alternatives%20to%20WCMUse
>>
>> On 07 Jul 2014, at 18:42, Carsten Ziegeler <cz...@apache.org> wrote:
>>
>>> 2014-07-07 18:29 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
>>>
>>>> Provide a meaningful error message to the author or at least to the
>>>> developer (leveraging the WCMDeveloperMode). By meaningful I don’t talk
>>>> about something hidden within the logs.
>>>>
>>>
>>> This doesn't really convince me - the same argument would hold true for
>>> every API where the exception (cause) is logged, but the method just gives
>>> back true/false,object/null. Even for APIs throwing an exception it might
>>> be hard to get a meaningful message to developer. So this isn't done for
>>> other APIs, why should we do it differently for adaptTo?
>>> In addition, if you have a lot of client code using the adapter pattern,
>>> then you end up in converting the exception to a meaningful message in
>>> various places.
>>>
>>> It would be so easy to let the adapter factory do a meaningful log
>>> statement and there are tools/apis to pick up this log message and display
>>> it to the dev without requiring the developer to go to the log
>>>
>>> Carsten
>>>
>>>
>>>
>>>> Konrad
>>>>
>>>> On 07 Jul 2014, at 18:27, Carsten Ziegeler <cz...@apache.org> wrote:
>>>>
>>>>> 2014-07-07 18:14 GMT+02:00 Justin Edelson <ju...@justinedelson.com>:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> I found a more concrete example in the AEM codebase (so apologies to
>>>>>> the non-Adobe people on this thread who will just have to take my word
>>>>>> for it). The adapter factory which adapts Resources into Scene7 "set"
>>>>>> objects makes a number of validations before returning a non-null
>>>>>> result:
>>>>>> 1) Is the Resource an Asset?
>>>>>> 2) Does the Asset represet a Scene7 set? (which is done by looking at
>>>>>> a property)
>>>>>> 3) Does the requested set class correspond to the set type of the Asset?
>>>>>>
>>>>>>
>>>>> But again, what different action would a client take depending on the
>>>> error
>>>>> condition 1, 2 or 3?
>>>>>
>>>>> Carsten
>>>>>
>>>>>
>>>>>> Regards,
>>>>>> Justin
>>>>>>
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Alex
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Carsten Ziegeler
>>>>> Adobe Research Switzerland
>>>>> cziegeler@apache.org
>>>>
>>>>
>>>
>>>
>>> --
>>> Carsten Ziegeler
>>> Adobe Research Switzerland
>>> cziegeler@apache.org
>>
Re: adaptTo and results ....
Posted by Justin Edelson <ju...@justinedelson.com>.
Hi Konrad,
In this case, I don't see how any of the options in this thread would
actually help because the code which calls adaptTo() is not under your
control. So there would be no way for the caller (i.e. your Sightly
script) to indicate that such an exception should be thrown.
Regards,
Justin
On Mon, Jul 28, 2014 at 11:44 AM, Konrad Windszus <ko...@gmx.de> wrote:
> I just came up with another example from CQ:
>
> In Sightly you can instantiate a model via the use API [1].
> Since that logic will first try to adapt from Resource then from Request and as fallback tries to instantiate the class leveraging the default constructor, you won’t get any exceptions in case required properties cannot be injected (and the default constructor is available).
>
> In most of the cases instantiating the class via the default constructor is not the right thing to do, because if the class is annotated with @Model and instanciation fails that should be propagated to the user. In this case it defers the error message to an NPE being thrown whenever someone is trying to access the field (which was not instantiated because the object was not created with Sling Models but as a regular POJO with no injections at all).
> That takes quite some time to figure out during development, that actually Sling Models cannot really instantiate the class and therefore the Sightly Use Extension will instantiate it as simple Pojo.
> That would not have happened, if Sling Models would be allowed to throw exceptions in case the instanciation was not successfull!
>
> [1] - http://docs.adobe.com/docs/en/aem/6-0/develop/sightly/use-api-in-java.html#Alternatives%20to%20WCMUse
>
> On 07 Jul 2014, at 18:42, Carsten Ziegeler <cz...@apache.org> wrote:
>
>> 2014-07-07 18:29 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
>>
>>> Provide a meaningful error message to the author or at least to the
>>> developer (leveraging the WCMDeveloperMode). By meaningful I don’t talk
>>> about something hidden within the logs.
>>>
>>
>> This doesn't really convince me - the same argument would hold true for
>> every API where the exception (cause) is logged, but the method just gives
>> back true/false,object/null. Even for APIs throwing an exception it might
>> be hard to get a meaningful message to developer. So this isn't done for
>> other APIs, why should we do it differently for adaptTo?
>> In addition, if you have a lot of client code using the adapter pattern,
>> then you end up in converting the exception to a meaningful message in
>> various places.
>>
>> It would be so easy to let the adapter factory do a meaningful log
>> statement and there are tools/apis to pick up this log message and display
>> it to the dev without requiring the developer to go to the log
>>
>> Carsten
>>
>>
>>
>>> Konrad
>>>
>>> On 07 Jul 2014, at 18:27, Carsten Ziegeler <cz...@apache.org> wrote:
>>>
>>>> 2014-07-07 18:14 GMT+02:00 Justin Edelson <ju...@justinedelson.com>:
>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>> I found a more concrete example in the AEM codebase (so apologies to
>>>>> the non-Adobe people on this thread who will just have to take my word
>>>>> for it). The adapter factory which adapts Resources into Scene7 "set"
>>>>> objects makes a number of validations before returning a non-null
>>>>> result:
>>>>> 1) Is the Resource an Asset?
>>>>> 2) Does the Asset represet a Scene7 set? (which is done by looking at
>>>>> a property)
>>>>> 3) Does the requested set class correspond to the set type of the Asset?
>>>>>
>>>>>
>>>> But again, what different action would a client take depending on the
>>> error
>>>> condition 1, 2 or 3?
>>>>
>>>> Carsten
>>>>
>>>>
>>>>> Regards,
>>>>> Justin
>>>>>
>>>>>>
>>>>>> Cheers,
>>>>>> Alex
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Carsten Ziegeler
>>>> Adobe Research Switzerland
>>>> cziegeler@apache.org
>>>
>>>
>>
>>
>> --
>> Carsten Ziegeler
>> Adobe Research Switzerland
>> cziegeler@apache.org
>
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
I just came up with another example from CQ:
In Sightly you can instantiate a model via the use API [1].
Since that logic will first try to adapt from Resource then from Request and as fallback tries to instantiate the class leveraging the default constructor, you won’t get any exceptions in case required properties cannot be injected (and the default constructor is available).
In most of the cases instantiating the class via the default constructor is not the right thing to do, because if the class is annotated with @Model and instanciation fails that should be propagated to the user. In this case it defers the error message to an NPE being thrown whenever someone is trying to access the field (which was not instantiated because the object was not created with Sling Models but as a regular POJO with no injections at all).
That takes quite some time to figure out during development, that actually Sling Models cannot really instantiate the class and therefore the Sightly Use Extension will instantiate it as simple Pojo.
That would not have happened, if Sling Models would be allowed to throw exceptions in case the instanciation was not successfull!
[1] - http://docs.adobe.com/docs/en/aem/6-0/develop/sightly/use-api-in-java.html#Alternatives%20to%20WCMUse
On 07 Jul 2014, at 18:42, Carsten Ziegeler <cz...@apache.org> wrote:
> 2014-07-07 18:29 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
>
>> Provide a meaningful error message to the author or at least to the
>> developer (leveraging the WCMDeveloperMode). By meaningful I don’t talk
>> about something hidden within the logs.
>>
>
> This doesn't really convince me - the same argument would hold true for
> every API where the exception (cause) is logged, but the method just gives
> back true/false,object/null. Even for APIs throwing an exception it might
> be hard to get a meaningful message to developer. So this isn't done for
> other APIs, why should we do it differently for adaptTo?
> In addition, if you have a lot of client code using the adapter pattern,
> then you end up in converting the exception to a meaningful message in
> various places.
>
> It would be so easy to let the adapter factory do a meaningful log
> statement and there are tools/apis to pick up this log message and display
> it to the dev without requiring the developer to go to the log
>
> Carsten
>
>
>
>> Konrad
>>
>> On 07 Jul 2014, at 18:27, Carsten Ziegeler <cz...@apache.org> wrote:
>>
>>> 2014-07-07 18:14 GMT+02:00 Justin Edelson <ju...@justinedelson.com>:
>>>
>>>> Hi,
>>>>
>>>>
>>>> I found a more concrete example in the AEM codebase (so apologies to
>>>> the non-Adobe people on this thread who will just have to take my word
>>>> for it). The adapter factory which adapts Resources into Scene7 "set"
>>>> objects makes a number of validations before returning a non-null
>>>> result:
>>>> 1) Is the Resource an Asset?
>>>> 2) Does the Asset represet a Scene7 set? (which is done by looking at
>>>> a property)
>>>> 3) Does the requested set class correspond to the set type of the Asset?
>>>>
>>>>
>>> But again, what different action would a client take depending on the
>> error
>>> condition 1, 2 or 3?
>>>
>>> Carsten
>>>
>>>
>>>> Regards,
>>>> Justin
>>>>
>>>>>
>>>>> Cheers,
>>>>> Alex
>>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> Carsten Ziegeler
>>> Adobe Research Switzerland
>>> cziegeler@apache.org
>>
>>
>
>
> --
> Carsten Ziegeler
> Adobe Research Switzerland
> cziegeler@apache.org
Re: adaptTo and results ....
Posted by Carsten Ziegeler <cz...@apache.org>.
2014-07-07 18:29 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
> Provide a meaningful error message to the author or at least to the
> developer (leveraging the WCMDeveloperMode). By meaningful I don’t talk
> about something hidden within the logs.
>
This doesn't really convince me - the same argument would hold true for
every API where the exception (cause) is logged, but the method just gives
back true/false,object/null. Even for APIs throwing an exception it might
be hard to get a meaningful message to developer. So this isn't done for
other APIs, why should we do it differently for adaptTo?
In addition, if you have a lot of client code using the adapter pattern,
then you end up in converting the exception to a meaningful message in
various places.
It would be so easy to let the adapter factory do a meaningful log
statement and there are tools/apis to pick up this log message and display
it to the dev without requiring the developer to go to the log
Carsten
> Konrad
>
> On 07 Jul 2014, at 18:27, Carsten Ziegeler <cz...@apache.org> wrote:
>
> > 2014-07-07 18:14 GMT+02:00 Justin Edelson <ju...@justinedelson.com>:
> >
> >> Hi,
> >>
> >>
> >> I found a more concrete example in the AEM codebase (so apologies to
> >> the non-Adobe people on this thread who will just have to take my word
> >> for it). The adapter factory which adapts Resources into Scene7 "set"
> >> objects makes a number of validations before returning a non-null
> >> result:
> >> 1) Is the Resource an Asset?
> >> 2) Does the Asset represet a Scene7 set? (which is done by looking at
> >> a property)
> >> 3) Does the requested set class correspond to the set type of the Asset?
> >>
> >>
> > But again, what different action would a client take depending on the
> error
> > condition 1, 2 or 3?
> >
> > Carsten
> >
> >
> >> Regards,
> >> Justin
> >>
> >>>
> >>> Cheers,
> >>> Alex
> >>>
> >>
> >
> >
> >
> > --
> > Carsten Ziegeler
> > Adobe Research Switzerland
> > cziegeler@apache.org
>
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
Provide a meaningful error message to the author or at least to the developer (leveraging the WCMDeveloperMode). By meaningful I don’t talk about something hidden within the logs.
Konrad
On 07 Jul 2014, at 18:27, Carsten Ziegeler <cz...@apache.org> wrote:
> 2014-07-07 18:14 GMT+02:00 Justin Edelson <ju...@justinedelson.com>:
>
>> Hi,
>>
>>
>> I found a more concrete example in the AEM codebase (so apologies to
>> the non-Adobe people on this thread who will just have to take my word
>> for it). The adapter factory which adapts Resources into Scene7 "set"
>> objects makes a number of validations before returning a non-null
>> result:
>> 1) Is the Resource an Asset?
>> 2) Does the Asset represet a Scene7 set? (which is done by looking at
>> a property)
>> 3) Does the requested set class correspond to the set type of the Asset?
>>
>>
> But again, what different action would a client take depending on the error
> condition 1, 2 or 3?
>
> Carsten
>
>
>> Regards,
>> Justin
>>
>>>
>>> Cheers,
>>> Alex
>>>
>>
>
>
>
> --
> Carsten Ziegeler
> Adobe Research Switzerland
> cziegeler@apache.org
Re: adaptTo and results ....
Posted by Carsten Ziegeler <cz...@apache.org>.
2014-07-07 18:14 GMT+02:00 Justin Edelson <ju...@justinedelson.com>:
> Hi,
>
>
> I found a more concrete example in the AEM codebase (so apologies to
> the non-Adobe people on this thread who will just have to take my word
> for it). The adapter factory which adapts Resources into Scene7 "set"
> objects makes a number of validations before returning a non-null
> result:
> 1) Is the Resource an Asset?
> 2) Does the Asset represet a Scene7 set? (which is done by looking at
> a property)
> 3) Does the requested set class correspond to the set type of the Asset?
>
>
But again, what different action would a client take depending on the error
condition 1, 2 or 3?
Carsten
> Regards,
> Justin
>
> >
> > Cheers,
> > Alex
> >
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: adaptTo and results ....
Posted by Justin Edelson <ju...@justinedelson.com>.
Hi,
On Mon, Jul 7, 2014 at 5:57 PM, Alexander Klimetschek
<ak...@adobe.com> wrote:
> On 07.07.2014, at 17:08, Carsten Ziegeler <cz...@apache.org> wrote:
>
>> 2014-07-07 14:55 GMT+02:00 Justin Edelson <ju...@justinedelson.com>:
>>
>>> Here's a sightly more real world case... let's say you have a call like
>>> this:
>>>
>>> Comment comment = resource.adaptTo(Comment.class);
>>>
>>> And for a Resource to be successfully adapted to a Comment, it must
>>> satisfy two criteria:
>>> 1) The resource type must be myco/comment
>>> 2) It must have a property called "commentType" (OK, this part isn't
>>> so real world).
>>>
>>> Right now, the caller has no way of knowing which of these critera
>>> wasn't met. That's IMHO the crux of this request - to provide a way
>>> for AdapterFactories to surface the failure reason back to the caller.
>>>
>>>
>> Hmm, this assumes the caller can do something meaningful with it. Given
>> your example, what could the client do?
Konrad could probably articulate this better than I can :)
>
> Right.
>
> In this case I would assume that if 1) is present, you get a Comment back, otherwise null. Then Comment has a getter method for the type 2), which would also handle the case of a missing type: usually you would fall back to a default if no type is specified, since that reduces the constraints the content has to follow; but you could also have the application handle the no-type case itself and fail in some way (as you were trying to with an exception that is passed through from adapterfactory to application code).
>
> Next example please :D
I found a more concrete example in the AEM codebase (so apologies to
the non-Adobe people on this thread who will just have to take my word
for it). The adapter factory which adapts Resources into Scene7 "set"
objects makes a number of validations before returning a non-null
result:
1) Is the Resource an Asset?
2) Does the Asset represet a Scene7 set? (which is done by looking at
a property)
3) Does the requested set class correspond to the set type of the Asset?
Regards,
Justin
>
> Cheers,
> Alex
>
Re: adaptTo and results ....
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 07.07.2014, at 17:08, Carsten Ziegeler <cz...@apache.org> wrote:
> 2014-07-07 14:55 GMT+02:00 Justin Edelson <ju...@justinedelson.com>:
>
>> Here's a sightly more real world case... let's say you have a call like
>> this:
>>
>> Comment comment = resource.adaptTo(Comment.class);
>>
>> And for a Resource to be successfully adapted to a Comment, it must
>> satisfy two criteria:
>> 1) The resource type must be myco/comment
>> 2) It must have a property called "commentType" (OK, this part isn't
>> so real world).
>>
>> Right now, the caller has no way of knowing which of these critera
>> wasn't met. That's IMHO the crux of this request - to provide a way
>> for AdapterFactories to surface the failure reason back to the caller.
>>
>>
> Hmm, this assumes the caller can do something meaningful with it. Given
> your example, what could the client do?
Right.
In this case I would assume that if 1) is present, you get a Comment back, otherwise null. Then Comment has a getter method for the type 2), which would also handle the case of a missing type: usually you would fall back to a default if no type is specified, since that reduces the constraints the content has to follow; but you could also have the application handle the no-type case itself and fail in some way (as you were trying to with an exception that is passed through from adapterfactory to application code).
Next example please :D
Cheers,
Alex
Re: adaptTo and results ....
Posted by Carsten Ziegeler <cz...@apache.org>.
2014-07-07 14:55 GMT+02:00 Justin Edelson <ju...@justinedelson.com>:
>
>
> Here's a sightly more real world case... let's say you have a call like
> this:
>
> Comment comment = resource.adaptTo(Comment.class);
>
> And for a Resource to be successfully adapted to a Comment, it must
> satisfy two criteria:
> 1) The resource type must be myco/comment
> 2) It must have a property called "commentType" (OK, this part isn't
> so real world).
>
> Right now, the caller has no way of knowing which of these critera
> wasn't met. That's IMHO the crux of this request - to provide a way
> for AdapterFactories to surface the failure reason back to the caller.
>
>
Hmm, this assumes the caller can do something meaningful with it. Given
your example, what could the client do?
Regards
Carsten
> Regards,
> Justin
>
>
> >
> > [1] http://markmail.org/message/lcujo4flwek3liez
> >
> > Cheers,
> > Alex
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: adaptTo and results ....
Posted by Justin Edelson <ju...@justinedelson.com>.
Hi,
On Thu, Jul 3, 2014 at 3:07 PM, Alexander Klimetschek
<ak...@adobe.com> wrote:
> On 03.07.2014, at 13:58, Justin Edelson <ju...@justinedelson.com> wrote:
>
>> It won't work :) This is a hugely non-backwards compatible change. It
>> happens to be binary compatible, but it is not semantically compatible
>> (which is in some ways just as important). Callers of adaptTo() assume
>> (because we have told them to assume) that null will be returned if
>> the adaptation can't be done. We can't now start throwing exceptions.
>> Callers won't expect this.
>
> There is a conflict with the other stated problem: that most callers don't expect null either :) So if we change something, this will have an effect on at least some callers either way, unless we add a new method with a different semantic.
>
> But I'd say this is just adding complexity for no notable benefit. And just improving the logging in case of exceptions in AdpaterFactories and Adaptables or that static adaptOrThrow helper should be enough.
>
> Maybe some actual real world cases would help (i.e. no "Foo.class" adaptations :). The only one I see is the Sling Models validation case as originally outlined here [1] - but could you elaborate? I probably miss the knowledge about sling models to see the issue.
Here's a sightly more real world case... let's say you have a call like this:
Comment comment = resource.adaptTo(Comment.class);
And for a Resource to be successfully adapted to a Comment, it must
satisfy two criteria:
1) The resource type must be myco/comment
2) It must have a property called "commentType" (OK, this part isn't
so real world).
Right now, the caller has no way of knowing which of these critera
wasn't met. That's IMHO the crux of this request - to provide a way
for AdapterFactories to surface the failure reason back to the caller.
Regards,
Justin
>
> [1] http://markmail.org/message/lcujo4flwek3liez
>
> Cheers,
> Alex
Re: adaptTo and results ....
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 03.07.2014, at 13:58, Justin Edelson <ju...@justinedelson.com> wrote:
> It won't work :) This is a hugely non-backwards compatible change. It
> happens to be binary compatible, but it is not semantically compatible
> (which is in some ways just as important). Callers of adaptTo() assume
> (because we have told them to assume) that null will be returned if
> the adaptation can't be done. We can't now start throwing exceptions.
> Callers won't expect this.
There is a conflict with the other stated problem: that most callers don't expect null either :) So if we change something, this will have an effect on at least some callers either way, unless we add a new method with a different semantic.
But I'd say this is just adding complexity for no notable benefit. And just improving the logging in case of exceptions in AdpaterFactories and Adaptables or that static adaptOrThrow helper should be enough.
Maybe some actual real world cases would help (i.e. no "Foo.class" adaptations :). The only one I see is the Sling Models validation case as originally outlined here [1] - but could you elaborate? I probably miss the knowledge about sling models to see the issue.
[1] http://markmail.org/message/lcujo4flwek3liez
Cheers,
Alex
Re: adaptTo and results ....
Posted by Justin Edelson <ju...@justinedelson.com>.
Hi,
On Thu, Jul 3, 2014 at 4:50 AM, Alexander Klimetschek
<ak...@adobe.com> wrote:
> On 03.07.2014, at 09:12, Konrad Windszus <ko...@gmx.de> wrote:
>
>> - The client can decide how to expose that error (whether just logging is fine or something more obvious). This cannot be achieved by just setting up a utility method, because that one does only see the null return value and not the original reason for that.
>
> Yes, but my question is whether there is any need to pass through the exception at all.
>
>> - Tracing problems during development is much easier (instead of looking at the log I can see the full exception)
>
> You can debug exceptions inside the adapterfactories as well (after seeing them in the log).
>
>> - It allows to use it for something like validation in Sling Models
>
> How would that work? (I saw the reference earlier in the thread, but I don't know how you'd use adaptTo for validation and can't really imagine it's a good fit)
Validation means a lot of things; I would say that your example below
where a resource has to be a certain type to be adapted to a resource
collection is a form of validation. As you note, using exceptions for
validation use cases is wrong.
>
>> - It is less error-prone to the developers (you can easily forget to either use the utility method or check for null)
>
> The null-returning method is out there, it cannot be changed to throw a checked exception (which is the only way to force handling for devs)
>
>> - In my regard in most of the cases if adaptation fails, there is something wrong with the deployment (required bundles are not installed, components are not running, ….) and I cannot reasonably work around that issue in the code -> that calls for an exception
>
> It's not only exception, it's also a way to check if something is of a certain type (say adapting a resource to a resourcecollection). In this case an exception is not the right thing.
>
> I guess it would make sense to have adapterfactories et. al. to work like this:
> a) if it is not of the desired type, i.e. cannot semantically be adapted, return null
> b) if it fails due to some actual exception, throw a runtimexception
>
> But not sure if that will work.
It won't work :) This is a hugely non-backwards compatible change. It
happens to be binary compatible, but it is not semantically compatible
(which is in some ways just as important). Callers of adaptTo() assume
(because we have told them to assume) that null will be returned if
the adaptation can't be done. We can't now start throwing exceptions.
Callers won't expect this.
The caller *must* indicate in some way that she wants behavior which
is different than the current. AFAICT, there are only two viable
solutions:
* My original suggestion of using a Result interface. This requires
more verbose code on the caller side -- the caller needs to check a
success flag -- but allows for fine-grained information (which would
be appropriate for a validation use case).
* Bertrand's suggestion of using some kind of ThreadLocal. Less
verbose code on the caller side. Would seem to violate Effective Java
#57 in that it is using exceptions for control flow.
Both cases can be implemented without impacting existing callers or
adapter factories. Adapter factories would need indicate that they
support the Result interface or exception throwing via a service
property. For factories without the property, the AdapterManagerImpl
would simulate the proper behavior.
Justin
>
> Cheers,
> Alex
Re: adaptTo and results ....
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 03.07.2014, at 09:12, Konrad Windszus <ko...@gmx.de> wrote:
> - The client can decide how to expose that error (whether just logging is fine or something more obvious). This cannot be achieved by just setting up a utility method, because that one does only see the null return value and not the original reason for that.
Yes, but my question is whether there is any need to pass through the exception at all.
> - Tracing problems during development is much easier (instead of looking at the log I can see the full exception)
You can debug exceptions inside the adapterfactories as well (after seeing them in the log).
> - It allows to use it for something like validation in Sling Models
How would that work? (I saw the reference earlier in the thread, but I don't know how you'd use adaptTo for validation and can't really imagine it's a good fit)
> - It is less error-prone to the developers (you can easily forget to either use the utility method or check for null)
The null-returning method is out there, it cannot be changed to throw a checked exception (which is the only way to force handling for devs)
> - In my regard in most of the cases if adaptation fails, there is something wrong with the deployment (required bundles are not installed, components are not running, ….) and I cannot reasonably work around that issue in the code -> that calls for an exception
It's not only exception, it's also a way to check if something is of a certain type (say adapting a resource to a resourcecollection). In this case an exception is not the right thing.
I guess it would make sense to have adapterfactories et. al. to work like this:
a) if it is not of the desired type, i.e. cannot semantically be adapted, return null
b) if it fails due to some actual exception, throw a runtimexception
But not sure if that will work.
Cheers,
Alex
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
Passing through exceptions has the following advantages IMHO:
- The client can decide how to expose that error (whether just logging is fine or something more obvious). This cannot be achieved by just setting up a utility method, because that one does only see the null return value and not the original reason for that.
- Tracing problems during development is much easier (instead of looking at the log I can see the full exception)
- It allows to use it for something like validation in Sling Models
- It is less error-prone to the developers (you can easily forget to either use the utility method or check for null)
- In my regard in most of the cases if adaptation fails, there is something wrong with the deployment (required bundles are not installed, components are not running, ….) and I cannot reasonably work around that issue in the code -> that calls for an exception
Regards,
Konrad
On 02 Jul 2014, at 12:33, Alexander Klimetschek <ak...@adobe.com> wrote:
> Just reading up on this and have the basic question: what is the motivation for passing through the exceptions?
>
> From what I can see it is simply that the exceptions become visible to the developer, which can be done by properly logging them (in the adapterfactories etc.). It was mentioned that this decision (whether to log or not) depends on the application = user of the adaptTo call. But I haven't seen an example of that and are unsure that really is the case.
>
> The example Stefan gave [1] is just about removing the boilerplate of the null check + throwing a runtime exception, which could be handled using a static utility method (adaptOrThrow, but outside the adaptable interface). Also, this seems to be heavily depending on the style of the application code, whether exceptions are used a lot or null handling (using optionals etc.) is prefered and done consistently. Hence I am not sure if that requires a major change in the adaptable interface contract.
>
> [1] https://issues.apache.org/jira/browse/SLING-3714?focusedCommentId=14048040&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14048040
>
> Cheers,
> Alex
RE: adaptTo and results ....
Posted by Stefan Seifert <ss...@pro-vision.de>.
>The example Stefan gave [1] is just about removing the boilerplate of the null
>check + throwing a runtime exception, which could be handled using a static
>utility method (adaptOrThrow, but outside the adaptable interface).
yes, you are right - this would be an alternative for this simple usecase with the null-check.
stefan
Re: adaptTo and results ....
Posted by Alexander Klimetschek <ak...@adobe.com>.
Just reading up on this and have the basic question: what is the motivation for passing through the exceptions?
>From what I can see it is simply that the exceptions become visible to the developer, which can be done by properly logging them (in the adapterfactories etc.). It was mentioned that this decision (whether to log or not) depends on the application = user of the adaptTo call. But I haven't seen an example of that and are unsure that really is the case.
The example Stefan gave [1] is just about removing the boilerplate of the null check + throwing a runtime exception, which could be handled using a static utility method (adaptOrThrow, but outside the adaptable interface). Also, this seems to be heavily depending on the style of the application code, whether exceptions are used a lot or null handling (using optionals etc.) is prefered and done consistently. Hence I am not sure if that requires a major change in the adaptable interface contract.
[1] https://issues.apache.org/jira/browse/SLING-3714?focusedCommentId=14048040&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14048040
Cheers,
Alex
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
I quickly tried to implement a POC, but due to type erasure the interface is not as simple as just putting RequireAdapter<Foo>.class
I found the following reference: http://gafter.blogspot.de/2006/12/super-type-tokens.html and tried to implement something like that but could not get it to work in a simple fashion.
@Bertrand: Do you have an example in mind on how to get the wrapped type of RequireAdapter?
Thanks,
Konrad
On 01 Jul 2014, at 12:09, Konrad Windszus <ko...@gmx.de> wrote:
>
> On 01 Jul 2014, at 12:05, Stefan Seifert <ss...@pro-vision.de> wrote:
>
>>> Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
>>
>> this would still require an "unwrapping" of the object out of the RequireAdapter<Foo> instance.
> In my regard there is an instanceof RequireAdapter check within the AdapterManagerImpl which would in that case just pass/throw exceptions. So no need to unwrap anything for the client.
> The only questions is how to get the generic type at runtime (within the AdapterManagerImpl), but there are solutions to that as well: http://stackoverflow.com/questions/3403909/get-generic-type-of-class-at-runtime
>
>
>>
>>> Foo f = someObject.adaptTo(RequireAdapter.for(Foo.class));
>>
>> this looks interesting, and does not need unwrapping if the return value is the input class.
>> i assume it could be implemented using a ThreadLocal or similar as well?
>>
>> stefan
>>
>>
>>> -----Original Message-----
>>> From: Konrad Windszus [mailto:konrad_w@gmx.de]
>>> Sent: Tuesday, July 01, 2014 11:58 AM
>>> To: dev@sling.apache.org
>>> Cc: Bertrand Delacretaz
>>> Subject: Re: adaptTo and results ....
>>>
>>> I like that approach. It is backwards-compatible and allows the developers to
>>> decide whether they want to check for null or to rely on exceptions.
>>> The AdapterManagerImpl indeed would need to deal with such a parametrisation
>>> and in addition the javadocs would need to be adjusted to make it clear that
>>> AdapterFactories may throw RuntimeExceptions.
>>> Those exceptions should be caught by the AdapterManagerImpl when the
>>> RequireAdapter was not requested and in the other case just passed along.
>>>
>>>
>>> On 01 Jul 2014, at 09:44, Bertrand Delacretaz <bd...@apache.org> wrote:
>>>
>>>> On Tue, Jul 1, 2014 at 9:41 AM, Bertrand Delacretaz
>>>> <bd...@apache.org> wrote:
>>>>> ...how about this:
>>>>>
>>>>> Foo f = someObject.adaptTo(RequireAdapter.for(Foo.class));
>>>>
>>>> Actually, rereading SLING-3714, this can be made simpler with generics
>>>>
>>>> Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
>>>>
>>>> where RequireAdapter causes AdapterManagerImpl to wrap the adapters to
>>>> throw an exception if adaption returns null.
>>>>
>>>> -Bertrand
>>
>
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
On 01 Jul 2014, at 12:05, Stefan Seifert <ss...@pro-vision.de> wrote:
>> Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
>
> this would still require an "unwrapping" of the object out of the RequireAdapter<Foo> instance.
In my regard there is an instanceof RequireAdapter check within the AdapterManagerImpl which would in that case just pass/throw exceptions. So no need to unwrap anything for the client.
The only questions is how to get the generic type at runtime (within the AdapterManagerImpl), but there are solutions to that as well: http://stackoverflow.com/questions/3403909/get-generic-type-of-class-at-runtime
>
>> Foo f = someObject.adaptTo(RequireAdapter.for(Foo.class));
>
> this looks interesting, and does not need unwrapping if the return value is the input class.
> i assume it could be implemented using a ThreadLocal or similar as well?
>
> stefan
>
>
>> -----Original Message-----
>> From: Konrad Windszus [mailto:konrad_w@gmx.de]
>> Sent: Tuesday, July 01, 2014 11:58 AM
>> To: dev@sling.apache.org
>> Cc: Bertrand Delacretaz
>> Subject: Re: adaptTo and results ....
>>
>> I like that approach. It is backwards-compatible and allows the developers to
>> decide whether they want to check for null or to rely on exceptions.
>> The AdapterManagerImpl indeed would need to deal with such a parametrisation
>> and in addition the javadocs would need to be adjusted to make it clear that
>> AdapterFactories may throw RuntimeExceptions.
>> Those exceptions should be caught by the AdapterManagerImpl when the
>> RequireAdapter was not requested and in the other case just passed along.
>>
>>
>> On 01 Jul 2014, at 09:44, Bertrand Delacretaz <bd...@apache.org> wrote:
>>
>>> On Tue, Jul 1, 2014 at 9:41 AM, Bertrand Delacretaz
>>> <bd...@apache.org> wrote:
>>>> ...how about this:
>>>>
>>>> Foo f = someObject.adaptTo(RequireAdapter.for(Foo.class));
>>>
>>> Actually, rereading SLING-3714, this can be made simpler with generics
>>>
>>> Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
>>>
>>> where RequireAdapter causes AdapterManagerImpl to wrap the adapters to
>>> throw an exception if adaption returns null.
>>>
>>> -Bertrand
>
RE: adaptTo and results ....
Posted by Stefan Seifert <ss...@pro-vision.de>.
> Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
this would still require an "unwrapping" of the object out of the RequireAdapter<Foo> instance.
> Foo f = someObject.adaptTo(RequireAdapter.for(Foo.class));
this looks interesting, and does not need unwrapping if the return value is the input class.
i assume it could be implemented using a ThreadLocal or similar as well?
stefan
>-----Original Message-----
>From: Konrad Windszus [mailto:konrad_w@gmx.de]
>Sent: Tuesday, July 01, 2014 11:58 AM
>To: dev@sling.apache.org
>Cc: Bertrand Delacretaz
>Subject: Re: adaptTo and results ....
>
>I like that approach. It is backwards-compatible and allows the developers to
>decide whether they want to check for null or to rely on exceptions.
>The AdapterManagerImpl indeed would need to deal with such a parametrisation
>and in addition the javadocs would need to be adjusted to make it clear that
>AdapterFactories may throw RuntimeExceptions.
>Those exceptions should be caught by the AdapterManagerImpl when the
>RequireAdapter was not requested and in the other case just passed along.
>
>
>On 01 Jul 2014, at 09:44, Bertrand Delacretaz <bd...@apache.org> wrote:
>
>> On Tue, Jul 1, 2014 at 9:41 AM, Bertrand Delacretaz
>> <bd...@apache.org> wrote:
>>> ...how about this:
>>>
>>> Foo f = someObject.adaptTo(RequireAdapter.for(Foo.class));
>>
>> Actually, rereading SLING-3714, this can be made simpler with generics
>>
>> Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
>>
>> where RequireAdapter causes AdapterManagerImpl to wrap the adapters to
>> throw an exception if adaption returns null.
>>
>> -Bertrand
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
I like that approach. It is backwards-compatible and allows the developers to decide whether they want to check for null or to rely on exceptions.
The AdapterManagerImpl indeed would need to deal with such a parametrisation and in addition the javadocs would need to be adjusted to make it clear that AdapterFactories may throw RuntimeExceptions.
Those exceptions should be caught by the AdapterManagerImpl when the RequireAdapter was not requested and in the other case just passed along.
On 01 Jul 2014, at 09:44, Bertrand Delacretaz <bd...@apache.org> wrote:
> On Tue, Jul 1, 2014 at 9:41 AM, Bertrand Delacretaz
> <bd...@apache.org> wrote:
>> ...how about this:
>>
>> Foo f = someObject.adaptTo(RequireAdapter.for(Foo.class));
>
> Actually, rereading SLING-3714, this can be made simpler with generics
>
> Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
>
> where RequireAdapter causes AdapterManagerImpl to wrap the adapters to
> throw an exception if adaption returns null.
>
> -Bertrand
Re: adaptTo and results ....
Posted by Alexander Klimetschek <ak...@adobe.com>.
I am still missing a use case that validates such changes... adaptTo is already a slightly hacky/magic approach, we should not introduce more magic :)
Cheers,
Alex
On 28.07.2014, at 21:49, Marius Petria <mp...@adobe.com> wrote:
>
> Hi,
>
> I just read this thread and it might be that I do not understand all the
> reasons behind surfacing exceptions through adaptTo. However, I wanted to
> share with you a variation of Bertrand¹s initial proposal which allows the
> consumer of the API to explicitly require an adaptation that throws
> exceptions.
>
> Instead of
>
> * Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
>
> One can use a double adaption
>
> * StrictAdaptable strictAdaptable =
> someObject.adaptTo(StrictAdaptable.class));
> * Foo f = strictAdaptable.adaptTo(Foo.class));
>
> where StrictAdaptable is just like Adaptable but in addition it specifies
> that adaptTo never returns null and can throw RuntimeExceptions.
>
> The boiler plate of ³double adaption² can be extracted in an adaptOrThrow
> util.
> In case of validation a more specific interface (ValidationAdaptable) can
> be used, for which the contract of adaptTo specifies that it throws
> ValidationException.
>
> WDYT?
>
> Marius
>
>
>
>
>
>
>
>
>
>
>
> On 7/3/14, 12:06 PM, "Bertrand Delacretaz" <bd...@apache.org> wrote:
>
>> Hi,
>>
>> On Tue, Jul 1, 2014 at 5:16 PM, Felix Meschberger <fm...@adobe.com>
>> wrote:
>>> Am 01.07.2014 um 09:44 schrieb Bertrand Delacretaz
>>> <bd...@apache.org>:
>>> ...Unfortunately, I don't think this works, because the adaptTo
>>> signature is:
>>>
>>> public <AdapterType> AdapterType adaptTo(Class<AdapterType> type);
>>>
>>> Hence the return type is the same as provided as the argument, that is
>>> if you pass
>>> RequireAdapter<Foo>, you get a RequireAdapter<Foo> object and not a Foo
>>> object...
>>
>> You're right, I overlooked that.
>>
>> The someObject.adaptTo(RequireAdapter.for(Foo.class)) variant should
>> work if for(...) returns Foo.class but uses a ThreadLocal to tell the
>> AdapterManagerImpl about the options. Not sure if it's worth the
>> effort or if a new API is better, we could implement a bridge between
>> the new and old API to avoid having to change all existing adapters.
>>
>> I see that the use case is under discussion anyway, so let's see what
>> comes out of that...
>>
>> -Bertrand
>
Re: adaptTo and results ....
Posted by Marius Petria <mp...@adobe.com>.
Hi,
I just read this thread and it might be that I do not understand all the
reasons behind surfacing exceptions through adaptTo. However, I wanted to
share with you a variation of Bertrand¹s initial proposal which allows the
consumer of the API to explicitly require an adaptation that throws
exceptions.
Instead of
* Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
One can use a double adaption
* StrictAdaptable strictAdaptable =
someObject.adaptTo(StrictAdaptable.class));
* Foo f = strictAdaptable.adaptTo(Foo.class));
where StrictAdaptable is just like Adaptable but in addition it specifies
that adaptTo never returns null and can throw RuntimeExceptions.
The boiler plate of ³double adaption² can be extracted in an adaptOrThrow
util.
In case of validation a more specific interface (ValidationAdaptable) can
be used, for which the contract of adaptTo specifies that it throws
ValidationException.
WDYT?
Marius
On 7/3/14, 12:06 PM, "Bertrand Delacretaz" <bd...@apache.org> wrote:
>Hi,
>
>On Tue, Jul 1, 2014 at 5:16 PM, Felix Meschberger <fm...@adobe.com>
>wrote:
>> Am 01.07.2014 um 09:44 schrieb Bertrand Delacretaz
>><bd...@apache.org>:
>> ...Unfortunately, I don't think this works, because the adaptTo
>>signature is:
>>
>> public <AdapterType> AdapterType adaptTo(Class<AdapterType> type);
>>
>> Hence the return type is the same as provided as the argument, that is
>>if you pass
>> RequireAdapter<Foo>, you get a RequireAdapter<Foo> object and not a Foo
>>object...
>
>You're right, I overlooked that.
>
>The someObject.adaptTo(RequireAdapter.for(Foo.class)) variant should
>work if for(...) returns Foo.class but uses a ThreadLocal to tell the
>AdapterManagerImpl about the options. Not sure if it's worth the
>effort or if a new API is better, we could implement a bridge between
>the new and old API to avoid having to change all existing adapters.
>
>I see that the use case is under discussion anyway, so let's see what
>comes out of that...
>
>-Bertrand
Re: adaptTo and results ....
Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi,
On Tue, Jul 1, 2014 at 5:16 PM, Felix Meschberger <fm...@adobe.com> wrote:
> Am 01.07.2014 um 09:44 schrieb Bertrand Delacretaz <bd...@apache.org>:
> ...Unfortunately, I don't think this works, because the adaptTo signature is:
>
> public <AdapterType> AdapterType adaptTo(Class<AdapterType> type);
>
> Hence the return type is the same as provided as the argument, that is if you pass
> RequireAdapter<Foo>, you get a RequireAdapter<Foo> object and not a Foo object...
You're right, I overlooked that.
The someObject.adaptTo(RequireAdapter.for(Foo.class)) variant should
work if for(...) returns Foo.class but uses a ThreadLocal to tell the
AdapterManagerImpl about the options. Not sure if it's worth the
effort or if a new API is better, we could implement a bridge between
the new and old API to avoid having to change all existing adapters.
I see that the use case is under discussion anyway, so let's see what
comes out of that...
-Bertrand
Re: adaptTo and results ....
Posted by Felix Meschberger <fm...@adobe.com>.
Hi
Am 01.07.2014 um 09:44 schrieb Bertrand Delacretaz <bd...@apache.org>:
> On Tue, Jul 1, 2014 at 9:41 AM, Bertrand Delacretaz
> <bd...@apache.org> wrote:
>> ...how about this:
>>
>> Foo f = someObject.adaptTo(RequireAdapter.for(Foo.class));
>
> Actually, rereading SLING-3714, this can be made simpler with generics
>
> Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
>
> where RequireAdapter causes AdapterManagerImpl to wrap the adapters to
> throw an exception if adaption returns null.
Unfortunately, I don't think this works, because the adaptTo signature is:
public <AdapterType> AdapterType adaptTo(Class<AdapterType> type);
Hence the return type is the same as provided as the argument, that is if you pass RequireAdapter<Foo>, you get a RequireAdapter<Foo> object and not a Foo object.
Regards
Felix
>
> -Bertrand
Re: adaptTo and results ....
Posted by Bertrand Delacretaz <bd...@apache.org>.
On Tue, Jul 1, 2014 at 9:41 AM, Bertrand Delacretaz
<bd...@apache.org> wrote:
> ...how about this:
>
> Foo f = someObject.adaptTo(RequireAdapter.for(Foo.class));
Actually, rereading SLING-3714, this can be made simpler with generics
Foo f = someObject.adaptTo(RequireAdapter<Foo>.class));
where RequireAdapter causes AdapterManagerImpl to wrap the adapters to
throw an exception if adaption returns null.
-Bertrand
Re: adaptTo and results ....
Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi,
On Tue, Jul 1, 2014 at 9:07 AM, Felix Meschberger <fm...@adobe.com> wrote:
> ..there are options available...
Just a wild idea, how about this:
Foo f = someObject.adaptTo(RequireAdapter.for(Foo.class));
which could be handled by the AdapterManagerImpl, by wrapping whatever
adapter it finds so that a null result throws a CannotAdaptException?
Needs some magic so that RequireAdapter.for manufactures a class that
triggers the AdapterManagerImpl wrapping, but that should be doable
with proxies or bytecode manipulation, and that magic is just between
RequireAdapter and AdapterManagerImpl, so doesn't leak everywhere.
-Bertrand
RE: adaptTo and results ....
Posted by Stefan Seifert <ss...@pro-vision.de>.
the NPE would swallow all maybe usefull excpetion information, that might be contained in the root cause of the exception throws by a method like adaptToOrThrow method. always logging the exception internally by the adapter manager has the drawback that the application might not be interested in the failure and does not want to log it. the decision whether a adaption failure is relevant or not should be taken by the application.
I'm not convinced that a new interface and a adaptToOrThrow is the best solution either - but lets start to convince ourselves that it is a relevant usecase to have (optional, but with full error information) exception handling on an adaptTo call, whatever solution we find to add this without a big mess in the interface design. bertrand opened an interesting discussion on alternatives.
stefan
>-----Original Message-----
>From: Carsten Ziegeler [mailto:cziegeler@apache.org]
>Sent: Tuesday, July 01, 2014 12:21 PM
>To: dev@sling.apache.org
>Subject: Re: adaptTo and results ....
>
>Yes, but right now you would get an NPE accessing the object - so you
>already have a runtime exception and don't need to check for null (I'm not
>arguing that this is a good way, I'm just trying to avoid heavy changes).
>And we could change the adapter manager/factory implemntation to log the
>exceptions (if they're not doing it already)
>
>Carsten
>
>
>2014-07-01 12:17 GMT+02:00 Stefan Seifert <ss...@pro-vision.de>:
>
>> example: usecase like here
>>
>> https://issues.apache.org/jira/browse/SLING-
>3714?focusedCommentId=14048040#comment-14048040
>>
>> the caller code expects that the adaption is always successful if
>> everything works correct - if not it is an application error which should
>> be propagated through error handling and result in an error log message.
>>
>> stefan
>>
>>
>> >-----Original Message-----
>> >From: Carsten Ziegeler [mailto:cziegeler@apache.org]
>> >Sent: Tuesday, July 01, 2014 12:14 PM
>> >To: dev@sling.apache.org
>> >Subject: Re: adaptTo and results ....
>> >
>> >So if your adapter is buggy and you get an exception, what do you do with
>> >it?
>> >
>> >Carsten
>> >
>> >
>> >2014-07-01 12:08 GMT+02:00 Jeff Young <je...@adobe.com>:
>> >
>> >> Hi Carsten,
>> >>
>> >> Sure, but Konrad has a point in that I think sometimes the client *does*
>> >> care why the adaption failed. For instance, if it had to do with
>> >> something entirely different from whether or not adaption would normally
>> >> work.
>> >>
>> >> Let's say that I have a resource that should adapt to XYZ, but that my
>> >> adapter is currently buggy. I'd like to get an exception for that, but
>> >> said exception is going to get eaten.
>> >>
>> >> I do agree that if I have a resource that should NOT adapt to XYZ, that
>> >> getting back null is fine, and that I don't care why the adaption
>> failed.
>> >>
>> >> Cheers,
>> >> Jeff.
>> >>
>> >>
>> >> On 01/07/2014 10:19, "Carsten Ziegeler" <cz...@apache.org> wrote:
>> >>
>> >> >Sure :) For the adapter pattern, the client does not care why the
>> adaption
>> >> >failed, the client is just interested in the result (success or not)
>> >> >Validation is a different beast, if validation fails you want to know
>> >> >specific reasons why it failed - and this can be multiple.
>> >> >I tried to explain in my first mail on this thread, that all other use
>> >> >cases mentioned can be handled with the current implementation - with
>> the
>> >> >exception of validation. But I think validation requires a different
>> >> >concept than the adapter pattern.
>> >> >
>> >> >Carsten
>> >> >
>> >> >
>> >> >2014-07-01 11:09 GMT+02:00 Jeff Young <je...@adobe.com>:
>> >> >
>> >> >> Hi Carsten,
>> >> >>
>> >> >> Can you say more? (I'm not sure I understand what you're getting
>> >> >>at....)
>> >> >>
>> >> >> Thanks,
>> >> >> Jeff.
>> >> >>
>> >> >>
>> >> >> On 01/07/2014 09:56, "Carsten Ziegeler" <cz...@apache.org>
>> wrote:
>> >> >>
>> >> >> >adaption and validation are different concerns
>> >> >> >
>> >> >> >Carsten
>> >> >> >
>> >> >> >
>> >> >> >2014-07-01 10:55 GMT+02:00 Jeff Young <je...@adobe.com>:
>> >> >> >
>> >> >> >> We could solve that by defining a specific exception for
>> >> >> >> adaptation-not-possible and then catch only that.
>> >> >> >>
>> >> >> >> Of course that would leak tons of exceptions from code written
>> before
>> >> >> >>that
>> >> >> >> exception became available. Maybe do the catching based on some
>> >> >>sort of
>> >> >> >> version clue?
>> >> >> >>
>> >> >> >> Cheers,
>> >> >> >> Jeff.
>> >> >> >>
>> >> >> >>
>> >> >> >> On 01/07/2014 09:40, "Konrad Windszus" <ko...@gmx.de> wrote:
>> >> >> >>
>> >> >> >> >It is not (only) about throwing exceptions in case no suitable
>> >> >>adapter
>> >> >> >>is
>> >> >> >> >available. It rather is about the fact, that today the adaptTo
>> is a
>> >> >> >> >barrier for all kinds of exceptions. In some cases the adaptation
>> >> >>fails
>> >> >> >> >for a specific reason (one example is Sling Models where
>> injection
>> >> >> >>fails,
>> >> >> >> >another one is the issue mentioned in
>> >> >> >> >https://issues.apache.org/jira/browse/SLING-2712 (ValueMap not
>> >> >> >>supporting
>> >> >> >> >primitives)). Both would be valid use cases where the client
>> would
>> >> >>be
>> >> >> >> >interested to catch the exception here.
>> >> >> >> >
>> >> >> >> >On 01 Jul 2014, at 10:34, Carsten Ziegeler <cziegeler@apache.org
>> >
>> >> >> >>wrote:
>> >> >> >> >
>> >> >> >> >> Adding a new interface would require us to implement it all
>> over
>> >> >>the
>> >> >> >> >>place
>> >> >> >> >> and as Felix points out, client code would always need to check
>> >> >> >>whether
>> >> >> >> >>the
>> >> >> >> >> new interface is implemented or not Having to methods, like
>> >> >> >>hasAdapter
>> >> >> >> >>and
>> >> >> >> >> adaptOrThrow does not work very well as between the two calls
>> >> >>things
>> >> >> >> >>might
>> >> >> >> >> have changed already: while hasAdapter returns true, the
>> >> >>underlying
>> >> >> >> >>factory
>> >> >> >> >> gets unregistered before adaptOrThrow is called.
>> >> >> >> >> In many use cases, the current pattern works fine - the client
>> >> >>does
>> >> >> >>not
>> >> >> >> >> care whether an exception is thrown within the adaption - it
>> just
>> >> >> >>cares
>> >> >> >> >> whether an object is returned or not. And there are valid use
>> >> >>cases,
>> >> >> >> >>where
>> >> >> >> >> client code does different things whether the adaption works or
>> >> >>not
>> >> >> >> >>(e.g.
>> >> >> >> >> the post servlet checks for adaptTo(Node) and then does
>> additional
>> >> >> >> >>things
>> >> >> >> >> if the resource is backed up by a node.)
>> >> >> >> >>
>> >> >> >> >> I see the point that there are also use cases where it would be
>> >> >>fine
>> >> >> >>to
>> >> >> >> >> simpy throw an exception if adaptTo is not successful. This
>> would
>> >> >> >>make
>> >> >> >> >>the
>> >> >> >> >> client code easier. However as this most properly is a runtime
>> >> >> >> >>exception,
>> >> >> >> >> client code can today just call a method on the object and end
>> up
>> >> >> >>with a
>> >> >> >> >> NPE - having the same result :)
>> >> >> >> >>
>> >> >> >> >> This leaves us with use cases where the client code explicitely
>> >> >> >>wants to
>> >> >> >> >> catch the exception and then do something depending on the
>> >> >>exception.
>> >> >> >> >>Maybe
>> >> >> >> >> we should just add something for this explicit use case
>> instead of
>> >> >> >> >>bloating
>> >> >> >> >> the general adaptTo mechanism?
>> >> >> >> >>
>> >> >> >> >> Regards
>> >> >> >> >> Carsten
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> 2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
>> >> >> >> >>
>> >> >> >> >>> Regarding 1) Having such a Result class would mean that all
>> >> >>consumer
>> >> >> >> >>>would
>> >> >> >> >>> need to unwrap the exception first. So instead of being
>> forced of
>> >> >> >> >>> implementing a null-check (as with the old solution) one would
>> >> >>need
>> >> >> >>to
>> >> >> >> >>> implement another check. I want to prevent such a burden to
>> the
>> >> >> >> >>>consumers.
>> >> >> >> >>> Regarding 2) Since the client code knows on which object the
>> >> >> >> >>> adaptToOrThrow is called I don¹t get why an instanceof check
>> is
>> >> >> >> >>>necessary.
>> >> >> >> >>> Whether this object implements Adaptable2 is known at
>> >> >>compile-time,
>> >> >> >>so
>> >> >> >> >>>you
>> >> >> >> >>> do have the full IDE-support here.
>> >> >> >> >>> Regarding 3) In that case I would no longer allow a null value
>> >> >>to be
>> >> >> >> >>> returned. One drawback is, that all the null checks are no
>> longer
>> >> >> >> >>>effective.
>> >> >> >> >>>
>> >> >> >> >>> IMHO solution 2) is the best. At the same time I would
>> deprecate
>> >> >>the
>> >> >> >> >>>old
>> >> >> >> >>> Adaptable, because I cannot come up with a real use-case where
>> >> >> >> >>>returning
>> >> >> >> >>> has an advantage. I would rather implement another method
>> boolean
>> >> >> >> >>> hasAdapter(Class<AdapterType> type) on the Adaptable2
>> interface.
>> >> >> >> >>> Regards,
>> >> >> >> >>> Konrad
>> >> >> >> >>>
>> >> >> >> >>> On 01 Jul 2014, at 09:07, Felix Meschberger <
>> fmeschbe@adobe.com>
>> >> >> >> wrote:
>> >> >> >> >>>
>> >> >> >> >>>> Hi
>> >> >> >> >>>>
>> >> >> >> >>>> There currently are two issues floating around dealing with
>> the
>> >> >> >> >>>>question
>> >> >> >> >>> of returning more information than just null from the
>> >> >> >> >>> Adaptable.adaptTo(Class) method:
>> >> >> >> >>> https://issues.apache.org/jira/browse/SLING-3714 and
>> >> >> >> >>> https://issues.apache.org/jira/browse/SLING-3709. I think
>> these
>> >> >> >> >>>requests
>> >> >> >> >>> warrant some discussion on the list.
>> >> >> >> >>>>
>> >> >> >> >>>> Background: adaptTo can be implemented by Adaptable
>> >> >>implementations
>> >> >> >> >>> themselves or, by extending from SlingAdaptable, they may
>> defer
>> >> >>to
>> >> >> >>an
>> >> >> >> >>> AdapterMananager. AdapterManager itself is extended by
>> >> >> >>AdapterFactory
>> >> >> >> >>> services. All these interfaces define an adaptTo method. All
>> >> >>these
>> >> >> >> >>>methods
>> >> >> >> >>> return null if adaption is not possible and don't declare or
>> >> >> >>document
>> >> >> >> >>>to
>> >> >> >> >>> throw an exception.
>> >> >> >> >>>>
>> >> >> >> >>>> While not explicitly documented as such, the intention is and
>> >> >>was
>> >> >> >>that
>> >> >> >> >>> adaptTo never throws on the grounds that adaption may fail
>> which
>> >> >>is
>> >> >> >> >>> considered a valid result and thus exceptions are not to be
>> >> >>expected
>> >> >> >> >>>and
>> >> >> >> >>> handled.
>> >> >> >> >>>>
>> >> >> >> >>>> Hence all implementations of the methods generally
>> >> >> >> >>> catch-and-log-but-don't-throw. Interestingly
>> >> >>SlingAdaptable.adaptTo
>> >> >> >>and
>> >> >> >> >>> AdapterManagerImpl.getAdapter don't catch ‹ so any
>> >> >>RuntimeException
>> >> >> >> >>>thrown
>> >> >> >> >>> from an AdapterFactory would be forwarded.
>> >> >> >> >>>>
>> >> >> >> >>>> Having said this there are options available:
>> >> >> >> >>>>
>> >> >> >> >>>> (1) Add support for a new Result<?> class. We would probably
>> >> >> >>implement
>> >> >> >> >>> this in the AdapterManager.getAdapter implementation
>> explicitly
>> >> >> >> >>>handling
>> >> >> >> >>> this case because it would entail catching the
>> adaptTo/getAdapter
>> >> >> >> >>>calls to
>> >> >> >> >>> get the exception (the Result.getError should return Throwable
>> >> >> >> >>>probably not
>> >> >> >> >>> Error)
>> >> >> >> >>>>
>> >> >> >> >>>> Use would be limited to new AdapterFactory implementations
>> >> >>throwing
>> >> >> >> >>> RuntimeExcpetion. For Sling Models this would be the case.
>> >> >> >> >>>>
>> >> >> >> >>>> (2) Add a new adaptToOrThrow method, which is declared to
>> throw
>> >> >>a
>> >> >> >> >>> RuntimeException and never return null: Either it can adapt
>> or it
>> >> >> >> >>>throws.
>> >> >> >> >>> This would require a new interface Adaptable2 (probably) to
>> not
>> >> >> >>break
>> >> >> >> >>> existing Adaptable implementations. The SlingAdaptable base
>> class
>> >> >> >>would
>> >> >> >> >>> implement the new method of course, probably something like
>> this:
>> >> >> >> >>>>
>> >> >> >> >>>>> SlingAdaptable implements Adaptable2 {
>> >> >> >> >>>>> Š
>> >> >> >> >>>>> public <AdapterType> AdapterType
>> >> >>adaptToOrThrow(Class<AdapterType>
>> >> >> >> >>> type) {
>> >> >> >> >>>>> AdapterType result = this.adaptTo(type);
>> >> >> >> >>>>> if (result != null) {
>> >> >> >> >>>>> return result;
>> >> >> >> >>>>> }
>> >> >> >> >>>>> throw new CannotAdaptException(Š);
>> >> >> >> >>>>> }
>> >> >> >> >>>>> }
>> >> >> >> >>>>>
>> >> >> >> >>>>
>> >> >> >> >>>> Use is problematic because you would have to know whether you
>> >> >>can
>> >> >> >>call
>> >> >> >> >>> the new method: So instead of an null check you now have an
>> >> >> >>instanceof
>> >> >> >> >>> check Š Except for the Resource interface which would be
>> >> >>extended to
>> >> >> >> >>>extend
>> >> >> >> >>> from Adaptable2 as well.
>> >> >> >> >>>>
>> >> >> >> >>>> (3) Document, that Adaptable.adaptTo may throw a
>> >> >>RuntimeException.
>> >> >> >> >>>>
>> >> >> >> >>>> The problem here is, that this may conceptually break
>> existing
>> >> >> >>callers
>> >> >> >> >>> of Adaptable.adaptTo which don't expect an exception at all ‹
>> >> >> >> >>>presumably
>> >> >> >> >>> this is a minor nuisance because technically a
>> RuntimeException
>> >> >>may
>> >> >> >> >>>always
>> >> >> >> >>> be thrown.
>> >> >> >> >>>>
>> >> >> >> >>>> Regards
>> >> >> >> >>>> Felix
>> >> >> >> >>>
>> >> >> >> >>>
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> --
>> >> >> >> >> Carsten Ziegeler
>> >> >> >> >> Adobe Research Switzerland
>> >> >> >> >> cziegeler@apache.org
>> >> >> >> >
>> >> >> >>
>> >> >> >>
>> >> >> >
>> >> >> >
>> >> >> >--
>> >> >> >Carsten Ziegeler
>> >> >> >Adobe Research Switzerland
>> >> >> >cziegeler@apache.org
>> >> >>
>> >> >>
>> >> >
>> >> >
>> >> >--
>> >> >Carsten Ziegeler
>> >> >Adobe Research Switzerland
>> >> >cziegeler@apache.org
>> >>
>> >>
>> >
>> >
>> >--
>> >Carsten Ziegeler
>> >Adobe Research Switzerland
>> >cziegeler@apache.org
>>
>
>
>
>--
>Carsten Ziegeler
>Adobe Research Switzerland
>cziegeler@apache.org
Re: adaptTo and results ....
Posted by Carsten Ziegeler <cz...@apache.org>.
Yes, but right now you would get an NPE accessing the object - so you
already have a runtime exception and don't need to check for null (I'm not
arguing that this is a good way, I'm just trying to avoid heavy changes).
And we could change the adapter manager/factory implemntation to log the
exceptions (if they're not doing it already)
Carsten
2014-07-01 12:17 GMT+02:00 Stefan Seifert <ss...@pro-vision.de>:
> example: usecase like here
>
> https://issues.apache.org/jira/browse/SLING-3714?focusedCommentId=14048040#comment-14048040
>
> the caller code expects that the adaption is always successful if
> everything works correct - if not it is an application error which should
> be propagated through error handling and result in an error log message.
>
> stefan
>
>
> >-----Original Message-----
> >From: Carsten Ziegeler [mailto:cziegeler@apache.org]
> >Sent: Tuesday, July 01, 2014 12:14 PM
> >To: dev@sling.apache.org
> >Subject: Re: adaptTo and results ....
> >
> >So if your adapter is buggy and you get an exception, what do you do with
> >it?
> >
> >Carsten
> >
> >
> >2014-07-01 12:08 GMT+02:00 Jeff Young <je...@adobe.com>:
> >
> >> Hi Carsten,
> >>
> >> Sure, but Konrad has a point in that I think sometimes the client *does*
> >> care why the adaption failed. For instance, if it had to do with
> >> something entirely different from whether or not adaption would normally
> >> work.
> >>
> >> Let's say that I have a resource that should adapt to XYZ, but that my
> >> adapter is currently buggy. I'd like to get an exception for that, but
> >> said exception is going to get eaten.
> >>
> >> I do agree that if I have a resource that should NOT adapt to XYZ, that
> >> getting back null is fine, and that I don't care why the adaption
> failed.
> >>
> >> Cheers,
> >> Jeff.
> >>
> >>
> >> On 01/07/2014 10:19, "Carsten Ziegeler" <cz...@apache.org> wrote:
> >>
> >> >Sure :) For the adapter pattern, the client does not care why the
> adaption
> >> >failed, the client is just interested in the result (success or not)
> >> >Validation is a different beast, if validation fails you want to know
> >> >specific reasons why it failed - and this can be multiple.
> >> >I tried to explain in my first mail on this thread, that all other use
> >> >cases mentioned can be handled with the current implementation - with
> the
> >> >exception of validation. But I think validation requires a different
> >> >concept than the adapter pattern.
> >> >
> >> >Carsten
> >> >
> >> >
> >> >2014-07-01 11:09 GMT+02:00 Jeff Young <je...@adobe.com>:
> >> >
> >> >> Hi Carsten,
> >> >>
> >> >> Can you say more? (I'm not sure I understand what you're getting
> >> >>at....)
> >> >>
> >> >> Thanks,
> >> >> Jeff.
> >> >>
> >> >>
> >> >> On 01/07/2014 09:56, "Carsten Ziegeler" <cz...@apache.org>
> wrote:
> >> >>
> >> >> >adaption and validation are different concerns
> >> >> >
> >> >> >Carsten
> >> >> >
> >> >> >
> >> >> >2014-07-01 10:55 GMT+02:00 Jeff Young <je...@adobe.com>:
> >> >> >
> >> >> >> We could solve that by defining a specific exception for
> >> >> >> adaptation-not-possible and then catch only that.
> >> >> >>
> >> >> >> Of course that would leak tons of exceptions from code written
> before
> >> >> >>that
> >> >> >> exception became available. Maybe do the catching based on some
> >> >>sort of
> >> >> >> version clue?
> >> >> >>
> >> >> >> Cheers,
> >> >> >> Jeff.
> >> >> >>
> >> >> >>
> >> >> >> On 01/07/2014 09:40, "Konrad Windszus" <ko...@gmx.de> wrote:
> >> >> >>
> >> >> >> >It is not (only) about throwing exceptions in case no suitable
> >> >>adapter
> >> >> >>is
> >> >> >> >available. It rather is about the fact, that today the adaptTo
> is a
> >> >> >> >barrier for all kinds of exceptions. In some cases the adaptation
> >> >>fails
> >> >> >> >for a specific reason (one example is Sling Models where
> injection
> >> >> >>fails,
> >> >> >> >another one is the issue mentioned in
> >> >> >> >https://issues.apache.org/jira/browse/SLING-2712 (ValueMap not
> >> >> >>supporting
> >> >> >> >primitives)). Both would be valid use cases where the client
> would
> >> >>be
> >> >> >> >interested to catch the exception here.
> >> >> >> >
> >> >> >> >On 01 Jul 2014, at 10:34, Carsten Ziegeler <cziegeler@apache.org
> >
> >> >> >>wrote:
> >> >> >> >
> >> >> >> >> Adding a new interface would require us to implement it all
> over
> >> >>the
> >> >> >> >>place
> >> >> >> >> and as Felix points out, client code would always need to check
> >> >> >>whether
> >> >> >> >>the
> >> >> >> >> new interface is implemented or not Having to methods, like
> >> >> >>hasAdapter
> >> >> >> >>and
> >> >> >> >> adaptOrThrow does not work very well as between the two calls
> >> >>things
> >> >> >> >>might
> >> >> >> >> have changed already: while hasAdapter returns true, the
> >> >>underlying
> >> >> >> >>factory
> >> >> >> >> gets unregistered before adaptOrThrow is called.
> >> >> >> >> In many use cases, the current pattern works fine - the client
> >> >>does
> >> >> >>not
> >> >> >> >> care whether an exception is thrown within the adaption - it
> just
> >> >> >>cares
> >> >> >> >> whether an object is returned or not. And there are valid use
> >> >>cases,
> >> >> >> >>where
> >> >> >> >> client code does different things whether the adaption works or
> >> >>not
> >> >> >> >>(e.g.
> >> >> >> >> the post servlet checks for adaptTo(Node) and then does
> additional
> >> >> >> >>things
> >> >> >> >> if the resource is backed up by a node.)
> >> >> >> >>
> >> >> >> >> I see the point that there are also use cases where it would be
> >> >>fine
> >> >> >>to
> >> >> >> >> simpy throw an exception if adaptTo is not successful. This
> would
> >> >> >>make
> >> >> >> >>the
> >> >> >> >> client code easier. However as this most properly is a runtime
> >> >> >> >>exception,
> >> >> >> >> client code can today just call a method on the object and end
> up
> >> >> >>with a
> >> >> >> >> NPE - having the same result :)
> >> >> >> >>
> >> >> >> >> This leaves us with use cases where the client code explicitely
> >> >> >>wants to
> >> >> >> >> catch the exception and then do something depending on the
> >> >>exception.
> >> >> >> >>Maybe
> >> >> >> >> we should just add something for this explicit use case
> instead of
> >> >> >> >>bloating
> >> >> >> >> the general adaptTo mechanism?
> >> >> >> >>
> >> >> >> >> Regards
> >> >> >> >> Carsten
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> 2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
> >> >> >> >>
> >> >> >> >>> Regarding 1) Having such a Result class would mean that all
> >> >>consumer
> >> >> >> >>>would
> >> >> >> >>> need to unwrap the exception first. So instead of being
> forced of
> >> >> >> >>> implementing a null-check (as with the old solution) one would
> >> >>need
> >> >> >>to
> >> >> >> >>> implement another check. I want to prevent such a burden to
> the
> >> >> >> >>>consumers.
> >> >> >> >>> Regarding 2) Since the client code knows on which object the
> >> >> >> >>> adaptToOrThrow is called I don¹t get why an instanceof check
> is
> >> >> >> >>>necessary.
> >> >> >> >>> Whether this object implements Adaptable2 is known at
> >> >>compile-time,
> >> >> >>so
> >> >> >> >>>you
> >> >> >> >>> do have the full IDE-support here.
> >> >> >> >>> Regarding 3) In that case I would no longer allow a null value
> >> >>to be
> >> >> >> >>> returned. One drawback is, that all the null checks are no
> longer
> >> >> >> >>>effective.
> >> >> >> >>>
> >> >> >> >>> IMHO solution 2) is the best. At the same time I would
> deprecate
> >> >>the
> >> >> >> >>>old
> >> >> >> >>> Adaptable, because I cannot come up with a real use-case where
> >> >> >> >>>returning
> >> >> >> >>> has an advantage. I would rather implement another method
> boolean
> >> >> >> >>> hasAdapter(Class<AdapterType> type) on the Adaptable2
> interface.
> >> >> >> >>> Regards,
> >> >> >> >>> Konrad
> >> >> >> >>>
> >> >> >> >>> On 01 Jul 2014, at 09:07, Felix Meschberger <
> fmeschbe@adobe.com>
> >> >> >> wrote:
> >> >> >> >>>
> >> >> >> >>>> Hi
> >> >> >> >>>>
> >> >> >> >>>> There currently are two issues floating around dealing with
> the
> >> >> >> >>>>question
> >> >> >> >>> of returning more information than just null from the
> >> >> >> >>> Adaptable.adaptTo(Class) method:
> >> >> >> >>> https://issues.apache.org/jira/browse/SLING-3714 and
> >> >> >> >>> https://issues.apache.org/jira/browse/SLING-3709. I think
> these
> >> >> >> >>>requests
> >> >> >> >>> warrant some discussion on the list.
> >> >> >> >>>>
> >> >> >> >>>> Background: adaptTo can be implemented by Adaptable
> >> >>implementations
> >> >> >> >>> themselves or, by extending from SlingAdaptable, they may
> defer
> >> >>to
> >> >> >>an
> >> >> >> >>> AdapterMananager. AdapterManager itself is extended by
> >> >> >>AdapterFactory
> >> >> >> >>> services. All these interfaces define an adaptTo method. All
> >> >>these
> >> >> >> >>>methods
> >> >> >> >>> return null if adaption is not possible and don't declare or
> >> >> >>document
> >> >> >> >>>to
> >> >> >> >>> throw an exception.
> >> >> >> >>>>
> >> >> >> >>>> While not explicitly documented as such, the intention is and
> >> >>was
> >> >> >>that
> >> >> >> >>> adaptTo never throws on the grounds that adaption may fail
> which
> >> >>is
> >> >> >> >>> considered a valid result and thus exceptions are not to be
> >> >>expected
> >> >> >> >>>and
> >> >> >> >>> handled.
> >> >> >> >>>>
> >> >> >> >>>> Hence all implementations of the methods generally
> >> >> >> >>> catch-and-log-but-don't-throw. Interestingly
> >> >>SlingAdaptable.adaptTo
> >> >> >>and
> >> >> >> >>> AdapterManagerImpl.getAdapter don't catch ‹ so any
> >> >>RuntimeException
> >> >> >> >>>thrown
> >> >> >> >>> from an AdapterFactory would be forwarded.
> >> >> >> >>>>
> >> >> >> >>>> Having said this there are options available:
> >> >> >> >>>>
> >> >> >> >>>> (1) Add support for a new Result<?> class. We would probably
> >> >> >>implement
> >> >> >> >>> this in the AdapterManager.getAdapter implementation
> explicitly
> >> >> >> >>>handling
> >> >> >> >>> this case because it would entail catching the
> adaptTo/getAdapter
> >> >> >> >>>calls to
> >> >> >> >>> get the exception (the Result.getError should return Throwable
> >> >> >> >>>probably not
> >> >> >> >>> Error)
> >> >> >> >>>>
> >> >> >> >>>> Use would be limited to new AdapterFactory implementations
> >> >>throwing
> >> >> >> >>> RuntimeExcpetion. For Sling Models this would be the case.
> >> >> >> >>>>
> >> >> >> >>>> (2) Add a new adaptToOrThrow method, which is declared to
> throw
> >> >>a
> >> >> >> >>> RuntimeException and never return null: Either it can adapt
> or it
> >> >> >> >>>throws.
> >> >> >> >>> This would require a new interface Adaptable2 (probably) to
> not
> >> >> >>break
> >> >> >> >>> existing Adaptable implementations. The SlingAdaptable base
> class
> >> >> >>would
> >> >> >> >>> implement the new method of course, probably something like
> this:
> >> >> >> >>>>
> >> >> >> >>>>> SlingAdaptable implements Adaptable2 {
> >> >> >> >>>>> Š
> >> >> >> >>>>> public <AdapterType> AdapterType
> >> >>adaptToOrThrow(Class<AdapterType>
> >> >> >> >>> type) {
> >> >> >> >>>>> AdapterType result = this.adaptTo(type);
> >> >> >> >>>>> if (result != null) {
> >> >> >> >>>>> return result;
> >> >> >> >>>>> }
> >> >> >> >>>>> throw new CannotAdaptException(Š);
> >> >> >> >>>>> }
> >> >> >> >>>>> }
> >> >> >> >>>>>
> >> >> >> >>>>
> >> >> >> >>>> Use is problematic because you would have to know whether you
> >> >>can
> >> >> >>call
> >> >> >> >>> the new method: So instead of an null check you now have an
> >> >> >>instanceof
> >> >> >> >>> check Š Except for the Resource interface which would be
> >> >>extended to
> >> >> >> >>>extend
> >> >> >> >>> from Adaptable2 as well.
> >> >> >> >>>>
> >> >> >> >>>> (3) Document, that Adaptable.adaptTo may throw a
> >> >>RuntimeException.
> >> >> >> >>>>
> >> >> >> >>>> The problem here is, that this may conceptually break
> existing
> >> >> >>callers
> >> >> >> >>> of Adaptable.adaptTo which don't expect an exception at all ‹
> >> >> >> >>>presumably
> >> >> >> >>> this is a minor nuisance because technically a
> RuntimeException
> >> >>may
> >> >> >> >>>always
> >> >> >> >>> be thrown.
> >> >> >> >>>>
> >> >> >> >>>> Regards
> >> >> >> >>>> Felix
> >> >> >> >>>
> >> >> >> >>>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> --
> >> >> >> >> Carsten Ziegeler
> >> >> >> >> Adobe Research Switzerland
> >> >> >> >> cziegeler@apache.org
> >> >> >> >
> >> >> >>
> >> >> >>
> >> >> >
> >> >> >
> >> >> >--
> >> >> >Carsten Ziegeler
> >> >> >Adobe Research Switzerland
> >> >> >cziegeler@apache.org
> >> >>
> >> >>
> >> >
> >> >
> >> >--
> >> >Carsten Ziegeler
> >> >Adobe Research Switzerland
> >> >cziegeler@apache.org
> >>
> >>
> >
> >
> >--
> >Carsten Ziegeler
> >Adobe Research Switzerland
> >cziegeler@apache.org
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
RE: adaptTo and results ....
Posted by Stefan Seifert <ss...@pro-vision.de>.
example: usecase like here
https://issues.apache.org/jira/browse/SLING-3714?focusedCommentId=14048040#comment-14048040
the caller code expects that the adaption is always successful if everything works correct - if not it is an application error which should be propagated through error handling and result in an error log message.
stefan
>-----Original Message-----
>From: Carsten Ziegeler [mailto:cziegeler@apache.org]
>Sent: Tuesday, July 01, 2014 12:14 PM
>To: dev@sling.apache.org
>Subject: Re: adaptTo and results ....
>
>So if your adapter is buggy and you get an exception, what do you do with
>it?
>
>Carsten
>
>
>2014-07-01 12:08 GMT+02:00 Jeff Young <je...@adobe.com>:
>
>> Hi Carsten,
>>
>> Sure, but Konrad has a point in that I think sometimes the client *does*
>> care why the adaption failed. For instance, if it had to do with
>> something entirely different from whether or not adaption would normally
>> work.
>>
>> Let's say that I have a resource that should adapt to XYZ, but that my
>> adapter is currently buggy. I'd like to get an exception for that, but
>> said exception is going to get eaten.
>>
>> I do agree that if I have a resource that should NOT adapt to XYZ, that
>> getting back null is fine, and that I don't care why the adaption failed.
>>
>> Cheers,
>> Jeff.
>>
>>
>> On 01/07/2014 10:19, "Carsten Ziegeler" <cz...@apache.org> wrote:
>>
>> >Sure :) For the adapter pattern, the client does not care why the adaption
>> >failed, the client is just interested in the result (success or not)
>> >Validation is a different beast, if validation fails you want to know
>> >specific reasons why it failed - and this can be multiple.
>> >I tried to explain in my first mail on this thread, that all other use
>> >cases mentioned can be handled with the current implementation - with the
>> >exception of validation. But I think validation requires a different
>> >concept than the adapter pattern.
>> >
>> >Carsten
>> >
>> >
>> >2014-07-01 11:09 GMT+02:00 Jeff Young <je...@adobe.com>:
>> >
>> >> Hi Carsten,
>> >>
>> >> Can you say more? (I'm not sure I understand what you're getting
>> >>at....)
>> >>
>> >> Thanks,
>> >> Jeff.
>> >>
>> >>
>> >> On 01/07/2014 09:56, "Carsten Ziegeler" <cz...@apache.org> wrote:
>> >>
>> >> >adaption and validation are different concerns
>> >> >
>> >> >Carsten
>> >> >
>> >> >
>> >> >2014-07-01 10:55 GMT+02:00 Jeff Young <je...@adobe.com>:
>> >> >
>> >> >> We could solve that by defining a specific exception for
>> >> >> adaptation-not-possible and then catch only that.
>> >> >>
>> >> >> Of course that would leak tons of exceptions from code written before
>> >> >>that
>> >> >> exception became available. Maybe do the catching based on some
>> >>sort of
>> >> >> version clue?
>> >> >>
>> >> >> Cheers,
>> >> >> Jeff.
>> >> >>
>> >> >>
>> >> >> On 01/07/2014 09:40, "Konrad Windszus" <ko...@gmx.de> wrote:
>> >> >>
>> >> >> >It is not (only) about throwing exceptions in case no suitable
>> >>adapter
>> >> >>is
>> >> >> >available. It rather is about the fact, that today the adaptTo is a
>> >> >> >barrier for all kinds of exceptions. In some cases the adaptation
>> >>fails
>> >> >> >for a specific reason (one example is Sling Models where injection
>> >> >>fails,
>> >> >> >another one is the issue mentioned in
>> >> >> >https://issues.apache.org/jira/browse/SLING-2712 (ValueMap not
>> >> >>supporting
>> >> >> >primitives)). Both would be valid use cases where the client would
>> >>be
>> >> >> >interested to catch the exception here.
>> >> >> >
>> >> >> >On 01 Jul 2014, at 10:34, Carsten Ziegeler <cz...@apache.org>
>> >> >>wrote:
>> >> >> >
>> >> >> >> Adding a new interface would require us to implement it all over
>> >>the
>> >> >> >>place
>> >> >> >> and as Felix points out, client code would always need to check
>> >> >>whether
>> >> >> >>the
>> >> >> >> new interface is implemented or not Having to methods, like
>> >> >>hasAdapter
>> >> >> >>and
>> >> >> >> adaptOrThrow does not work very well as between the two calls
>> >>things
>> >> >> >>might
>> >> >> >> have changed already: while hasAdapter returns true, the
>> >>underlying
>> >> >> >>factory
>> >> >> >> gets unregistered before adaptOrThrow is called.
>> >> >> >> In many use cases, the current pattern works fine - the client
>> >>does
>> >> >>not
>> >> >> >> care whether an exception is thrown within the adaption - it just
>> >> >>cares
>> >> >> >> whether an object is returned or not. And there are valid use
>> >>cases,
>> >> >> >>where
>> >> >> >> client code does different things whether the adaption works or
>> >>not
>> >> >> >>(e.g.
>> >> >> >> the post servlet checks for adaptTo(Node) and then does additional
>> >> >> >>things
>> >> >> >> if the resource is backed up by a node.)
>> >> >> >>
>> >> >> >> I see the point that there are also use cases where it would be
>> >>fine
>> >> >>to
>> >> >> >> simpy throw an exception if adaptTo is not successful. This would
>> >> >>make
>> >> >> >>the
>> >> >> >> client code easier. However as this most properly is a runtime
>> >> >> >>exception,
>> >> >> >> client code can today just call a method on the object and end up
>> >> >>with a
>> >> >> >> NPE - having the same result :)
>> >> >> >>
>> >> >> >> This leaves us with use cases where the client code explicitely
>> >> >>wants to
>> >> >> >> catch the exception and then do something depending on the
>> >>exception.
>> >> >> >>Maybe
>> >> >> >> we should just add something for this explicit use case instead of
>> >> >> >>bloating
>> >> >> >> the general adaptTo mechanism?
>> >> >> >>
>> >> >> >> Regards
>> >> >> >> Carsten
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >> 2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
>> >> >> >>
>> >> >> >>> Regarding 1) Having such a Result class would mean that all
>> >>consumer
>> >> >> >>>would
>> >> >> >>> need to unwrap the exception first. So instead of being forced of
>> >> >> >>> implementing a null-check (as with the old solution) one would
>> >>need
>> >> >>to
>> >> >> >>> implement another check. I want to prevent such a burden to the
>> >> >> >>>consumers.
>> >> >> >>> Regarding 2) Since the client code knows on which object the
>> >> >> >>> adaptToOrThrow is called I don¹t get why an instanceof check is
>> >> >> >>>necessary.
>> >> >> >>> Whether this object implements Adaptable2 is known at
>> >>compile-time,
>> >> >>so
>> >> >> >>>you
>> >> >> >>> do have the full IDE-support here.
>> >> >> >>> Regarding 3) In that case I would no longer allow a null value
>> >>to be
>> >> >> >>> returned. One drawback is, that all the null checks are no longer
>> >> >> >>>effective.
>> >> >> >>>
>> >> >> >>> IMHO solution 2) is the best. At the same time I would deprecate
>> >>the
>> >> >> >>>old
>> >> >> >>> Adaptable, because I cannot come up with a real use-case where
>> >> >> >>>returning
>> >> >> >>> has an advantage. I would rather implement another method boolean
>> >> >> >>> hasAdapter(Class<AdapterType> type) on the Adaptable2 interface.
>> >> >> >>> Regards,
>> >> >> >>> Konrad
>> >> >> >>>
>> >> >> >>> On 01 Jul 2014, at 09:07, Felix Meschberger <fm...@adobe.com>
>> >> >> wrote:
>> >> >> >>>
>> >> >> >>>> Hi
>> >> >> >>>>
>> >> >> >>>> There currently are two issues floating around dealing with the
>> >> >> >>>>question
>> >> >> >>> of returning more information than just null from the
>> >> >> >>> Adaptable.adaptTo(Class) method:
>> >> >> >>> https://issues.apache.org/jira/browse/SLING-3714 and
>> >> >> >>> https://issues.apache.org/jira/browse/SLING-3709. I think these
>> >> >> >>>requests
>> >> >> >>> warrant some discussion on the list.
>> >> >> >>>>
>> >> >> >>>> Background: adaptTo can be implemented by Adaptable
>> >>implementations
>> >> >> >>> themselves or, by extending from SlingAdaptable, they may defer
>> >>to
>> >> >>an
>> >> >> >>> AdapterMananager. AdapterManager itself is extended by
>> >> >>AdapterFactory
>> >> >> >>> services. All these interfaces define an adaptTo method. All
>> >>these
>> >> >> >>>methods
>> >> >> >>> return null if adaption is not possible and don't declare or
>> >> >>document
>> >> >> >>>to
>> >> >> >>> throw an exception.
>> >> >> >>>>
>> >> >> >>>> While not explicitly documented as such, the intention is and
>> >>was
>> >> >>that
>> >> >> >>> adaptTo never throws on the grounds that adaption may fail which
>> >>is
>> >> >> >>> considered a valid result and thus exceptions are not to be
>> >>expected
>> >> >> >>>and
>> >> >> >>> handled.
>> >> >> >>>>
>> >> >> >>>> Hence all implementations of the methods generally
>> >> >> >>> catch-and-log-but-don't-throw. Interestingly
>> >>SlingAdaptable.adaptTo
>> >> >>and
>> >> >> >>> AdapterManagerImpl.getAdapter don't catch ‹ so any
>> >>RuntimeException
>> >> >> >>>thrown
>> >> >> >>> from an AdapterFactory would be forwarded.
>> >> >> >>>>
>> >> >> >>>> Having said this there are options available:
>> >> >> >>>>
>> >> >> >>>> (1) Add support for a new Result<?> class. We would probably
>> >> >>implement
>> >> >> >>> this in the AdapterManager.getAdapter implementation explicitly
>> >> >> >>>handling
>> >> >> >>> this case because it would entail catching the adaptTo/getAdapter
>> >> >> >>>calls to
>> >> >> >>> get the exception (the Result.getError should return Throwable
>> >> >> >>>probably not
>> >> >> >>> Error)
>> >> >> >>>>
>> >> >> >>>> Use would be limited to new AdapterFactory implementations
>> >>throwing
>> >> >> >>> RuntimeExcpetion. For Sling Models this would be the case.
>> >> >> >>>>
>> >> >> >>>> (2) Add a new adaptToOrThrow method, which is declared to throw
>> >>a
>> >> >> >>> RuntimeException and never return null: Either it can adapt or it
>> >> >> >>>throws.
>> >> >> >>> This would require a new interface Adaptable2 (probably) to not
>> >> >>break
>> >> >> >>> existing Adaptable implementations. The SlingAdaptable base class
>> >> >>would
>> >> >> >>> implement the new method of course, probably something like this:
>> >> >> >>>>
>> >> >> >>>>> SlingAdaptable implements Adaptable2 {
>> >> >> >>>>> Š
>> >> >> >>>>> public <AdapterType> AdapterType
>> >>adaptToOrThrow(Class<AdapterType>
>> >> >> >>> type) {
>> >> >> >>>>> AdapterType result = this.adaptTo(type);
>> >> >> >>>>> if (result != null) {
>> >> >> >>>>> return result;
>> >> >> >>>>> }
>> >> >> >>>>> throw new CannotAdaptException(Š);
>> >> >> >>>>> }
>> >> >> >>>>> }
>> >> >> >>>>>
>> >> >> >>>>
>> >> >> >>>> Use is problematic because you would have to know whether you
>> >>can
>> >> >>call
>> >> >> >>> the new method: So instead of an null check you now have an
>> >> >>instanceof
>> >> >> >>> check Š Except for the Resource interface which would be
>> >>extended to
>> >> >> >>>extend
>> >> >> >>> from Adaptable2 as well.
>> >> >> >>>>
>> >> >> >>>> (3) Document, that Adaptable.adaptTo may throw a
>> >>RuntimeException.
>> >> >> >>>>
>> >> >> >>>> The problem here is, that this may conceptually break existing
>> >> >>callers
>> >> >> >>> of Adaptable.adaptTo which don't expect an exception at all ‹
>> >> >> >>>presumably
>> >> >> >>> this is a minor nuisance because technically a RuntimeException
>> >>may
>> >> >> >>>always
>> >> >> >>> be thrown.
>> >> >> >>>>
>> >> >> >>>> Regards
>> >> >> >>>> Felix
>> >> >> >>>
>> >> >> >>>
>> >> >> >>
>> >> >> >>
>> >> >> >> --
>> >> >> >> Carsten Ziegeler
>> >> >> >> Adobe Research Switzerland
>> >> >> >> cziegeler@apache.org
>> >> >> >
>> >> >>
>> >> >>
>> >> >
>> >> >
>> >> >--
>> >> >Carsten Ziegeler
>> >> >Adobe Research Switzerland
>> >> >cziegeler@apache.org
>> >>
>> >>
>> >
>> >
>> >--
>> >Carsten Ziegeler
>> >Adobe Research Switzerland
>> >cziegeler@apache.org
>>
>>
>
>
>--
>Carsten Ziegeler
>Adobe Research Switzerland
>cziegeler@apache.org
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
I just fix it in the code ;-). Those exceptions should only happen during runtime (due to some false assumptions).
For the same reasons methods do throw IllegalArgumentExceptions in case a given parameter is null (http://stackoverflow.com/questions/5172948/should-we-always-check-each-parameter-of-method-in-java-for-null-in-the-first-li). This is mainly for the developer, but makes the life much easier as with that information it is obvious how to fix :-)
Konrad
On 01 Jul 2014, at 12:14, Carsten Ziegeler <cz...@apache.org> wrote:
> So if your adapter is buggy and you get an exception, what do you do with
> it?
>
> Carsten
>
>
> 2014-07-01 12:08 GMT+02:00 Jeff Young <je...@adobe.com>:
>
>> Hi Carsten,
>>
>> Sure, but Konrad has a point in that I think sometimes the client *does*
>> care why the adaption failed. For instance, if it had to do with
>> something entirely different from whether or not adaption would normally
>> work.
>>
>> Let's say that I have a resource that should adapt to XYZ, but that my
>> adapter is currently buggy. I'd like to get an exception for that, but
>> said exception is going to get eaten.
>>
>> I do agree that if I have a resource that should NOT adapt to XYZ, that
>> getting back null is fine, and that I don't care why the adaption failed.
>>
>> Cheers,
>> Jeff.
>>
>>
>> On 01/07/2014 10:19, "Carsten Ziegeler" <cz...@apache.org> wrote:
>>
>>> Sure :) For the adapter pattern, the client does not care why the adaption
>>> failed, the client is just interested in the result (success or not)
>>> Validation is a different beast, if validation fails you want to know
>>> specific reasons why it failed - and this can be multiple.
>>> I tried to explain in my first mail on this thread, that all other use
>>> cases mentioned can be handled with the current implementation - with the
>>> exception of validation. But I think validation requires a different
>>> concept than the adapter pattern.
>>>
>>> Carsten
>>>
>>>
>>> 2014-07-01 11:09 GMT+02:00 Jeff Young <je...@adobe.com>:
>>>
>>>> Hi Carsten,
>>>>
>>>> Can you say more? (I'm not sure I understand what you're getting
>>>> at....)
>>>>
>>>> Thanks,
>>>> Jeff.
>>>>
>>>>
>>>> On 01/07/2014 09:56, "Carsten Ziegeler" <cz...@apache.org> wrote:
>>>>
>>>>> adaption and validation are different concerns
>>>>>
>>>>> Carsten
>>>>>
>>>>>
>>>>> 2014-07-01 10:55 GMT+02:00 Jeff Young <je...@adobe.com>:
>>>>>
>>>>>> We could solve that by defining a specific exception for
>>>>>> adaptation-not-possible and then catch only that.
>>>>>>
>>>>>> Of course that would leak tons of exceptions from code written before
>>>>>> that
>>>>>> exception became available. Maybe do the catching based on some
>>>> sort of
>>>>>> version clue?
>>>>>>
>>>>>> Cheers,
>>>>>> Jeff.
>>>>>>
>>>>>>
>>>>>> On 01/07/2014 09:40, "Konrad Windszus" <ko...@gmx.de> wrote:
>>>>>>
>>>>>>> It is not (only) about throwing exceptions in case no suitable
>>>> adapter
>>>>>> is
>>>>>>> available. It rather is about the fact, that today the adaptTo is a
>>>>>>> barrier for all kinds of exceptions. In some cases the adaptation
>>>> fails
>>>>>>> for a specific reason (one example is Sling Models where injection
>>>>>> fails,
>>>>>>> another one is the issue mentioned in
>>>>>>> https://issues.apache.org/jira/browse/SLING-2712 (ValueMap not
>>>>>> supporting
>>>>>>> primitives)). Both would be valid use cases where the client would
>>>> be
>>>>>>> interested to catch the exception here.
>>>>>>>
>>>>>>> On 01 Jul 2014, at 10:34, Carsten Ziegeler <cz...@apache.org>
>>>>>> wrote:
>>>>>>>
>>>>>>>> Adding a new interface would require us to implement it all over
>>>> the
>>>>>>>> place
>>>>>>>> and as Felix points out, client code would always need to check
>>>>>> whether
>>>>>>>> the
>>>>>>>> new interface is implemented or not Having to methods, like
>>>>>> hasAdapter
>>>>>>>> and
>>>>>>>> adaptOrThrow does not work very well as between the two calls
>>>> things
>>>>>>>> might
>>>>>>>> have changed already: while hasAdapter returns true, the
>>>> underlying
>>>>>>>> factory
>>>>>>>> gets unregistered before adaptOrThrow is called.
>>>>>>>> In many use cases, the current pattern works fine - the client
>>>> does
>>>>>> not
>>>>>>>> care whether an exception is thrown within the adaption - it just
>>>>>> cares
>>>>>>>> whether an object is returned or not. And there are valid use
>>>> cases,
>>>>>>>> where
>>>>>>>> client code does different things whether the adaption works or
>>>> not
>>>>>>>> (e.g.
>>>>>>>> the post servlet checks for adaptTo(Node) and then does additional
>>>>>>>> things
>>>>>>>> if the resource is backed up by a node.)
>>>>>>>>
>>>>>>>> I see the point that there are also use cases where it would be
>>>> fine
>>>>>> to
>>>>>>>> simpy throw an exception if adaptTo is not successful. This would
>>>>>> make
>>>>>>>> the
>>>>>>>> client code easier. However as this most properly is a runtime
>>>>>>>> exception,
>>>>>>>> client code can today just call a method on the object and end up
>>>>>> with a
>>>>>>>> NPE - having the same result :)
>>>>>>>>
>>>>>>>> This leaves us with use cases where the client code explicitely
>>>>>> wants to
>>>>>>>> catch the exception and then do something depending on the
>>>> exception.
>>>>>>>> Maybe
>>>>>>>> we should just add something for this explicit use case instead of
>>>>>>>> bloating
>>>>>>>> the general adaptTo mechanism?
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Carsten
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
>>>>>>>>
>>>>>>>>> Regarding 1) Having such a Result class would mean that all
>>>> consumer
>>>>>>>>> would
>>>>>>>>> need to unwrap the exception first. So instead of being forced of
>>>>>>>>> implementing a null-check (as with the old solution) one would
>>>> need
>>>>>> to
>>>>>>>>> implement another check. I want to prevent such a burden to the
>>>>>>>>> consumers.
>>>>>>>>> Regarding 2) Since the client code knows on which object the
>>>>>>>>> adaptToOrThrow is called I don¹t get why an instanceof check is
>>>>>>>>> necessary.
>>>>>>>>> Whether this object implements Adaptable2 is known at
>>>> compile-time,
>>>>>> so
>>>>>>>>> you
>>>>>>>>> do have the full IDE-support here.
>>>>>>>>> Regarding 3) In that case I would no longer allow a null value
>>>> to be
>>>>>>>>> returned. One drawback is, that all the null checks are no longer
>>>>>>>>> effective.
>>>>>>>>>
>>>>>>>>> IMHO solution 2) is the best. At the same time I would deprecate
>>>> the
>>>>>>>>> old
>>>>>>>>> Adaptable, because I cannot come up with a real use-case where
>>>>>>>>> returning
>>>>>>>>> has an advantage. I would rather implement another method boolean
>>>>>>>>> hasAdapter(Class<AdapterType> type) on the Adaptable2 interface.
>>>>>>>>> Regards,
>>>>>>>>> Konrad
>>>>>>>>>
>>>>>>>>> On 01 Jul 2014, at 09:07, Felix Meschberger <fm...@adobe.com>
>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Hi
>>>>>>>>>>
>>>>>>>>>> There currently are two issues floating around dealing with the
>>>>>>>>>> question
>>>>>>>>> of returning more information than just null from the
>>>>>>>>> Adaptable.adaptTo(Class) method:
>>>>>>>>> https://issues.apache.org/jira/browse/SLING-3714 and
>>>>>>>>> https://issues.apache.org/jira/browse/SLING-3709. I think these
>>>>>>>>> requests
>>>>>>>>> warrant some discussion on the list.
>>>>>>>>>>
>>>>>>>>>> Background: adaptTo can be implemented by Adaptable
>>>> implementations
>>>>>>>>> themselves or, by extending from SlingAdaptable, they may defer
>>>> to
>>>>>> an
>>>>>>>>> AdapterMananager. AdapterManager itself is extended by
>>>>>> AdapterFactory
>>>>>>>>> services. All these interfaces define an adaptTo method. All
>>>> these
>>>>>>>>> methods
>>>>>>>>> return null if adaption is not possible and don't declare or
>>>>>> document
>>>>>>>>> to
>>>>>>>>> throw an exception.
>>>>>>>>>>
>>>>>>>>>> While not explicitly documented as such, the intention is and
>>>> was
>>>>>> that
>>>>>>>>> adaptTo never throws on the grounds that adaption may fail which
>>>> is
>>>>>>>>> considered a valid result and thus exceptions are not to be
>>>> expected
>>>>>>>>> and
>>>>>>>>> handled.
>>>>>>>>>>
>>>>>>>>>> Hence all implementations of the methods generally
>>>>>>>>> catch-and-log-but-don't-throw. Interestingly
>>>> SlingAdaptable.adaptTo
>>>>>> and
>>>>>>>>> AdapterManagerImpl.getAdapter don't catch ‹ so any
>>>> RuntimeException
>>>>>>>>> thrown
>>>>>>>>> from an AdapterFactory would be forwarded.
>>>>>>>>>>
>>>>>>>>>> Having said this there are options available:
>>>>>>>>>>
>>>>>>>>>> (1) Add support for a new Result<?> class. We would probably
>>>>>> implement
>>>>>>>>> this in the AdapterManager.getAdapter implementation explicitly
>>>>>>>>> handling
>>>>>>>>> this case because it would entail catching the adaptTo/getAdapter
>>>>>>>>> calls to
>>>>>>>>> get the exception (the Result.getError should return Throwable
>>>>>>>>> probably not
>>>>>>>>> Error)
>>>>>>>>>>
>>>>>>>>>> Use would be limited to new AdapterFactory implementations
>>>> throwing
>>>>>>>>> RuntimeExcpetion. For Sling Models this would be the case.
>>>>>>>>>>
>>>>>>>>>> (2) Add a new adaptToOrThrow method, which is declared to throw
>>>> a
>>>>>>>>> RuntimeException and never return null: Either it can adapt or it
>>>>>>>>> throws.
>>>>>>>>> This would require a new interface Adaptable2 (probably) to not
>>>>>> break
>>>>>>>>> existing Adaptable implementations. The SlingAdaptable base class
>>>>>> would
>>>>>>>>> implement the new method of course, probably something like this:
>>>>>>>>>>
>>>>>>>>>>> SlingAdaptable implements Adaptable2 {
>>>>>>>>>>> Š
>>>>>>>>>>> public <AdapterType> AdapterType
>>>> adaptToOrThrow(Class<AdapterType>
>>>>>>>>> type) {
>>>>>>>>>>> AdapterType result = this.adaptTo(type);
>>>>>>>>>>> if (result != null) {
>>>>>>>>>>> return result;
>>>>>>>>>>> }
>>>>>>>>>>> throw new CannotAdaptException(Š);
>>>>>>>>>>> }
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Use is problematic because you would have to know whether you
>>>> can
>>>>>> call
>>>>>>>>> the new method: So instead of an null check you now have an
>>>>>> instanceof
>>>>>>>>> check Š Except for the Resource interface which would be
>>>> extended to
>>>>>>>>> extend
>>>>>>>>> from Adaptable2 as well.
>>>>>>>>>>
>>>>>>>>>> (3) Document, that Adaptable.adaptTo may throw a
>>>> RuntimeException.
>>>>>>>>>>
>>>>>>>>>> The problem here is, that this may conceptually break existing
>>>>>> callers
>>>>>>>>> of Adaptable.adaptTo which don't expect an exception at all ‹
>>>>>>>>> presumably
>>>>>>>>> this is a minor nuisance because technically a RuntimeException
>>>> may
>>>>>>>>> always
>>>>>>>>> be thrown.
>>>>>>>>>>
>>>>>>>>>> Regards
>>>>>>>>>> Felix
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Carsten Ziegeler
>>>>>>>> Adobe Research Switzerland
>>>>>>>> cziegeler@apache.org
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Carsten Ziegeler
>>>>> Adobe Research Switzerland
>>>>> cziegeler@apache.org
>>>>
>>>>
>>>
>>>
>>> --
>>> Carsten Ziegeler
>>> Adobe Research Switzerland
>>> cziegeler@apache.org
>>
>>
>
>
> --
> Carsten Ziegeler
> Adobe Research Switzerland
> cziegeler@apache.org
Re: adaptTo and results ....
Posted by Jeff Young <je...@adobe.com>.
Well, for one thing, display it in the Developer Mode console (or whatever
other debugging UIs my app happens to have).
Jeff.
On 01/07/2014 11:14, "Carsten Ziegeler" <cz...@apache.org> wrote:
>So if your adapter is buggy and you get an exception, what do you do with
>it?
>
>Carsten
>
>
>2014-07-01 12:08 GMT+02:00 Jeff Young <je...@adobe.com>:
>
>> Hi Carsten,
>>
>> Sure, but Konrad has a point in that I think sometimes the client *does*
>> care why the adaption failed. For instance, if it had to do with
>> something entirely different from whether or not adaption would normally
>> work.
>>
>> Let's say that I have a resource that should adapt to XYZ, but that my
>> adapter is currently buggy. I'd like to get an exception for that, but
>> said exception is going to get eaten.
>>
>> I do agree that if I have a resource that should NOT adapt to XYZ, that
>> getting back null is fine, and that I don't care why the adaption
>>failed.
>>
>> Cheers,
>> Jeff.
>>
>>
>> On 01/07/2014 10:19, "Carsten Ziegeler" <cz...@apache.org> wrote:
>>
>> >Sure :) For the adapter pattern, the client does not care why the
>>adaption
>> >failed, the client is just interested in the result (success or not)
>> >Validation is a different beast, if validation fails you want to know
>> >specific reasons why it failed - and this can be multiple.
>> >I tried to explain in my first mail on this thread, that all other use
>> >cases mentioned can be handled with the current implementation - with
>>the
>> >exception of validation. But I think validation requires a different
>> >concept than the adapter pattern.
>> >
>> >Carsten
>> >
>> >
>> >2014-07-01 11:09 GMT+02:00 Jeff Young <je...@adobe.com>:
>> >
>> >> Hi Carsten,
>> >>
>> >> Can you say more? (I'm not sure I understand what you're getting
>> >>at....)
>> >>
>> >> Thanks,
>> >> Jeff.
>> >>
>> >>
>> >> On 01/07/2014 09:56, "Carsten Ziegeler" <cz...@apache.org> wrote:
>> >>
>> >> >adaption and validation are different concerns
>> >> >
>> >> >Carsten
>> >> >
>> >> >
>> >> >2014-07-01 10:55 GMT+02:00 Jeff Young <je...@adobe.com>:
>> >> >
>> >> >> We could solve that by defining a specific exception for
>> >> >> adaptation-not-possible and then catch only that.
>> >> >>
>> >> >> Of course that would leak tons of exceptions from code written
>>before
>> >> >>that
>> >> >> exception became available. Maybe do the catching based on some
>> >>sort of
>> >> >> version clue?
>> >> >>
>> >> >> Cheers,
>> >> >> Jeff.
>> >> >>
>> >> >>
>> >> >> On 01/07/2014 09:40, "Konrad Windszus" <ko...@gmx.de> wrote:
>> >> >>
>> >> >> >It is not (only) about throwing exceptions in case no suitable
>> >>adapter
>> >> >>is
>> >> >> >available. It rather is about the fact, that today the adaptTo
>>is a
>> >> >> >barrier for all kinds of exceptions. In some cases the adaptation
>> >>fails
>> >> >> >for a specific reason (one example is Sling Models where
>>injection
>> >> >>fails,
>> >> >> >another one is the issue mentioned in
>> >> >> >https://issues.apache.org/jira/browse/SLING-2712 (ValueMap not
>> >> >>supporting
>> >> >> >primitives)). Both would be valid use cases where the client
>>would
>> >>be
>> >> >> >interested to catch the exception here.
>> >> >> >
>> >> >> >On 01 Jul 2014, at 10:34, Carsten Ziegeler <cz...@apache.org>
>> >> >>wrote:
>> >> >> >
>> >> >> >> Adding a new interface would require us to implement it all
>>over
>> >>the
>> >> >> >>place
>> >> >> >> and as Felix points out, client code would always need to check
>> >> >>whether
>> >> >> >>the
>> >> >> >> new interface is implemented or not Having to methods, like
>> >> >>hasAdapter
>> >> >> >>and
>> >> >> >> adaptOrThrow does not work very well as between the two calls
>> >>things
>> >> >> >>might
>> >> >> >> have changed already: while hasAdapter returns true, the
>> >>underlying
>> >> >> >>factory
>> >> >> >> gets unregistered before adaptOrThrow is called.
>> >> >> >> In many use cases, the current pattern works fine - the client
>> >>does
>> >> >>not
>> >> >> >> care whether an exception is thrown within the adaption - it
>>just
>> >> >>cares
>> >> >> >> whether an object is returned or not. And there are valid use
>> >>cases,
>> >> >> >>where
>> >> >> >> client code does different things whether the adaption works or
>> >>not
>> >> >> >>(e.g.
>> >> >> >> the post servlet checks for adaptTo(Node) and then does
>>additional
>> >> >> >>things
>> >> >> >> if the resource is backed up by a node.)
>> >> >> >>
>> >> >> >> I see the point that there are also use cases where it would be
>> >>fine
>> >> >>to
>> >> >> >> simpy throw an exception if adaptTo is not successful. This
>>would
>> >> >>make
>> >> >> >>the
>> >> >> >> client code easier. However as this most properly is a runtime
>> >> >> >>exception,
>> >> >> >> client code can today just call a method on the object and end
>>up
>> >> >>with a
>> >> >> >> NPE - having the same result :)
>> >> >> >>
>> >> >> >> This leaves us with use cases where the client code explicitely
>> >> >>wants to
>> >> >> >> catch the exception and then do something depending on the
>> >>exception.
>> >> >> >>Maybe
>> >> >> >> we should just add something for this explicit use case
>>instead of
>> >> >> >>bloating
>> >> >> >> the general adaptTo mechanism?
>> >> >> >>
>> >> >> >> Regards
>> >> >> >> Carsten
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >> 2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
>> >> >> >>
>> >> >> >>> Regarding 1) Having such a Result class would mean that all
>> >>consumer
>> >> >> >>>would
>> >> >> >>> need to unwrap the exception first. So instead of being
>>forced of
>> >> >> >>> implementing a null-check (as with the old solution) one would
>> >>need
>> >> >>to
>> >> >> >>> implement another check. I want to prevent such a burden to
>>the
>> >> >> >>>consumers.
>> >> >> >>> Regarding 2) Since the client code knows on which object the
>> >> >> >>> adaptToOrThrow is called I don¹t get why an instanceof check
>>is
>> >> >> >>>necessary.
>> >> >> >>> Whether this object implements Adaptable2 is known at
>> >>compile-time,
>> >> >>so
>> >> >> >>>you
>> >> >> >>> do have the full IDE-support here.
>> >> >> >>> Regarding 3) In that case I would no longer allow a null value
>> >>to be
>> >> >> >>> returned. One drawback is, that all the null checks are no
>>longer
>> >> >> >>>effective.
>> >> >> >>>
>> >> >> >>> IMHO solution 2) is the best. At the same time I would
>>deprecate
>> >>the
>> >> >> >>>old
>> >> >> >>> Adaptable, because I cannot come up with a real use-case where
>> >> >> >>>returning
>> >> >> >>> has an advantage. I would rather implement another method
>>boolean
>> >> >> >>> hasAdapter(Class<AdapterType> type) on the Adaptable2
>>interface.
>> >> >> >>> Regards,
>> >> >> >>> Konrad
>> >> >> >>>
>> >> >> >>> On 01 Jul 2014, at 09:07, Felix Meschberger
>><fm...@adobe.com>
>> >> >> wrote:
>> >> >> >>>
>> >> >> >>>> Hi
>> >> >> >>>>
>> >> >> >>>> There currently are two issues floating around dealing with
>>the
>> >> >> >>>>question
>> >> >> >>> of returning more information than just null from the
>> >> >> >>> Adaptable.adaptTo(Class) method:
>> >> >> >>> https://issues.apache.org/jira/browse/SLING-3714 and
>> >> >> >>> https://issues.apache.org/jira/browse/SLING-3709. I think
>>these
>> >> >> >>>requests
>> >> >> >>> warrant some discussion on the list.
>> >> >> >>>>
>> >> >> >>>> Background: adaptTo can be implemented by Adaptable
>> >>implementations
>> >> >> >>> themselves or, by extending from SlingAdaptable, they may
>>defer
>> >>to
>> >> >>an
>> >> >> >>> AdapterMananager. AdapterManager itself is extended by
>> >> >>AdapterFactory
>> >> >> >>> services. All these interfaces define an adaptTo method. All
>> >>these
>> >> >> >>>methods
>> >> >> >>> return null if adaption is not possible and don't declare or
>> >> >>document
>> >> >> >>>to
>> >> >> >>> throw an exception.
>> >> >> >>>>
>> >> >> >>>> While not explicitly documented as such, the intention is and
>> >>was
>> >> >>that
>> >> >> >>> adaptTo never throws on the grounds that adaption may fail
>>which
>> >>is
>> >> >> >>> considered a valid result and thus exceptions are not to be
>> >>expected
>> >> >> >>>and
>> >> >> >>> handled.
>> >> >> >>>>
>> >> >> >>>> Hence all implementations of the methods generally
>> >> >> >>> catch-and-log-but-don't-throw. Interestingly
>> >>SlingAdaptable.adaptTo
>> >> >>and
>> >> >> >>> AdapterManagerImpl.getAdapter don't catch ‹ so any
>> >>RuntimeException
>> >> >> >>>thrown
>> >> >> >>> from an AdapterFactory would be forwarded.
>> >> >> >>>>
>> >> >> >>>> Having said this there are options available:
>> >> >> >>>>
>> >> >> >>>> (1) Add support for a new Result<?> class. We would probably
>> >> >>implement
>> >> >> >>> this in the AdapterManager.getAdapter implementation
>>explicitly
>> >> >> >>>handling
>> >> >> >>> this case because it would entail catching the
>>adaptTo/getAdapter
>> >> >> >>>calls to
>> >> >> >>> get the exception (the Result.getError should return Throwable
>> >> >> >>>probably not
>> >> >> >>> Error)
>> >> >> >>>>
>> >> >> >>>> Use would be limited to new AdapterFactory implementations
>> >>throwing
>> >> >> >>> RuntimeExcpetion. For Sling Models this would be the case.
>> >> >> >>>>
>> >> >> >>>> (2) Add a new adaptToOrThrow method, which is declared to
>>throw
>> >>a
>> >> >> >>> RuntimeException and never return null: Either it can adapt
>>or it
>> >> >> >>>throws.
>> >> >> >>> This would require a new interface Adaptable2 (probably) to
>>not
>> >> >>break
>> >> >> >>> existing Adaptable implementations. The SlingAdaptable base
>>class
>> >> >>would
>> >> >> >>> implement the new method of course, probably something like
>>this:
>> >> >> >>>>
>> >> >> >>>>> SlingAdaptable implements Adaptable2 {
>> >> >> >>>>> Š
>> >> >> >>>>> public <AdapterType> AdapterType
>> >>adaptToOrThrow(Class<AdapterType>
>> >> >> >>> type) {
>> >> >> >>>>> AdapterType result = this.adaptTo(type);
>> >> >> >>>>> if (result != null) {
>> >> >> >>>>> return result;
>> >> >> >>>>> }
>> >> >> >>>>> throw new CannotAdaptException(Š);
>> >> >> >>>>> }
>> >> >> >>>>> }
>> >> >> >>>>>
>> >> >> >>>>
>> >> >> >>>> Use is problematic because you would have to know whether you
>> >>can
>> >> >>call
>> >> >> >>> the new method: So instead of an null check you now have an
>> >> >>instanceof
>> >> >> >>> check Š Except for the Resource interface which would be
>> >>extended to
>> >> >> >>>extend
>> >> >> >>> from Adaptable2 as well.
>> >> >> >>>>
>> >> >> >>>> (3) Document, that Adaptable.adaptTo may throw a
>> >>RuntimeException.
>> >> >> >>>>
>> >> >> >>>> The problem here is, that this may conceptually break
>>existing
>> >> >>callers
>> >> >> >>> of Adaptable.adaptTo which don't expect an exception at all ‹
>> >> >> >>>presumably
>> >> >> >>> this is a minor nuisance because technically a
>>RuntimeException
>> >>may
>> >> >> >>>always
>> >> >> >>> be thrown.
>> >> >> >>>>
>> >> >> >>>> Regards
>> >> >> >>>> Felix
>> >> >> >>>
>> >> >> >>>
>> >> >> >>
>> >> >> >>
>> >> >> >> --
>> >> >> >> Carsten Ziegeler
>> >> >> >> Adobe Research Switzerland
>> >> >> >> cziegeler@apache.org
>> >> >> >
>> >> >>
>> >> >>
>> >> >
>> >> >
>> >> >--
>> >> >Carsten Ziegeler
>> >> >Adobe Research Switzerland
>> >> >cziegeler@apache.org
>> >>
>> >>
>> >
>> >
>> >--
>> >Carsten Ziegeler
>> >Adobe Research Switzerland
>> >cziegeler@apache.org
>>
>>
>
>
>--
>Carsten Ziegeler
>Adobe Research Switzerland
>cziegeler@apache.org
Re: adaptTo and results ....
Posted by Carsten Ziegeler <cz...@apache.org>.
So if your adapter is buggy and you get an exception, what do you do with
it?
Carsten
2014-07-01 12:08 GMT+02:00 Jeff Young <je...@adobe.com>:
> Hi Carsten,
>
> Sure, but Konrad has a point in that I think sometimes the client *does*
> care why the adaption failed. For instance, if it had to do with
> something entirely different from whether or not adaption would normally
> work.
>
> Let's say that I have a resource that should adapt to XYZ, but that my
> adapter is currently buggy. I'd like to get an exception for that, but
> said exception is going to get eaten.
>
> I do agree that if I have a resource that should NOT adapt to XYZ, that
> getting back null is fine, and that I don't care why the adaption failed.
>
> Cheers,
> Jeff.
>
>
> On 01/07/2014 10:19, "Carsten Ziegeler" <cz...@apache.org> wrote:
>
> >Sure :) For the adapter pattern, the client does not care why the adaption
> >failed, the client is just interested in the result (success or not)
> >Validation is a different beast, if validation fails you want to know
> >specific reasons why it failed - and this can be multiple.
> >I tried to explain in my first mail on this thread, that all other use
> >cases mentioned can be handled with the current implementation - with the
> >exception of validation. But I think validation requires a different
> >concept than the adapter pattern.
> >
> >Carsten
> >
> >
> >2014-07-01 11:09 GMT+02:00 Jeff Young <je...@adobe.com>:
> >
> >> Hi Carsten,
> >>
> >> Can you say more? (I'm not sure I understand what you're getting
> >>at....)
> >>
> >> Thanks,
> >> Jeff.
> >>
> >>
> >> On 01/07/2014 09:56, "Carsten Ziegeler" <cz...@apache.org> wrote:
> >>
> >> >adaption and validation are different concerns
> >> >
> >> >Carsten
> >> >
> >> >
> >> >2014-07-01 10:55 GMT+02:00 Jeff Young <je...@adobe.com>:
> >> >
> >> >> We could solve that by defining a specific exception for
> >> >> adaptation-not-possible and then catch only that.
> >> >>
> >> >> Of course that would leak tons of exceptions from code written before
> >> >>that
> >> >> exception became available. Maybe do the catching based on some
> >>sort of
> >> >> version clue?
> >> >>
> >> >> Cheers,
> >> >> Jeff.
> >> >>
> >> >>
> >> >> On 01/07/2014 09:40, "Konrad Windszus" <ko...@gmx.de> wrote:
> >> >>
> >> >> >It is not (only) about throwing exceptions in case no suitable
> >>adapter
> >> >>is
> >> >> >available. It rather is about the fact, that today the adaptTo is a
> >> >> >barrier for all kinds of exceptions. In some cases the adaptation
> >>fails
> >> >> >for a specific reason (one example is Sling Models where injection
> >> >>fails,
> >> >> >another one is the issue mentioned in
> >> >> >https://issues.apache.org/jira/browse/SLING-2712 (ValueMap not
> >> >>supporting
> >> >> >primitives)). Both would be valid use cases where the client would
> >>be
> >> >> >interested to catch the exception here.
> >> >> >
> >> >> >On 01 Jul 2014, at 10:34, Carsten Ziegeler <cz...@apache.org>
> >> >>wrote:
> >> >> >
> >> >> >> Adding a new interface would require us to implement it all over
> >>the
> >> >> >>place
> >> >> >> and as Felix points out, client code would always need to check
> >> >>whether
> >> >> >>the
> >> >> >> new interface is implemented or not Having to methods, like
> >> >>hasAdapter
> >> >> >>and
> >> >> >> adaptOrThrow does not work very well as between the two calls
> >>things
> >> >> >>might
> >> >> >> have changed already: while hasAdapter returns true, the
> >>underlying
> >> >> >>factory
> >> >> >> gets unregistered before adaptOrThrow is called.
> >> >> >> In many use cases, the current pattern works fine - the client
> >>does
> >> >>not
> >> >> >> care whether an exception is thrown within the adaption - it just
> >> >>cares
> >> >> >> whether an object is returned or not. And there are valid use
> >>cases,
> >> >> >>where
> >> >> >> client code does different things whether the adaption works or
> >>not
> >> >> >>(e.g.
> >> >> >> the post servlet checks for adaptTo(Node) and then does additional
> >> >> >>things
> >> >> >> if the resource is backed up by a node.)
> >> >> >>
> >> >> >> I see the point that there are also use cases where it would be
> >>fine
> >> >>to
> >> >> >> simpy throw an exception if adaptTo is not successful. This would
> >> >>make
> >> >> >>the
> >> >> >> client code easier. However as this most properly is a runtime
> >> >> >>exception,
> >> >> >> client code can today just call a method on the object and end up
> >> >>with a
> >> >> >> NPE - having the same result :)
> >> >> >>
> >> >> >> This leaves us with use cases where the client code explicitely
> >> >>wants to
> >> >> >> catch the exception and then do something depending on the
> >>exception.
> >> >> >>Maybe
> >> >> >> we should just add something for this explicit use case instead of
> >> >> >>bloating
> >> >> >> the general adaptTo mechanism?
> >> >> >>
> >> >> >> Regards
> >> >> >> Carsten
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> 2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
> >> >> >>
> >> >> >>> Regarding 1) Having such a Result class would mean that all
> >>consumer
> >> >> >>>would
> >> >> >>> need to unwrap the exception first. So instead of being forced of
> >> >> >>> implementing a null-check (as with the old solution) one would
> >>need
> >> >>to
> >> >> >>> implement another check. I want to prevent such a burden to the
> >> >> >>>consumers.
> >> >> >>> Regarding 2) Since the client code knows on which object the
> >> >> >>> adaptToOrThrow is called I don¹t get why an instanceof check is
> >> >> >>>necessary.
> >> >> >>> Whether this object implements Adaptable2 is known at
> >>compile-time,
> >> >>so
> >> >> >>>you
> >> >> >>> do have the full IDE-support here.
> >> >> >>> Regarding 3) In that case I would no longer allow a null value
> >>to be
> >> >> >>> returned. One drawback is, that all the null checks are no longer
> >> >> >>>effective.
> >> >> >>>
> >> >> >>> IMHO solution 2) is the best. At the same time I would deprecate
> >>the
> >> >> >>>old
> >> >> >>> Adaptable, because I cannot come up with a real use-case where
> >> >> >>>returning
> >> >> >>> has an advantage. I would rather implement another method boolean
> >> >> >>> hasAdapter(Class<AdapterType> type) on the Adaptable2 interface.
> >> >> >>> Regards,
> >> >> >>> Konrad
> >> >> >>>
> >> >> >>> On 01 Jul 2014, at 09:07, Felix Meschberger <fm...@adobe.com>
> >> >> wrote:
> >> >> >>>
> >> >> >>>> Hi
> >> >> >>>>
> >> >> >>>> There currently are two issues floating around dealing with the
> >> >> >>>>question
> >> >> >>> of returning more information than just null from the
> >> >> >>> Adaptable.adaptTo(Class) method:
> >> >> >>> https://issues.apache.org/jira/browse/SLING-3714 and
> >> >> >>> https://issues.apache.org/jira/browse/SLING-3709. I think these
> >> >> >>>requests
> >> >> >>> warrant some discussion on the list.
> >> >> >>>>
> >> >> >>>> Background: adaptTo can be implemented by Adaptable
> >>implementations
> >> >> >>> themselves or, by extending from SlingAdaptable, they may defer
> >>to
> >> >>an
> >> >> >>> AdapterMananager. AdapterManager itself is extended by
> >> >>AdapterFactory
> >> >> >>> services. All these interfaces define an adaptTo method. All
> >>these
> >> >> >>>methods
> >> >> >>> return null if adaption is not possible and don't declare or
> >> >>document
> >> >> >>>to
> >> >> >>> throw an exception.
> >> >> >>>>
> >> >> >>>> While not explicitly documented as such, the intention is and
> >>was
> >> >>that
> >> >> >>> adaptTo never throws on the grounds that adaption may fail which
> >>is
> >> >> >>> considered a valid result and thus exceptions are not to be
> >>expected
> >> >> >>>and
> >> >> >>> handled.
> >> >> >>>>
> >> >> >>>> Hence all implementations of the methods generally
> >> >> >>> catch-and-log-but-don't-throw. Interestingly
> >>SlingAdaptable.adaptTo
> >> >>and
> >> >> >>> AdapterManagerImpl.getAdapter don't catch ‹ so any
> >>RuntimeException
> >> >> >>>thrown
> >> >> >>> from an AdapterFactory would be forwarded.
> >> >> >>>>
> >> >> >>>> Having said this there are options available:
> >> >> >>>>
> >> >> >>>> (1) Add support for a new Result<?> class. We would probably
> >> >>implement
> >> >> >>> this in the AdapterManager.getAdapter implementation explicitly
> >> >> >>>handling
> >> >> >>> this case because it would entail catching the adaptTo/getAdapter
> >> >> >>>calls to
> >> >> >>> get the exception (the Result.getError should return Throwable
> >> >> >>>probably not
> >> >> >>> Error)
> >> >> >>>>
> >> >> >>>> Use would be limited to new AdapterFactory implementations
> >>throwing
> >> >> >>> RuntimeExcpetion. For Sling Models this would be the case.
> >> >> >>>>
> >> >> >>>> (2) Add a new adaptToOrThrow method, which is declared to throw
> >>a
> >> >> >>> RuntimeException and never return null: Either it can adapt or it
> >> >> >>>throws.
> >> >> >>> This would require a new interface Adaptable2 (probably) to not
> >> >>break
> >> >> >>> existing Adaptable implementations. The SlingAdaptable base class
> >> >>would
> >> >> >>> implement the new method of course, probably something like this:
> >> >> >>>>
> >> >> >>>>> SlingAdaptable implements Adaptable2 {
> >> >> >>>>> Š
> >> >> >>>>> public <AdapterType> AdapterType
> >>adaptToOrThrow(Class<AdapterType>
> >> >> >>> type) {
> >> >> >>>>> AdapterType result = this.adaptTo(type);
> >> >> >>>>> if (result != null) {
> >> >> >>>>> return result;
> >> >> >>>>> }
> >> >> >>>>> throw new CannotAdaptException(Š);
> >> >> >>>>> }
> >> >> >>>>> }
> >> >> >>>>>
> >> >> >>>>
> >> >> >>>> Use is problematic because you would have to know whether you
> >>can
> >> >>call
> >> >> >>> the new method: So instead of an null check you now have an
> >> >>instanceof
> >> >> >>> check Š Except for the Resource interface which would be
> >>extended to
> >> >> >>>extend
> >> >> >>> from Adaptable2 as well.
> >> >> >>>>
> >> >> >>>> (3) Document, that Adaptable.adaptTo may throw a
> >>RuntimeException.
> >> >> >>>>
> >> >> >>>> The problem here is, that this may conceptually break existing
> >> >>callers
> >> >> >>> of Adaptable.adaptTo which don't expect an exception at all ‹
> >> >> >>>presumably
> >> >> >>> this is a minor nuisance because technically a RuntimeException
> >>may
> >> >> >>>always
> >> >> >>> be thrown.
> >> >> >>>>
> >> >> >>>> Regards
> >> >> >>>> Felix
> >> >> >>>
> >> >> >>>
> >> >> >>
> >> >> >>
> >> >> >> --
> >> >> >> Carsten Ziegeler
> >> >> >> Adobe Research Switzerland
> >> >> >> cziegeler@apache.org
> >> >> >
> >> >>
> >> >>
> >> >
> >> >
> >> >--
> >> >Carsten Ziegeler
> >> >Adobe Research Switzerland
> >> >cziegeler@apache.org
> >>
> >>
> >
> >
> >--
> >Carsten Ziegeler
> >Adobe Research Switzerland
> >cziegeler@apache.org
>
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: adaptTo and results ....
Posted by Jeff Young <je...@adobe.com>.
Hi Carsten,
Sure, but Konrad has a point in that I think sometimes the client *does*
care why the adaption failed. For instance, if it had to do with
something entirely different from whether or not adaption would normally
work.
Let's say that I have a resource that should adapt to XYZ, but that my
adapter is currently buggy. I'd like to get an exception for that, but
said exception is going to get eaten.
I do agree that if I have a resource that should NOT adapt to XYZ, that
getting back null is fine, and that I don't care why the adaption failed.
Cheers,
Jeff.
On 01/07/2014 10:19, "Carsten Ziegeler" <cz...@apache.org> wrote:
>Sure :) For the adapter pattern, the client does not care why the adaption
>failed, the client is just interested in the result (success or not)
>Validation is a different beast, if validation fails you want to know
>specific reasons why it failed - and this can be multiple.
>I tried to explain in my first mail on this thread, that all other use
>cases mentioned can be handled with the current implementation - with the
>exception of validation. But I think validation requires a different
>concept than the adapter pattern.
>
>Carsten
>
>
>2014-07-01 11:09 GMT+02:00 Jeff Young <je...@adobe.com>:
>
>> Hi Carsten,
>>
>> Can you say more? (I'm not sure I understand what you're getting
>>at....)
>>
>> Thanks,
>> Jeff.
>>
>>
>> On 01/07/2014 09:56, "Carsten Ziegeler" <cz...@apache.org> wrote:
>>
>> >adaption and validation are different concerns
>> >
>> >Carsten
>> >
>> >
>> >2014-07-01 10:55 GMT+02:00 Jeff Young <je...@adobe.com>:
>> >
>> >> We could solve that by defining a specific exception for
>> >> adaptation-not-possible and then catch only that.
>> >>
>> >> Of course that would leak tons of exceptions from code written before
>> >>that
>> >> exception became available. Maybe do the catching based on some
>>sort of
>> >> version clue?
>> >>
>> >> Cheers,
>> >> Jeff.
>> >>
>> >>
>> >> On 01/07/2014 09:40, "Konrad Windszus" <ko...@gmx.de> wrote:
>> >>
>> >> >It is not (only) about throwing exceptions in case no suitable
>>adapter
>> >>is
>> >> >available. It rather is about the fact, that today the adaptTo is a
>> >> >barrier for all kinds of exceptions. In some cases the adaptation
>>fails
>> >> >for a specific reason (one example is Sling Models where injection
>> >>fails,
>> >> >another one is the issue mentioned in
>> >> >https://issues.apache.org/jira/browse/SLING-2712 (ValueMap not
>> >>supporting
>> >> >primitives)). Both would be valid use cases where the client would
>>be
>> >> >interested to catch the exception here.
>> >> >
>> >> >On 01 Jul 2014, at 10:34, Carsten Ziegeler <cz...@apache.org>
>> >>wrote:
>> >> >
>> >> >> Adding a new interface would require us to implement it all over
>>the
>> >> >>place
>> >> >> and as Felix points out, client code would always need to check
>> >>whether
>> >> >>the
>> >> >> new interface is implemented or not Having to methods, like
>> >>hasAdapter
>> >> >>and
>> >> >> adaptOrThrow does not work very well as between the two calls
>>things
>> >> >>might
>> >> >> have changed already: while hasAdapter returns true, the
>>underlying
>> >> >>factory
>> >> >> gets unregistered before adaptOrThrow is called.
>> >> >> In many use cases, the current pattern works fine - the client
>>does
>> >>not
>> >> >> care whether an exception is thrown within the adaption - it just
>> >>cares
>> >> >> whether an object is returned or not. And there are valid use
>>cases,
>> >> >>where
>> >> >> client code does different things whether the adaption works or
>>not
>> >> >>(e.g.
>> >> >> the post servlet checks for adaptTo(Node) and then does additional
>> >> >>things
>> >> >> if the resource is backed up by a node.)
>> >> >>
>> >> >> I see the point that there are also use cases where it would be
>>fine
>> >>to
>> >> >> simpy throw an exception if adaptTo is not successful. This would
>> >>make
>> >> >>the
>> >> >> client code easier. However as this most properly is a runtime
>> >> >>exception,
>> >> >> client code can today just call a method on the object and end up
>> >>with a
>> >> >> NPE - having the same result :)
>> >> >>
>> >> >> This leaves us with use cases where the client code explicitely
>> >>wants to
>> >> >> catch the exception and then do something depending on the
>>exception.
>> >> >>Maybe
>> >> >> we should just add something for this explicit use case instead of
>> >> >>bloating
>> >> >> the general adaptTo mechanism?
>> >> >>
>> >> >> Regards
>> >> >> Carsten
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >> 2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
>> >> >>
>> >> >>> Regarding 1) Having such a Result class would mean that all
>>consumer
>> >> >>>would
>> >> >>> need to unwrap the exception first. So instead of being forced of
>> >> >>> implementing a null-check (as with the old solution) one would
>>need
>> >>to
>> >> >>> implement another check. I want to prevent such a burden to the
>> >> >>>consumers.
>> >> >>> Regarding 2) Since the client code knows on which object the
>> >> >>> adaptToOrThrow is called I don¹t get why an instanceof check is
>> >> >>>necessary.
>> >> >>> Whether this object implements Adaptable2 is known at
>>compile-time,
>> >>so
>> >> >>>you
>> >> >>> do have the full IDE-support here.
>> >> >>> Regarding 3) In that case I would no longer allow a null value
>>to be
>> >> >>> returned. One drawback is, that all the null checks are no longer
>> >> >>>effective.
>> >> >>>
>> >> >>> IMHO solution 2) is the best. At the same time I would deprecate
>>the
>> >> >>>old
>> >> >>> Adaptable, because I cannot come up with a real use-case where
>> >> >>>returning
>> >> >>> has an advantage. I would rather implement another method boolean
>> >> >>> hasAdapter(Class<AdapterType> type) on the Adaptable2 interface.
>> >> >>> Regards,
>> >> >>> Konrad
>> >> >>>
>> >> >>> On 01 Jul 2014, at 09:07, Felix Meschberger <fm...@adobe.com>
>> >> wrote:
>> >> >>>
>> >> >>>> Hi
>> >> >>>>
>> >> >>>> There currently are two issues floating around dealing with the
>> >> >>>>question
>> >> >>> of returning more information than just null from the
>> >> >>> Adaptable.adaptTo(Class) method:
>> >> >>> https://issues.apache.org/jira/browse/SLING-3714 and
>> >> >>> https://issues.apache.org/jira/browse/SLING-3709. I think these
>> >> >>>requests
>> >> >>> warrant some discussion on the list.
>> >> >>>>
>> >> >>>> Background: adaptTo can be implemented by Adaptable
>>implementations
>> >> >>> themselves or, by extending from SlingAdaptable, they may defer
>>to
>> >>an
>> >> >>> AdapterMananager. AdapterManager itself is extended by
>> >>AdapterFactory
>> >> >>> services. All these interfaces define an adaptTo method. All
>>these
>> >> >>>methods
>> >> >>> return null if adaption is not possible and don't declare or
>> >>document
>> >> >>>to
>> >> >>> throw an exception.
>> >> >>>>
>> >> >>>> While not explicitly documented as such, the intention is and
>>was
>> >>that
>> >> >>> adaptTo never throws on the grounds that adaption may fail which
>>is
>> >> >>> considered a valid result and thus exceptions are not to be
>>expected
>> >> >>>and
>> >> >>> handled.
>> >> >>>>
>> >> >>>> Hence all implementations of the methods generally
>> >> >>> catch-and-log-but-don't-throw. Interestingly
>>SlingAdaptable.adaptTo
>> >>and
>> >> >>> AdapterManagerImpl.getAdapter don't catch ‹ so any
>>RuntimeException
>> >> >>>thrown
>> >> >>> from an AdapterFactory would be forwarded.
>> >> >>>>
>> >> >>>> Having said this there are options available:
>> >> >>>>
>> >> >>>> (1) Add support for a new Result<?> class. We would probably
>> >>implement
>> >> >>> this in the AdapterManager.getAdapter implementation explicitly
>> >> >>>handling
>> >> >>> this case because it would entail catching the adaptTo/getAdapter
>> >> >>>calls to
>> >> >>> get the exception (the Result.getError should return Throwable
>> >> >>>probably not
>> >> >>> Error)
>> >> >>>>
>> >> >>>> Use would be limited to new AdapterFactory implementations
>>throwing
>> >> >>> RuntimeExcpetion. For Sling Models this would be the case.
>> >> >>>>
>> >> >>>> (2) Add a new adaptToOrThrow method, which is declared to throw
>>a
>> >> >>> RuntimeException and never return null: Either it can adapt or it
>> >> >>>throws.
>> >> >>> This would require a new interface Adaptable2 (probably) to not
>> >>break
>> >> >>> existing Adaptable implementations. The SlingAdaptable base class
>> >>would
>> >> >>> implement the new method of course, probably something like this:
>> >> >>>>
>> >> >>>>> SlingAdaptable implements Adaptable2 {
>> >> >>>>> Š
>> >> >>>>> public <AdapterType> AdapterType
>>adaptToOrThrow(Class<AdapterType>
>> >> >>> type) {
>> >> >>>>> AdapterType result = this.adaptTo(type);
>> >> >>>>> if (result != null) {
>> >> >>>>> return result;
>> >> >>>>> }
>> >> >>>>> throw new CannotAdaptException(Š);
>> >> >>>>> }
>> >> >>>>> }
>> >> >>>>>
>> >> >>>>
>> >> >>>> Use is problematic because you would have to know whether you
>>can
>> >>call
>> >> >>> the new method: So instead of an null check you now have an
>> >>instanceof
>> >> >>> check Š Except for the Resource interface which would be
>>extended to
>> >> >>>extend
>> >> >>> from Adaptable2 as well.
>> >> >>>>
>> >> >>>> (3) Document, that Adaptable.adaptTo may throw a
>>RuntimeException.
>> >> >>>>
>> >> >>>> The problem here is, that this may conceptually break existing
>> >>callers
>> >> >>> of Adaptable.adaptTo which don't expect an exception at all ‹
>> >> >>>presumably
>> >> >>> this is a minor nuisance because technically a RuntimeException
>>may
>> >> >>>always
>> >> >>> be thrown.
>> >> >>>>
>> >> >>>> Regards
>> >> >>>> Felix
>> >> >>>
>> >> >>>
>> >> >>
>> >> >>
>> >> >> --
>> >> >> Carsten Ziegeler
>> >> >> Adobe Research Switzerland
>> >> >> cziegeler@apache.org
>> >> >
>> >>
>> >>
>> >
>> >
>> >--
>> >Carsten Ziegeler
>> >Adobe Research Switzerland
>> >cziegeler@apache.org
>>
>>
>
>
>--
>Carsten Ziegeler
>Adobe Research Switzerland
>cziegeler@apache.org
Re: adaptTo and results ....
Posted by Carsten Ziegeler <cz...@apache.org>.
Sure :) For the adapter pattern, the client does not care why the adaption
failed, the client is just interested in the result (success or not)
Validation is a different beast, if validation fails you want to know
specific reasons why it failed - and this can be multiple.
I tried to explain in my first mail on this thread, that all other use
cases mentioned can be handled with the current implementation - with the
exception of validation. But I think validation requires a different
concept than the adapter pattern.
Carsten
2014-07-01 11:09 GMT+02:00 Jeff Young <je...@adobe.com>:
> Hi Carsten,
>
> Can you say more? (I'm not sure I understand what you're getting at....)
>
> Thanks,
> Jeff.
>
>
> On 01/07/2014 09:56, "Carsten Ziegeler" <cz...@apache.org> wrote:
>
> >adaption and validation are different concerns
> >
> >Carsten
> >
> >
> >2014-07-01 10:55 GMT+02:00 Jeff Young <je...@adobe.com>:
> >
> >> We could solve that by defining a specific exception for
> >> adaptation-not-possible and then catch only that.
> >>
> >> Of course that would leak tons of exceptions from code written before
> >>that
> >> exception became available. Maybe do the catching based on some sort of
> >> version clue?
> >>
> >> Cheers,
> >> Jeff.
> >>
> >>
> >> On 01/07/2014 09:40, "Konrad Windszus" <ko...@gmx.de> wrote:
> >>
> >> >It is not (only) about throwing exceptions in case no suitable adapter
> >>is
> >> >available. It rather is about the fact, that today the adaptTo is a
> >> >barrier for all kinds of exceptions. In some cases the adaptation fails
> >> >for a specific reason (one example is Sling Models where injection
> >>fails,
> >> >another one is the issue mentioned in
> >> >https://issues.apache.org/jira/browse/SLING-2712 (ValueMap not
> >>supporting
> >> >primitives)). Both would be valid use cases where the client would be
> >> >interested to catch the exception here.
> >> >
> >> >On 01 Jul 2014, at 10:34, Carsten Ziegeler <cz...@apache.org>
> >>wrote:
> >> >
> >> >> Adding a new interface would require us to implement it all over the
> >> >>place
> >> >> and as Felix points out, client code would always need to check
> >>whether
> >> >>the
> >> >> new interface is implemented or not Having to methods, like
> >>hasAdapter
> >> >>and
> >> >> adaptOrThrow does not work very well as between the two calls things
> >> >>might
> >> >> have changed already: while hasAdapter returns true, the underlying
> >> >>factory
> >> >> gets unregistered before adaptOrThrow is called.
> >> >> In many use cases, the current pattern works fine - the client does
> >>not
> >> >> care whether an exception is thrown within the adaption - it just
> >>cares
> >> >> whether an object is returned or not. And there are valid use cases,
> >> >>where
> >> >> client code does different things whether the adaption works or not
> >> >>(e.g.
> >> >> the post servlet checks for adaptTo(Node) and then does additional
> >> >>things
> >> >> if the resource is backed up by a node.)
> >> >>
> >> >> I see the point that there are also use cases where it would be fine
> >>to
> >> >> simpy throw an exception if adaptTo is not successful. This would
> >>make
> >> >>the
> >> >> client code easier. However as this most properly is a runtime
> >> >>exception,
> >> >> client code can today just call a method on the object and end up
> >>with a
> >> >> NPE - having the same result :)
> >> >>
> >> >> This leaves us with use cases where the client code explicitely
> >>wants to
> >> >> catch the exception and then do something depending on the exception.
> >> >>Maybe
> >> >> we should just add something for this explicit use case instead of
> >> >>bloating
> >> >> the general adaptTo mechanism?
> >> >>
> >> >> Regards
> >> >> Carsten
> >> >>
> >> >>
> >> >>
> >> >>
> >> >> 2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
> >> >>
> >> >>> Regarding 1) Having such a Result class would mean that all consumer
> >> >>>would
> >> >>> need to unwrap the exception first. So instead of being forced of
> >> >>> implementing a null-check (as with the old solution) one would need
> >>to
> >> >>> implement another check. I want to prevent such a burden to the
> >> >>>consumers.
> >> >>> Regarding 2) Since the client code knows on which object the
> >> >>> adaptToOrThrow is called I don¹t get why an instanceof check is
> >> >>>necessary.
> >> >>> Whether this object implements Adaptable2 is known at compile-time,
> >>so
> >> >>>you
> >> >>> do have the full IDE-support here.
> >> >>> Regarding 3) In that case I would no longer allow a null value to be
> >> >>> returned. One drawback is, that all the null checks are no longer
> >> >>>effective.
> >> >>>
> >> >>> IMHO solution 2) is the best. At the same time I would deprecate the
> >> >>>old
> >> >>> Adaptable, because I cannot come up with a real use-case where
> >> >>>returning
> >> >>> has an advantage. I would rather implement another method boolean
> >> >>> hasAdapter(Class<AdapterType> type) on the Adaptable2 interface.
> >> >>> Regards,
> >> >>> Konrad
> >> >>>
> >> >>> On 01 Jul 2014, at 09:07, Felix Meschberger <fm...@adobe.com>
> >> wrote:
> >> >>>
> >> >>>> Hi
> >> >>>>
> >> >>>> There currently are two issues floating around dealing with the
> >> >>>>question
> >> >>> of returning more information than just null from the
> >> >>> Adaptable.adaptTo(Class) method:
> >> >>> https://issues.apache.org/jira/browse/SLING-3714 and
> >> >>> https://issues.apache.org/jira/browse/SLING-3709. I think these
> >> >>>requests
> >> >>> warrant some discussion on the list.
> >> >>>>
> >> >>>> Background: adaptTo can be implemented by Adaptable implementations
> >> >>> themselves or, by extending from SlingAdaptable, they may defer to
> >>an
> >> >>> AdapterMananager. AdapterManager itself is extended by
> >>AdapterFactory
> >> >>> services. All these interfaces define an adaptTo method. All these
> >> >>>methods
> >> >>> return null if adaption is not possible and don't declare or
> >>document
> >> >>>to
> >> >>> throw an exception.
> >> >>>>
> >> >>>> While not explicitly documented as such, the intention is and was
> >>that
> >> >>> adaptTo never throws on the grounds that adaption may fail which is
> >> >>> considered a valid result and thus exceptions are not to be expected
> >> >>>and
> >> >>> handled.
> >> >>>>
> >> >>>> Hence all implementations of the methods generally
> >> >>> catch-and-log-but-don't-throw. Interestingly SlingAdaptable.adaptTo
> >>and
> >> >>> AdapterManagerImpl.getAdapter don't catch ‹ so any RuntimeException
> >> >>>thrown
> >> >>> from an AdapterFactory would be forwarded.
> >> >>>>
> >> >>>> Having said this there are options available:
> >> >>>>
> >> >>>> (1) Add support for a new Result<?> class. We would probably
> >>implement
> >> >>> this in the AdapterManager.getAdapter implementation explicitly
> >> >>>handling
> >> >>> this case because it would entail catching the adaptTo/getAdapter
> >> >>>calls to
> >> >>> get the exception (the Result.getError should return Throwable
> >> >>>probably not
> >> >>> Error)
> >> >>>>
> >> >>>> Use would be limited to new AdapterFactory implementations throwing
> >> >>> RuntimeExcpetion. For Sling Models this would be the case.
> >> >>>>
> >> >>>> (2) Add a new adaptToOrThrow method, which is declared to throw a
> >> >>> RuntimeException and never return null: Either it can adapt or it
> >> >>>throws.
> >> >>> This would require a new interface Adaptable2 (probably) to not
> >>break
> >> >>> existing Adaptable implementations. The SlingAdaptable base class
> >>would
> >> >>> implement the new method of course, probably something like this:
> >> >>>>
> >> >>>>> SlingAdaptable implements Adaptable2 {
> >> >>>>> Š
> >> >>>>> public <AdapterType> AdapterType adaptToOrThrow(Class<AdapterType>
> >> >>> type) {
> >> >>>>> AdapterType result = this.adaptTo(type);
> >> >>>>> if (result != null) {
> >> >>>>> return result;
> >> >>>>> }
> >> >>>>> throw new CannotAdaptException(Š);
> >> >>>>> }
> >> >>>>> }
> >> >>>>>
> >> >>>>
> >> >>>> Use is problematic because you would have to know whether you can
> >>call
> >> >>> the new method: So instead of an null check you now have an
> >>instanceof
> >> >>> check Š Except for the Resource interface which would be extended to
> >> >>>extend
> >> >>> from Adaptable2 as well.
> >> >>>>
> >> >>>> (3) Document, that Adaptable.adaptTo may throw a RuntimeException.
> >> >>>>
> >> >>>> The problem here is, that this may conceptually break existing
> >>callers
> >> >>> of Adaptable.adaptTo which don't expect an exception at all ‹
> >> >>>presumably
> >> >>> this is a minor nuisance because technically a RuntimeException may
> >> >>>always
> >> >>> be thrown.
> >> >>>>
> >> >>>> Regards
> >> >>>> Felix
> >> >>>
> >> >>>
> >> >>
> >> >>
> >> >> --
> >> >> Carsten Ziegeler
> >> >> Adobe Research Switzerland
> >> >> cziegeler@apache.org
> >> >
> >>
> >>
> >
> >
> >--
> >Carsten Ziegeler
> >Adobe Research Switzerland
> >cziegeler@apache.org
>
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: adaptTo and results ....
Posted by Jeff Young <je...@adobe.com>.
Hi Carsten,
Can you say more? (I'm not sure I understand what you're getting at....)
Thanks,
Jeff.
On 01/07/2014 09:56, "Carsten Ziegeler" <cz...@apache.org> wrote:
>adaption and validation are different concerns
>
>Carsten
>
>
>2014-07-01 10:55 GMT+02:00 Jeff Young <je...@adobe.com>:
>
>> We could solve that by defining a specific exception for
>> adaptation-not-possible and then catch only that.
>>
>> Of course that would leak tons of exceptions from code written before
>>that
>> exception became available. Maybe do the catching based on some sort of
>> version clue?
>>
>> Cheers,
>> Jeff.
>>
>>
>> On 01/07/2014 09:40, "Konrad Windszus" <ko...@gmx.de> wrote:
>>
>> >It is not (only) about throwing exceptions in case no suitable adapter
>>is
>> >available. It rather is about the fact, that today the adaptTo is a
>> >barrier for all kinds of exceptions. In some cases the adaptation fails
>> >for a specific reason (one example is Sling Models where injection
>>fails,
>> >another one is the issue mentioned in
>> >https://issues.apache.org/jira/browse/SLING-2712 (ValueMap not
>>supporting
>> >primitives)). Both would be valid use cases where the client would be
>> >interested to catch the exception here.
>> >
>> >On 01 Jul 2014, at 10:34, Carsten Ziegeler <cz...@apache.org>
>>wrote:
>> >
>> >> Adding a new interface would require us to implement it all over the
>> >>place
>> >> and as Felix points out, client code would always need to check
>>whether
>> >>the
>> >> new interface is implemented or not Having to methods, like
>>hasAdapter
>> >>and
>> >> adaptOrThrow does not work very well as between the two calls things
>> >>might
>> >> have changed already: while hasAdapter returns true, the underlying
>> >>factory
>> >> gets unregistered before adaptOrThrow is called.
>> >> In many use cases, the current pattern works fine - the client does
>>not
>> >> care whether an exception is thrown within the adaption - it just
>>cares
>> >> whether an object is returned or not. And there are valid use cases,
>> >>where
>> >> client code does different things whether the adaption works or not
>> >>(e.g.
>> >> the post servlet checks for adaptTo(Node) and then does additional
>> >>things
>> >> if the resource is backed up by a node.)
>> >>
>> >> I see the point that there are also use cases where it would be fine
>>to
>> >> simpy throw an exception if adaptTo is not successful. This would
>>make
>> >>the
>> >> client code easier. However as this most properly is a runtime
>> >>exception,
>> >> client code can today just call a method on the object and end up
>>with a
>> >> NPE - having the same result :)
>> >>
>> >> This leaves us with use cases where the client code explicitely
>>wants to
>> >> catch the exception and then do something depending on the exception.
>> >>Maybe
>> >> we should just add something for this explicit use case instead of
>> >>bloating
>> >> the general adaptTo mechanism?
>> >>
>> >> Regards
>> >> Carsten
>> >>
>> >>
>> >>
>> >>
>> >> 2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
>> >>
>> >>> Regarding 1) Having such a Result class would mean that all consumer
>> >>>would
>> >>> need to unwrap the exception first. So instead of being forced of
>> >>> implementing a null-check (as with the old solution) one would need
>>to
>> >>> implement another check. I want to prevent such a burden to the
>> >>>consumers.
>> >>> Regarding 2) Since the client code knows on which object the
>> >>> adaptToOrThrow is called I don¹t get why an instanceof check is
>> >>>necessary.
>> >>> Whether this object implements Adaptable2 is known at compile-time,
>>so
>> >>>you
>> >>> do have the full IDE-support here.
>> >>> Regarding 3) In that case I would no longer allow a null value to be
>> >>> returned. One drawback is, that all the null checks are no longer
>> >>>effective.
>> >>>
>> >>> IMHO solution 2) is the best. At the same time I would deprecate the
>> >>>old
>> >>> Adaptable, because I cannot come up with a real use-case where
>> >>>returning
>> >>> has an advantage. I would rather implement another method boolean
>> >>> hasAdapter(Class<AdapterType> type) on the Adaptable2 interface.
>> >>> Regards,
>> >>> Konrad
>> >>>
>> >>> On 01 Jul 2014, at 09:07, Felix Meschberger <fm...@adobe.com>
>> wrote:
>> >>>
>> >>>> Hi
>> >>>>
>> >>>> There currently are two issues floating around dealing with the
>> >>>>question
>> >>> of returning more information than just null from the
>> >>> Adaptable.adaptTo(Class) method:
>> >>> https://issues.apache.org/jira/browse/SLING-3714 and
>> >>> https://issues.apache.org/jira/browse/SLING-3709. I think these
>> >>>requests
>> >>> warrant some discussion on the list.
>> >>>>
>> >>>> Background: adaptTo can be implemented by Adaptable implementations
>> >>> themselves or, by extending from SlingAdaptable, they may defer to
>>an
>> >>> AdapterMananager. AdapterManager itself is extended by
>>AdapterFactory
>> >>> services. All these interfaces define an adaptTo method. All these
>> >>>methods
>> >>> return null if adaption is not possible and don't declare or
>>document
>> >>>to
>> >>> throw an exception.
>> >>>>
>> >>>> While not explicitly documented as such, the intention is and was
>>that
>> >>> adaptTo never throws on the grounds that adaption may fail which is
>> >>> considered a valid result and thus exceptions are not to be expected
>> >>>and
>> >>> handled.
>> >>>>
>> >>>> Hence all implementations of the methods generally
>> >>> catch-and-log-but-don't-throw. Interestingly SlingAdaptable.adaptTo
>>and
>> >>> AdapterManagerImpl.getAdapter don't catch ‹ so any RuntimeException
>> >>>thrown
>> >>> from an AdapterFactory would be forwarded.
>> >>>>
>> >>>> Having said this there are options available:
>> >>>>
>> >>>> (1) Add support for a new Result<?> class. We would probably
>>implement
>> >>> this in the AdapterManager.getAdapter implementation explicitly
>> >>>handling
>> >>> this case because it would entail catching the adaptTo/getAdapter
>> >>>calls to
>> >>> get the exception (the Result.getError should return Throwable
>> >>>probably not
>> >>> Error)
>> >>>>
>> >>>> Use would be limited to new AdapterFactory implementations throwing
>> >>> RuntimeExcpetion. For Sling Models this would be the case.
>> >>>>
>> >>>> (2) Add a new adaptToOrThrow method, which is declared to throw a
>> >>> RuntimeException and never return null: Either it can adapt or it
>> >>>throws.
>> >>> This would require a new interface Adaptable2 (probably) to not
>>break
>> >>> existing Adaptable implementations. The SlingAdaptable base class
>>would
>> >>> implement the new method of course, probably something like this:
>> >>>>
>> >>>>> SlingAdaptable implements Adaptable2 {
>> >>>>> Š
>> >>>>> public <AdapterType> AdapterType adaptToOrThrow(Class<AdapterType>
>> >>> type) {
>> >>>>> AdapterType result = this.adaptTo(type);
>> >>>>> if (result != null) {
>> >>>>> return result;
>> >>>>> }
>> >>>>> throw new CannotAdaptException(Š);
>> >>>>> }
>> >>>>> }
>> >>>>>
>> >>>>
>> >>>> Use is problematic because you would have to know whether you can
>>call
>> >>> the new method: So instead of an null check you now have an
>>instanceof
>> >>> check Š Except for the Resource interface which would be extended to
>> >>>extend
>> >>> from Adaptable2 as well.
>> >>>>
>> >>>> (3) Document, that Adaptable.adaptTo may throw a RuntimeException.
>> >>>>
>> >>>> The problem here is, that this may conceptually break existing
>>callers
>> >>> of Adaptable.adaptTo which don't expect an exception at all ‹
>> >>>presumably
>> >>> this is a minor nuisance because technically a RuntimeException may
>> >>>always
>> >>> be thrown.
>> >>>>
>> >>>> Regards
>> >>>> Felix
>> >>>
>> >>>
>> >>
>> >>
>> >> --
>> >> Carsten Ziegeler
>> >> Adobe Research Switzerland
>> >> cziegeler@apache.org
>> >
>>
>>
>
>
>--
>Carsten Ziegeler
>Adobe Research Switzerland
>cziegeler@apache.org
Re: adaptTo and results ....
Posted by Carsten Ziegeler <cz...@apache.org>.
adaption and validation are different concerns
Carsten
2014-07-01 10:55 GMT+02:00 Jeff Young <je...@adobe.com>:
> We could solve that by defining a specific exception for
> adaptation-not-possible and then catch only that.
>
> Of course that would leak tons of exceptions from code written before that
> exception became available. Maybe do the catching based on some sort of
> version clue?
>
> Cheers,
> Jeff.
>
>
> On 01/07/2014 09:40, "Konrad Windszus" <ko...@gmx.de> wrote:
>
> >It is not (only) about throwing exceptions in case no suitable adapter is
> >available. It rather is about the fact, that today the adaptTo is a
> >barrier for all kinds of exceptions. In some cases the adaptation fails
> >for a specific reason (one example is Sling Models where injection fails,
> >another one is the issue mentioned in
> >https://issues.apache.org/jira/browse/SLING-2712 (ValueMap not supporting
> >primitives)). Both would be valid use cases where the client would be
> >interested to catch the exception here.
> >
> >On 01 Jul 2014, at 10:34, Carsten Ziegeler <cz...@apache.org> wrote:
> >
> >> Adding a new interface would require us to implement it all over the
> >>place
> >> and as Felix points out, client code would always need to check whether
> >>the
> >> new interface is implemented or not Having to methods, like hasAdapter
> >>and
> >> adaptOrThrow does not work very well as between the two calls things
> >>might
> >> have changed already: while hasAdapter returns true, the underlying
> >>factory
> >> gets unregistered before adaptOrThrow is called.
> >> In many use cases, the current pattern works fine - the client does not
> >> care whether an exception is thrown within the adaption - it just cares
> >> whether an object is returned or not. And there are valid use cases,
> >>where
> >> client code does different things whether the adaption works or not
> >>(e.g.
> >> the post servlet checks for adaptTo(Node) and then does additional
> >>things
> >> if the resource is backed up by a node.)
> >>
> >> I see the point that there are also use cases where it would be fine to
> >> simpy throw an exception if adaptTo is not successful. This would make
> >>the
> >> client code easier. However as this most properly is a runtime
> >>exception,
> >> client code can today just call a method on the object and end up with a
> >> NPE - having the same result :)
> >>
> >> This leaves us with use cases where the client code explicitely wants to
> >> catch the exception and then do something depending on the exception.
> >>Maybe
> >> we should just add something for this explicit use case instead of
> >>bloating
> >> the general adaptTo mechanism?
> >>
> >> Regards
> >> Carsten
> >>
> >>
> >>
> >>
> >> 2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
> >>
> >>> Regarding 1) Having such a Result class would mean that all consumer
> >>>would
> >>> need to unwrap the exception first. So instead of being forced of
> >>> implementing a null-check (as with the old solution) one would need to
> >>> implement another check. I want to prevent such a burden to the
> >>>consumers.
> >>> Regarding 2) Since the client code knows on which object the
> >>> adaptToOrThrow is called I don¹t get why an instanceof check is
> >>>necessary.
> >>> Whether this object implements Adaptable2 is known at compile-time, so
> >>>you
> >>> do have the full IDE-support here.
> >>> Regarding 3) In that case I would no longer allow a null value to be
> >>> returned. One drawback is, that all the null checks are no longer
> >>>effective.
> >>>
> >>> IMHO solution 2) is the best. At the same time I would deprecate the
> >>>old
> >>> Adaptable, because I cannot come up with a real use-case where
> >>>returning
> >>> has an advantage. I would rather implement another method boolean
> >>> hasAdapter(Class<AdapterType> type) on the Adaptable2 interface.
> >>> Regards,
> >>> Konrad
> >>>
> >>> On 01 Jul 2014, at 09:07, Felix Meschberger <fm...@adobe.com>
> wrote:
> >>>
> >>>> Hi
> >>>>
> >>>> There currently are two issues floating around dealing with the
> >>>>question
> >>> of returning more information than just null from the
> >>> Adaptable.adaptTo(Class) method:
> >>> https://issues.apache.org/jira/browse/SLING-3714 and
> >>> https://issues.apache.org/jira/browse/SLING-3709. I think these
> >>>requests
> >>> warrant some discussion on the list.
> >>>>
> >>>> Background: adaptTo can be implemented by Adaptable implementations
> >>> themselves or, by extending from SlingAdaptable, they may defer to an
> >>> AdapterMananager. AdapterManager itself is extended by AdapterFactory
> >>> services. All these interfaces define an adaptTo method. All these
> >>>methods
> >>> return null if adaption is not possible and don't declare or document
> >>>to
> >>> throw an exception.
> >>>>
> >>>> While not explicitly documented as such, the intention is and was that
> >>> adaptTo never throws on the grounds that adaption may fail which is
> >>> considered a valid result and thus exceptions are not to be expected
> >>>and
> >>> handled.
> >>>>
> >>>> Hence all implementations of the methods generally
> >>> catch-and-log-but-don't-throw. Interestingly SlingAdaptable.adaptTo and
> >>> AdapterManagerImpl.getAdapter don't catch ‹ so any RuntimeException
> >>>thrown
> >>> from an AdapterFactory would be forwarded.
> >>>>
> >>>> Having said this there are options available:
> >>>>
> >>>> (1) Add support for a new Result<?> class. We would probably implement
> >>> this in the AdapterManager.getAdapter implementation explicitly
> >>>handling
> >>> this case because it would entail catching the adaptTo/getAdapter
> >>>calls to
> >>> get the exception (the Result.getError should return Throwable
> >>>probably not
> >>> Error)
> >>>>
> >>>> Use would be limited to new AdapterFactory implementations throwing
> >>> RuntimeExcpetion. For Sling Models this would be the case.
> >>>>
> >>>> (2) Add a new adaptToOrThrow method, which is declared to throw a
> >>> RuntimeException and never return null: Either it can adapt or it
> >>>throws.
> >>> This would require a new interface Adaptable2 (probably) to not break
> >>> existing Adaptable implementations. The SlingAdaptable base class would
> >>> implement the new method of course, probably something like this:
> >>>>
> >>>>> SlingAdaptable implements Adaptable2 {
> >>>>> Š
> >>>>> public <AdapterType> AdapterType adaptToOrThrow(Class<AdapterType>
> >>> type) {
> >>>>> AdapterType result = this.adaptTo(type);
> >>>>> if (result != null) {
> >>>>> return result;
> >>>>> }
> >>>>> throw new CannotAdaptException(Š);
> >>>>> }
> >>>>> }
> >>>>>
> >>>>
> >>>> Use is problematic because you would have to know whether you can call
> >>> the new method: So instead of an null check you now have an instanceof
> >>> check Š Except for the Resource interface which would be extended to
> >>>extend
> >>> from Adaptable2 as well.
> >>>>
> >>>> (3) Document, that Adaptable.adaptTo may throw a RuntimeException.
> >>>>
> >>>> The problem here is, that this may conceptually break existing callers
> >>> of Adaptable.adaptTo which don't expect an exception at all ‹
> >>>presumably
> >>> this is a minor nuisance because technically a RuntimeException may
> >>>always
> >>> be thrown.
> >>>>
> >>>> Regards
> >>>> Felix
> >>>
> >>>
> >>
> >>
> >> --
> >> Carsten Ziegeler
> >> Adobe Research Switzerland
> >> cziegeler@apache.org
> >
>
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: adaptTo and results ....
Posted by Jeff Young <je...@adobe.com>.
We could solve that by defining a specific exception for
adaptation-not-possible and then catch only that.
Of course that would leak tons of exceptions from code written before that
exception became available. Maybe do the catching based on some sort of
version clue?
Cheers,
Jeff.
On 01/07/2014 09:40, "Konrad Windszus" <ko...@gmx.de> wrote:
>It is not (only) about throwing exceptions in case no suitable adapter is
>available. It rather is about the fact, that today the adaptTo is a
>barrier for all kinds of exceptions. In some cases the adaptation fails
>for a specific reason (one example is Sling Models where injection fails,
>another one is the issue mentioned in
>https://issues.apache.org/jira/browse/SLING-2712 (ValueMap not supporting
>primitives)). Both would be valid use cases where the client would be
>interested to catch the exception here.
>
>On 01 Jul 2014, at 10:34, Carsten Ziegeler <cz...@apache.org> wrote:
>
>> Adding a new interface would require us to implement it all over the
>>place
>> and as Felix points out, client code would always need to check whether
>>the
>> new interface is implemented or not Having to methods, like hasAdapter
>>and
>> adaptOrThrow does not work very well as between the two calls things
>>might
>> have changed already: while hasAdapter returns true, the underlying
>>factory
>> gets unregistered before adaptOrThrow is called.
>> In many use cases, the current pattern works fine - the client does not
>> care whether an exception is thrown within the adaption - it just cares
>> whether an object is returned or not. And there are valid use cases,
>>where
>> client code does different things whether the adaption works or not
>>(e.g.
>> the post servlet checks for adaptTo(Node) and then does additional
>>things
>> if the resource is backed up by a node.)
>>
>> I see the point that there are also use cases where it would be fine to
>> simpy throw an exception if adaptTo is not successful. This would make
>>the
>> client code easier. However as this most properly is a runtime
>>exception,
>> client code can today just call a method on the object and end up with a
>> NPE - having the same result :)
>>
>> This leaves us with use cases where the client code explicitely wants to
>> catch the exception and then do something depending on the exception.
>>Maybe
>> we should just add something for this explicit use case instead of
>>bloating
>> the general adaptTo mechanism?
>>
>> Regards
>> Carsten
>>
>>
>>
>>
>> 2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
>>
>>> Regarding 1) Having such a Result class would mean that all consumer
>>>would
>>> need to unwrap the exception first. So instead of being forced of
>>> implementing a null-check (as with the old solution) one would need to
>>> implement another check. I want to prevent such a burden to the
>>>consumers.
>>> Regarding 2) Since the client code knows on which object the
>>> adaptToOrThrow is called I don¹t get why an instanceof check is
>>>necessary.
>>> Whether this object implements Adaptable2 is known at compile-time, so
>>>you
>>> do have the full IDE-support here.
>>> Regarding 3) In that case I would no longer allow a null value to be
>>> returned. One drawback is, that all the null checks are no longer
>>>effective.
>>>
>>> IMHO solution 2) is the best. At the same time I would deprecate the
>>>old
>>> Adaptable, because I cannot come up with a real use-case where
>>>returning
>>> has an advantage. I would rather implement another method boolean
>>> hasAdapter(Class<AdapterType> type) on the Adaptable2 interface.
>>> Regards,
>>> Konrad
>>>
>>> On 01 Jul 2014, at 09:07, Felix Meschberger <fm...@adobe.com> wrote:
>>>
>>>> Hi
>>>>
>>>> There currently are two issues floating around dealing with the
>>>>question
>>> of returning more information than just null from the
>>> Adaptable.adaptTo(Class) method:
>>> https://issues.apache.org/jira/browse/SLING-3714 and
>>> https://issues.apache.org/jira/browse/SLING-3709. I think these
>>>requests
>>> warrant some discussion on the list.
>>>>
>>>> Background: adaptTo can be implemented by Adaptable implementations
>>> themselves or, by extending from SlingAdaptable, they may defer to an
>>> AdapterMananager. AdapterManager itself is extended by AdapterFactory
>>> services. All these interfaces define an adaptTo method. All these
>>>methods
>>> return null if adaption is not possible and don't declare or document
>>>to
>>> throw an exception.
>>>>
>>>> While not explicitly documented as such, the intention is and was that
>>> adaptTo never throws on the grounds that adaption may fail which is
>>> considered a valid result and thus exceptions are not to be expected
>>>and
>>> handled.
>>>>
>>>> Hence all implementations of the methods generally
>>> catch-and-log-but-don't-throw. Interestingly SlingAdaptable.adaptTo and
>>> AdapterManagerImpl.getAdapter don't catch ‹ so any RuntimeException
>>>thrown
>>> from an AdapterFactory would be forwarded.
>>>>
>>>> Having said this there are options available:
>>>>
>>>> (1) Add support for a new Result<?> class. We would probably implement
>>> this in the AdapterManager.getAdapter implementation explicitly
>>>handling
>>> this case because it would entail catching the adaptTo/getAdapter
>>>calls to
>>> get the exception (the Result.getError should return Throwable
>>>probably not
>>> Error)
>>>>
>>>> Use would be limited to new AdapterFactory implementations throwing
>>> RuntimeExcpetion. For Sling Models this would be the case.
>>>>
>>>> (2) Add a new adaptToOrThrow method, which is declared to throw a
>>> RuntimeException and never return null: Either it can adapt or it
>>>throws.
>>> This would require a new interface Adaptable2 (probably) to not break
>>> existing Adaptable implementations. The SlingAdaptable base class would
>>> implement the new method of course, probably something like this:
>>>>
>>>>> SlingAdaptable implements Adaptable2 {
>>>>> Š
>>>>> public <AdapterType> AdapterType adaptToOrThrow(Class<AdapterType>
>>> type) {
>>>>> AdapterType result = this.adaptTo(type);
>>>>> if (result != null) {
>>>>> return result;
>>>>> }
>>>>> throw new CannotAdaptException(Š);
>>>>> }
>>>>> }
>>>>>
>>>>
>>>> Use is problematic because you would have to know whether you can call
>>> the new method: So instead of an null check you now have an instanceof
>>> check Š Except for the Resource interface which would be extended to
>>>extend
>>> from Adaptable2 as well.
>>>>
>>>> (3) Document, that Adaptable.adaptTo may throw a RuntimeException.
>>>>
>>>> The problem here is, that this may conceptually break existing callers
>>> of Adaptable.adaptTo which don't expect an exception at all ‹
>>>presumably
>>> this is a minor nuisance because technically a RuntimeException may
>>>always
>>> be thrown.
>>>>
>>>> Regards
>>>> Felix
>>>
>>>
>>
>>
>> --
>> Carsten Ziegeler
>> Adobe Research Switzerland
>> cziegeler@apache.org
>
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
It is not (only) about throwing exceptions in case no suitable adapter is available. It rather is about the fact, that today the adaptTo is a barrier for all kinds of exceptions. In some cases the adaptation fails for a specific reason (one example is Sling Models where injection fails, another one is the issue mentioned in https://issues.apache.org/jira/browse/SLING-2712 (ValueMap not supporting primitives)). Both would be valid use cases where the client would be interested to catch the exception here.
On 01 Jul 2014, at 10:34, Carsten Ziegeler <cz...@apache.org> wrote:
> Adding a new interface would require us to implement it all over the place
> and as Felix points out, client code would always need to check whether the
> new interface is implemented or not Having to methods, like hasAdapter and
> adaptOrThrow does not work very well as between the two calls things might
> have changed already: while hasAdapter returns true, the underlying factory
> gets unregistered before adaptOrThrow is called.
> In many use cases, the current pattern works fine - the client does not
> care whether an exception is thrown within the adaption - it just cares
> whether an object is returned or not. And there are valid use cases, where
> client code does different things whether the adaption works or not (e.g.
> the post servlet checks for adaptTo(Node) and then does additional things
> if the resource is backed up by a node.)
>
> I see the point that there are also use cases where it would be fine to
> simpy throw an exception if adaptTo is not successful. This would make the
> client code easier. However as this most properly is a runtime exception,
> client code can today just call a method on the object and end up with a
> NPE - having the same result :)
>
> This leaves us with use cases where the client code explicitely wants to
> catch the exception and then do something depending on the exception. Maybe
> we should just add something for this explicit use case instead of bloating
> the general adaptTo mechanism?
>
> Regards
> Carsten
>
>
>
>
> 2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
>
>> Regarding 1) Having such a Result class would mean that all consumer would
>> need to unwrap the exception first. So instead of being forced of
>> implementing a null-check (as with the old solution) one would need to
>> implement another check. I want to prevent such a burden to the consumers.
>> Regarding 2) Since the client code knows on which object the
>> adaptToOrThrow is called I don’t get why an instanceof check is necessary.
>> Whether this object implements Adaptable2 is known at compile-time, so you
>> do have the full IDE-support here.
>> Regarding 3) In that case I would no longer allow a null value to be
>> returned. One drawback is, that all the null checks are no longer effective.
>>
>> IMHO solution 2) is the best. At the same time I would deprecate the old
>> Adaptable, because I cannot come up with a real use-case where returning
>> has an advantage. I would rather implement another method boolean
>> hasAdapter(Class<AdapterType> type) on the Adaptable2 interface.
>> Regards,
>> Konrad
>>
>> On 01 Jul 2014, at 09:07, Felix Meschberger <fm...@adobe.com> wrote:
>>
>>> Hi
>>>
>>> There currently are two issues floating around dealing with the question
>> of returning more information than just null from the
>> Adaptable.adaptTo(Class) method:
>> https://issues.apache.org/jira/browse/SLING-3714 and
>> https://issues.apache.org/jira/browse/SLING-3709. I think these requests
>> warrant some discussion on the list.
>>>
>>> Background: adaptTo can be implemented by Adaptable implementations
>> themselves or, by extending from SlingAdaptable, they may defer to an
>> AdapterMananager. AdapterManager itself is extended by AdapterFactory
>> services. All these interfaces define an adaptTo method. All these methods
>> return null if adaption is not possible and don't declare or document to
>> throw an exception.
>>>
>>> While not explicitly documented as such, the intention is and was that
>> adaptTo never throws on the grounds that adaption may fail which is
>> considered a valid result and thus exceptions are not to be expected and
>> handled.
>>>
>>> Hence all implementations of the methods generally
>> catch-and-log-but-don't-throw. Interestingly SlingAdaptable.adaptTo and
>> AdapterManagerImpl.getAdapter don't catch — so any RuntimeException thrown
>> from an AdapterFactory would be forwarded.
>>>
>>> Having said this there are options available:
>>>
>>> (1) Add support for a new Result<?> class. We would probably implement
>> this in the AdapterManager.getAdapter implementation explicitly handling
>> this case because it would entail catching the adaptTo/getAdapter calls to
>> get the exception (the Result.getError should return Throwable probably not
>> Error)
>>>
>>> Use would be limited to new AdapterFactory implementations throwing
>> RuntimeExcpetion. For Sling Models this would be the case.
>>>
>>> (2) Add a new adaptToOrThrow method, which is declared to throw a
>> RuntimeException and never return null: Either it can adapt or it throws.
>> This would require a new interface Adaptable2 (probably) to not break
>> existing Adaptable implementations. The SlingAdaptable base class would
>> implement the new method of course, probably something like this:
>>>
>>>> SlingAdaptable implements Adaptable2 {
>>>> …
>>>> public <AdapterType> AdapterType adaptToOrThrow(Class<AdapterType>
>> type) {
>>>> AdapterType result = this.adaptTo(type);
>>>> if (result != null) {
>>>> return result;
>>>> }
>>>> throw new CannotAdaptException(…);
>>>> }
>>>> }
>>>>
>>>
>>> Use is problematic because you would have to know whether you can call
>> the new method: So instead of an null check you now have an instanceof
>> check … Except for the Resource interface which would be extended to extend
>> from Adaptable2 as well.
>>>
>>> (3) Document, that Adaptable.adaptTo may throw a RuntimeException.
>>>
>>> The problem here is, that this may conceptually break existing callers
>> of Adaptable.adaptTo which don't expect an exception at all — presumably
>> this is a minor nuisance because technically a RuntimeException may always
>> be thrown.
>>>
>>> Regards
>>> Felix
>>
>>
>
>
> --
> Carsten Ziegeler
> Adobe Research Switzerland
> cziegeler@apache.org
Re: adaptTo and results ....
Posted by Carsten Ziegeler <cz...@apache.org>.
Adding a new interface would require us to implement it all over the place
and as Felix points out, client code would always need to check whether the
new interface is implemented or not Having to methods, like hasAdapter and
adaptOrThrow does not work very well as between the two calls things might
have changed already: while hasAdapter returns true, the underlying factory
gets unregistered before adaptOrThrow is called.
In many use cases, the current pattern works fine - the client does not
care whether an exception is thrown within the adaption - it just cares
whether an object is returned or not. And there are valid use cases, where
client code does different things whether the adaption works or not (e.g.
the post servlet checks for adaptTo(Node) and then does additional things
if the resource is backed up by a node.)
I see the point that there are also use cases where it would be fine to
simpy throw an exception if adaptTo is not successful. This would make the
client code easier. However as this most properly is a runtime exception,
client code can today just call a method on the object and end up with a
NPE - having the same result :)
This leaves us with use cases where the client code explicitely wants to
catch the exception and then do something depending on the exception. Maybe
we should just add something for this explicit use case instead of bloating
the general adaptTo mechanism?
Regards
Carsten
2014-07-01 9:44 GMT+02:00 Konrad Windszus <ko...@gmx.de>:
> Regarding 1) Having such a Result class would mean that all consumer would
> need to unwrap the exception first. So instead of being forced of
> implementing a null-check (as with the old solution) one would need to
> implement another check. I want to prevent such a burden to the consumers.
> Regarding 2) Since the client code knows on which object the
> adaptToOrThrow is called I don’t get why an instanceof check is necessary.
> Whether this object implements Adaptable2 is known at compile-time, so you
> do have the full IDE-support here.
> Regarding 3) In that case I would no longer allow a null value to be
> returned. One drawback is, that all the null checks are no longer effective.
>
> IMHO solution 2) is the best. At the same time I would deprecate the old
> Adaptable, because I cannot come up with a real use-case where returning
> has an advantage. I would rather implement another method boolean
> hasAdapter(Class<AdapterType> type) on the Adaptable2 interface.
> Regards,
> Konrad
>
> On 01 Jul 2014, at 09:07, Felix Meschberger <fm...@adobe.com> wrote:
>
> > Hi
> >
> > There currently are two issues floating around dealing with the question
> of returning more information than just null from the
> Adaptable.adaptTo(Class) method:
> https://issues.apache.org/jira/browse/SLING-3714 and
> https://issues.apache.org/jira/browse/SLING-3709. I think these requests
> warrant some discussion on the list.
> >
> > Background: adaptTo can be implemented by Adaptable implementations
> themselves or, by extending from SlingAdaptable, they may defer to an
> AdapterMananager. AdapterManager itself is extended by AdapterFactory
> services. All these interfaces define an adaptTo method. All these methods
> return null if adaption is not possible and don't declare or document to
> throw an exception.
> >
> > While not explicitly documented as such, the intention is and was that
> adaptTo never throws on the grounds that adaption may fail which is
> considered a valid result and thus exceptions are not to be expected and
> handled.
> >
> > Hence all implementations of the methods generally
> catch-and-log-but-don't-throw. Interestingly SlingAdaptable.adaptTo and
> AdapterManagerImpl.getAdapter don't catch — so any RuntimeException thrown
> from an AdapterFactory would be forwarded.
> >
> > Having said this there are options available:
> >
> > (1) Add support for a new Result<?> class. We would probably implement
> this in the AdapterManager.getAdapter implementation explicitly handling
> this case because it would entail catching the adaptTo/getAdapter calls to
> get the exception (the Result.getError should return Throwable probably not
> Error)
> >
> > Use would be limited to new AdapterFactory implementations throwing
> RuntimeExcpetion. For Sling Models this would be the case.
> >
> > (2) Add a new adaptToOrThrow method, which is declared to throw a
> RuntimeException and never return null: Either it can adapt or it throws.
> This would require a new interface Adaptable2 (probably) to not break
> existing Adaptable implementations. The SlingAdaptable base class would
> implement the new method of course, probably something like this:
> >
> >> SlingAdaptable implements Adaptable2 {
> >> …
> >> public <AdapterType> AdapterType adaptToOrThrow(Class<AdapterType>
> type) {
> >> AdapterType result = this.adaptTo(type);
> >> if (result != null) {
> >> return result;
> >> }
> >> throw new CannotAdaptException(…);
> >> }
> >> }
> >>
> >
> > Use is problematic because you would have to know whether you can call
> the new method: So instead of an null check you now have an instanceof
> check … Except for the Resource interface which would be extended to extend
> from Adaptable2 as well.
> >
> > (3) Document, that Adaptable.adaptTo may throw a RuntimeException.
> >
> > The problem here is, that this may conceptually break existing callers
> of Adaptable.adaptTo which don't expect an exception at all — presumably
> this is a minor nuisance because technically a RuntimeException may always
> be thrown.
> >
> > Regards
> > Felix
>
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
Re: adaptTo and results ....
Posted by Jeff Young <je...@adobe.com>.
adaptTo() is currently commonly used as a test, similar to instanceof.
Throwing and catching to return null is a very poor implementation
(performance-wise) for this use.
Adding a hasAdapter() or canAdaptTo() might decrease the number of
implementations that think throwing is OK, but only if the new interface
is visible at the level of the specific implementation. (If they're just
done as wrappers with catches somewhere in the machinery, and the
AdapterFactory writer is unaware of them, then they probably don't help
this issue.)
Cheers,
Jeff.
On 01/07/2014 08:44, "Konrad Windszus" <ko...@gmx.de> wrote:
>Regarding 1) Having such a Result class would mean that all consumer
>would need to unwrap the exception first. So instead of being forced of
>implementing a null-check (as with the old solution) one would need to
>implement another check. I want to prevent such a burden to the consumers.
>Regarding 2) Since the client code knows on which object the
>adaptToOrThrow is called I don¹t get why an instanceof check is
>necessary. Whether this object implements Adaptable2 is known at
>compile-time, so you do have the full IDE-support here.
>Regarding 3) In that case I would no longer allow a null value to be
>returned. One drawback is, that all the null checks are no longer
>effective.
>
>IMHO solution 2) is the best. At the same time I would deprecate the old
>Adaptable, because I cannot come up with a real use-case where returning
>has an advantage. I would rather implement another method boolean
>hasAdapter(Class<AdapterType> type) on the Adaptable2 interface.
>Regards,
>Konrad
>
>On 01 Jul 2014, at 09:07, Felix Meschberger <fm...@adobe.com> wrote:
>
>> Hi
>>
>> There currently are two issues floating around dealing with the
>>question of returning more information than just null from the
>>Adaptable.adaptTo(Class) method:
>>https://issues.apache.org/jira/browse/SLING-3714 and
>>https://issues.apache.org/jira/browse/SLING-3709. I think these requests
>>warrant some discussion on the list.
>>
>> Background: adaptTo can be implemented by Adaptable implementations
>>themselves or, by extending from SlingAdaptable, they may defer to an
>>AdapterMananager. AdapterManager itself is extended by AdapterFactory
>>services. All these interfaces define an adaptTo method. All these
>>methods return null if adaption is not possible and don't declare or
>>document to throw an exception.
>>
>> While not explicitly documented as such, the intention is and was that
>>adaptTo never throws on the grounds that adaption may fail which is
>>considered a valid result and thus exceptions are not to be expected and
>>handled.
>>
>> Hence all implementations of the methods generally
>>catch-and-log-but-don't-throw. Interestingly SlingAdaptable.adaptTo and
>>AdapterManagerImpl.getAdapter don't catch ‹ so any RuntimeException
>>thrown from an AdapterFactory would be forwarded.
>>
>> Having said this there are options available:
>>
>> (1) Add support for a new Result<?> class. We would probably implement
>>this in the AdapterManager.getAdapter implementation explicitly handling
>>this case because it would entail catching the adaptTo/getAdapter calls
>>to get the exception (the Result.getError should return Throwable
>>probably not Error)
>>
>> Use would be limited to new AdapterFactory implementations throwing
>>RuntimeExcpetion. For Sling Models this would be the case.
>>
>> (2) Add a new adaptToOrThrow method, which is declared to throw a
>>RuntimeException and never return null: Either it can adapt or it
>>throws. This would require a new interface Adaptable2 (probably) to not
>>break existing Adaptable implementations. The SlingAdaptable base class
>>would implement the new method of course, probably something like this:
>>
>>> SlingAdaptable implements Adaptable2 {
>>> Š
>>> public <AdapterType> AdapterType adaptToOrThrow(Class<AdapterType>
>>>type) {
>>> AdapterType result = this.adaptTo(type);
>>> if (result != null) {
>>> return result;
>>> }
>>> throw new CannotAdaptException(Š);
>>> }
>>> }
>>>
>>
>> Use is problematic because you would have to know whether you can call
>>the new method: So instead of an null check you now have an instanceof
>>check Š Except for the Resource interface which would be extended to
>>extend from Adaptable2 as well.
>>
>> (3) Document, that Adaptable.adaptTo may throw a RuntimeException.
>>
>> The problem here is, that this may conceptually break existing callers
>>of Adaptable.adaptTo which don't expect an exception at all ‹ presumably
>>this is a minor nuisance because technically a RuntimeException may
>>always be thrown.
>>
>> Regards
>> Felix
>
Re: adaptTo and results ....
Posted by Konrad Windszus <ko...@gmx.de>.
Regarding 1) Having such a Result class would mean that all consumer would need to unwrap the exception first. So instead of being forced of implementing a null-check (as with the old solution) one would need to implement another check. I want to prevent such a burden to the consumers.
Regarding 2) Since the client code knows on which object the adaptToOrThrow is called I don’t get why an instanceof check is necessary. Whether this object implements Adaptable2 is known at compile-time, so you do have the full IDE-support here.
Regarding 3) In that case I would no longer allow a null value to be returned. One drawback is, that all the null checks are no longer effective.
IMHO solution 2) is the best. At the same time I would deprecate the old Adaptable, because I cannot come up with a real use-case where returning has an advantage. I would rather implement another method boolean hasAdapter(Class<AdapterType> type) on the Adaptable2 interface.
Regards,
Konrad
On 01 Jul 2014, at 09:07, Felix Meschberger <fm...@adobe.com> wrote:
> Hi
>
> There currently are two issues floating around dealing with the question of returning more information than just null from the Adaptable.adaptTo(Class) method: https://issues.apache.org/jira/browse/SLING-3714 and https://issues.apache.org/jira/browse/SLING-3709. I think these requests warrant some discussion on the list.
>
> Background: adaptTo can be implemented by Adaptable implementations themselves or, by extending from SlingAdaptable, they may defer to an AdapterMananager. AdapterManager itself is extended by AdapterFactory services. All these interfaces define an adaptTo method. All these methods return null if adaption is not possible and don't declare or document to throw an exception.
>
> While not explicitly documented as such, the intention is and was that adaptTo never throws on the grounds that adaption may fail which is considered a valid result and thus exceptions are not to be expected and handled.
>
> Hence all implementations of the methods generally catch-and-log-but-don't-throw. Interestingly SlingAdaptable.adaptTo and AdapterManagerImpl.getAdapter don't catch — so any RuntimeException thrown from an AdapterFactory would be forwarded.
>
> Having said this there are options available:
>
> (1) Add support for a new Result<?> class. We would probably implement this in the AdapterManager.getAdapter implementation explicitly handling this case because it would entail catching the adaptTo/getAdapter calls to get the exception (the Result.getError should return Throwable probably not Error)
>
> Use would be limited to new AdapterFactory implementations throwing RuntimeExcpetion. For Sling Models this would be the case.
>
> (2) Add a new adaptToOrThrow method, which is declared to throw a RuntimeException and never return null: Either it can adapt or it throws. This would require a new interface Adaptable2 (probably) to not break existing Adaptable implementations. The SlingAdaptable base class would implement the new method of course, probably something like this:
>
>> SlingAdaptable implements Adaptable2 {
>> …
>> public <AdapterType> AdapterType adaptToOrThrow(Class<AdapterType> type) {
>> AdapterType result = this.adaptTo(type);
>> if (result != null) {
>> return result;
>> }
>> throw new CannotAdaptException(…);
>> }
>> }
>>
>
> Use is problematic because you would have to know whether you can call the new method: So instead of an null check you now have an instanceof check … Except for the Resource interface which would be extended to extend from Adaptable2 as well.
>
> (3) Document, that Adaptable.adaptTo may throw a RuntimeException.
>
> The problem here is, that this may conceptually break existing callers of Adaptable.adaptTo which don't expect an exception at all — presumably this is a minor nuisance because technically a RuntimeException may always be thrown.
>
> Regards
> Felix