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
