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
