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:cziege...@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 <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
