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
