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
