[jira] [Comment Edited] (SLING-3709) Sling Models: Allow caller to deal with exceptions

["Konrad Windszus (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SLING-3709?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14338339#comment-14338339 ] 

Konrad Windszus edited comment on SLING-3709 at 2/26/15 12:46 PM:
------------------------------------------------------------------

Basically this is working. But I do not agree with this statement here:
bq. in order to keep the internals simpler, I created a Result object which can either throw the appropriate exception or generate the appropriate log message.

Rather I find the code pretty confusing now because we do have one generic {{Failure}} class with lots of different parametrisation possibilities.
I find the exception based solution rather straightforward here. So I would argue that throwing an exception in case of obvious development errors (which will not happen in production) is totally fine, even if we are called from adaptTo. Of course we must make sure to catch those exceptions to not violate the adaptTo contract. But there is IMHO no need for wrapping that in a convoluted Result/Failure class. Only if the exception might happen also for correctly developed models we should prevent exceptions for performance reasons (as those errors might also occur on production).
Due to this we already had an issue with parametrisation (see SLING-4432).

[~sseifert@pro-vision.de][~justinedelson] WDYT?


was (Author: kwin):
Basically this is working. But I do not agree with this statement here:
bq. in order to keep the internals simpler, I created a Result object which can either throw the appropriate exception or generate the appropriate log message.

Rather I find the code pretty confusing now because we do have one generic Failure with lots of different parametrisation possibilities.
I find the exception based solution rather straightforward here. So I would argue that throwing an exception in case of obvious development errors (which will not happen in production) is totally fine, even if we are called from adaptTo. Of course we must make sure to catch those exceptions to not violate the adaptTo contract. But there is IMHO no need for wrapping that in a convoluted Result/Failure class. Only if the exception might happen also for correctly developed models we should prevent exceptions for performance reasons (as those errors might also occur on production).
Due to this we already had an issue with parametrisation (see SLING-4432).

[~sseifert@pro-vision.de][~justinedelson] WDYT?

> Sling Models: Allow caller to deal with exceptions
> --------------------------------------------------
>
>                 Key: SLING-3709
>                 URL: https://issues.apache.org/jira/browse/SLING-3709
>             Project: Sling
>          Issue Type: Improvement
>          Components: Extensions
>    Affects Versions: Sling Models Implementation 1.0.4, Sling Models Implementation 1.0.6
>            Reporter: Konrad Windszus
>              Labels: models
>             Fix For: Sling Models API 1.2.0, Sling Models Impl 1.2.0
>
>
> Currently due to the specification of the adaptTo-method to return null if adaptation is not possible, the caller is not notified about any exceptions (because they are caught within the ModelAdapterFactory).
> This is e.g. necessary to deal with validation exceptions properly (i.e. required field injection not possible).  The problem was also discussed briefly in http://apache-sling.73963.n3.nabble.com/Silng-Models-Validation-Framework-td4033411.html.
> All exceptions either being thrown by the 
> @PostConstruct method or caused by the field/method injection are not propagated but basically swallowed by Sling Models.
> It would be great to be able to catch those exceptions either in the view or in a servlet filter. I think it should be possible to throw unchecked exceptions in the ModelAdapterFactory.getFactory() method if it is requested (i.e. through a global OSGi configuration flag for Sling Models).
> WDYT?
> Would you accept such a patch or do you think this breaks the API (also compare with https://issues.apache.org/jira/browse/SLING-2712?focusedCommentId=13561516&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13561516).
> If it does not work through the adaptTo, SlingModels should provide an alternative way of instanciating models (and propagating exceptions), although this is kind of tricky, because it internally relies on adaptTo as well (e.g. in https://github.com/apache/sling/blob/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L647)



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)