adaptTo() is currently commonly used as a test, similar to instanceof. Throwing and catching to return null is a very poor implementation (performance-wise) for this use.
Adding a hasAdapter() or canAdaptTo() might decrease the number of implementations that think throwing is OK, but only if the new interface is visible at the level of the specific implementation. (If they're just done as wrappers with catches somewhere in the machinery, and the AdapterFactory writer is unaware of them, then they probably don't help this issue.) Cheers, Jeff. On 01/07/2014 08:44, "Konrad Windszus" <konra...@gmx.de> wrote: >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 >
