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
