[
https://issues.apache.org/jira/browse/SLING-3709?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14109354#comment-14109354
]
Konrad Windszus commented on SLING-3709:
----------------------------------------
Thanks for the review. Regarding your comments:
{quote}
Exports should not be in the spi package. It should be in a separate
package either just org.apache.sling.models or org.apache.sling.models.factory.
{quote}
I agree, I am gonna change that.
{quote}
Exceptions should not be thrown at all during the adaptTo code path. This
is just wasteful. In fact, the adaptTo code path should not be changed to the
extent possible.
{quote}
Let me comment on the individual exceptions:
- InvalidAdaptableException, that happens quite regularaly. I am gonna make
sure, that this is never thrown when adaptTo is called. Also I will add a check
method on the model factory which will return true if a model can be
instanciated based on a given adaptable
- ValidationException, that of course can happen during runtime as well. I am
gonna make sure that also this exception is never thrown during adaptTo.
- IllegalArgumentException (for missing model annotation), the same as before.
I make sure that this is not gonna be thrown during adaptTo
- The only exception which indicates an actual implementation error is
currently IllegalStateException. I am gonna replace those by more meaningfull
runtime exceptions, but those should be thrown and caught even during adaptTo.
{quote}
I don't understand the use of IllegalStateException. According to the
JavaDocs, this "Signals that a method has been invoked at an illegal or
inappropriate time. In other words, the Java environment or Java application is
not in an appropriate state for the requested operation." That doesn't appear
appropriate for how it is used here.
{quote}
I am gonna refactor that so that reasonable exceptions are thrown in case:
# instanciation fails due to errors in post construct (custom
PostConstructException, wrapping the original exception)
# instanciation fails due to invalid constructors (InstanciationException)
# injection fails due to some reflection issues (not sure yet which Exception
to throw here)
{quote}
In general, this would appear to have very limited value at this point. All you
can see is whether the class had the right annotation. Everything else is
wrapped in a generic ValidationException. I assume that is going to be changed
to provide more fine grained information
{quote}
It turned out to be already quite useful during development (E.g. I came up
with my own Sightly Use Extension which uses this Factory), as the errors
become immediately visible to the developers. And there are a lot of errors to
make here (wrong adaptable, invalid constructor, error in post construct). I
agree that probably the validation exception needs to be extended a littlebit
to always include the real name of the thing which could not be injected and
also to take care of the injector-specific annotation to be able to say
something like
{noformat}
"Could not inject the value "jcr:title" from the resource at
"/content/sling/democontent" into the field "title" with type "String" because
the value is not there/could not be converted to that type."
{noformat}
So I am gonna refactor that part as well.
I am trying to come up with a new patch by the end of the week.
> 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
>
> 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.2#6252)
