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