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 <sseif...@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:cziege...@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 <j...@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" <cziege...@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 <j...@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" <cziege...@apache.org> > wrote: > >> >> > >> >> >adaption and validation are different concerns > >> >> > > >> >> >Carsten > >> >> > > >> >> > > >> >> >2014-07-01 10:55 GMT+02:00 Jeff Young <j...@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" <konra...@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 <cziege...@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 <konra...@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 < > fmesc...@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 > >> >> >> >> cziege...@apache.org > >> >> >> > > >> >> >> > >> >> >> > >> >> > > >> >> > > >> >> >-- > >> >> >Carsten Ziegeler > >> >> >Adobe Research Switzerland > >> >> >cziege...@apache.org > >> >> > >> >> > >> > > >> > > >> >-- > >> >Carsten Ziegeler > >> >Adobe Research Switzerland > >> >cziege...@apache.org > >> > >> > > > > > >-- > >Carsten Ziegeler > >Adobe Research Switzerland > >cziege...@apache.org > -- Carsten Ziegeler Adobe Research Switzerland cziege...@apache.org
