Uploaded image for project: 'Sling'
  1. Sling
  2. SLING-3709

Sling Models: Allow caller to deal with exceptions

    Details

      Description

      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)

        Issue Links

          Activity

          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          this is a big bunch of changes - i could not review every code line, but i think the approach is better.

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - this is a big bunch of changes - i could not review every code line, but i think the approach is better.
          Hide
          kwin Konrad Windszus added a comment -

          I now rely again on the exception classes (but encapsulating them in the Result) instead of directly throwing them). Committed the change with rev1680581.
          Stefan Seifert and Justin Edelson: Could you quickly check whether you are fine with those changes?

          Show
          kwin Konrad Windszus added a comment - I now rely again on the exception classes (but encapsulating them in the Result ) instead of directly throwing them). Committed the change with rev1680581. Stefan Seifert and Justin Edelson : Could you quickly check whether you are fine with those changes?
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          i'm not sure how the code was before which is now encapsulated in the Result class, but i find the current code in the class a bit brittle as well with the different sets of attributes populated only in some cases and then passed over for parametrization.
          i'm fine with sticking to normal exceptions when detecting "programming errors" in the models.

          what is your proposal to make it better - do you want to remove the class altogether, or only some aspects from it?

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - i'm not sure how the code was before which is now encapsulated in the Result class, but i find the current code in the class a bit brittle as well with the different sets of attributes populated only in some cases and then passed over for parametrization. i'm fine with sticking to normal exceptions when detecting "programming errors" in the models. what is your proposal to make it better - do you want to remove the class altogether, or only some aspects from it?
          Hide
          kwin Konrad Windszus added a comment - - edited

          Basically this is working. But I do not agree with this statement here:

          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).

          Stefan SeifertJustin Edelson WDYT?

          Show
          kwin Konrad Windszus added a comment - - edited Basically this is working. But I do not agree with this statement here: 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 ). Stefan Seifert Justin Edelson WDYT?
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          Konrad Windszus is there any work left on this ticket - or can we set it to resolved?

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - Konrad Windszus is there any work left on this ticket - or can we set it to resolved?
          Hide
          kwin Konrad Windszus added a comment -

          pushed a documentation update in rev. 1644385.

          Show
          kwin Konrad Windszus added a comment - pushed a documentation update in rev. 1644385.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user kwin closed the pull request at:

          https://github.com/apache/sling/pull/24

          Show
          githubbot ASF GitHub Bot added a comment - Github user kwin closed the pull request at: https://github.com/apache/sling/pull/24
          Hide
          kwin Konrad Windszus added a comment -

          Hi Justin Edelson, thanks a lot for bringing this forward. I am only missing that the ModelFactory is also called if I try to inject one Model into the other. In that case the exception is not propagated, because the inner model is instanciated via adaptTo.

          Show
          kwin Konrad Windszus added a comment - Hi Justin Edelson , thanks a lot for bringing this forward. I am only missing that the ModelFactory is also called if I try to inject one Model into the other. In that case the exception is not propagated, because the inner model is instanciated via adaptTo.
          Hide
          justinedelson Justin Edelson added a comment -

          In r1628824, I committed a version of this. I kept the API basically the same, but 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.

          I did change the name of one of the exceptions from NoInjectorException to MissingElementsException as that seemed more accurate.

          Leaving this issue open for a bit as I want to add some more unit tests for the ModelFactory code path.

          Thanks for the patch and for pushing this along.

          Show
          justinedelson Justin Edelson added a comment - In r1628824, I committed a version of this. I kept the API basically the same, but 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. I did change the name of one of the exceptions from NoInjectorException to MissingElementsException as that seemed more accurate. Leaving this issue open for a bit as I want to add some more unit tests for the ModelFactory code path. Thanks for the patch and for pushing this along.
          Hide
          kwin Konrad Windszus added a comment -

          Justin Edelson Would you mind taking another look at the proposed PR (https://github.com/apache/sling/pull/24). Now that Sling Models 1.1.0 is out it would be great if you could consider adding that.

          Show
          kwin Konrad Windszus added a comment - Justin Edelson Would you mind taking another look at the proposed PR ( https://github.com/apache/sling/pull/24 ). Now that Sling Models 1.1.0 is out it would be great if you could consider adding that.
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          my example was not targeted at the sightly - sling models integration, but when you want to adapt a model yourself in your code. as your usecase is completely different let us forget my proposal for now, it will not help (although the problems you mentioned should be solvable).

          if your real goal and usecase is only to have a nicer representation of sling model adaptions error (instead of just having them in the sling error log as it is the case now) i wonder if there are not easier and more straightforward solutions than extending the sling models API in this way plus a custom use provider. i've not yet looked into the inner workings of the AEM6 develop layer yet, so i have currently no idea what might be alternatives to push this error information to it.

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - my example was not targeted at the sightly - sling models integration, but when you want to adapt a model yourself in your code. as your usecase is completely different let us forget my proposal for now, it will not help (although the problems you mentioned should be solvable). if your real goal and usecase is only to have a nicer representation of sling model adaptions error (instead of just having them in the sling error log as it is the case now) i wonder if there are not easier and more straightforward solutions than extending the sling models API in this way plus a custom use provider. i've not yet looked into the inner workings of the AEM6 develop layer yet, so i have currently no idea what might be alternatives to push this error information to it.
          Hide
          kwin Konrad Windszus added a comment - - edited

          Stefan Seifert I still don't get what your proposal is. Do you mean the following behaviour:

          1. call resource/request.adaptTo (e.g. through the CQ Sightly Use Provider).
          2. Sling Models will put the missing injections into a thread local and return null for the adapter.
            What happens now is that CQ Sightly Use Provider will then just instanciate a simple pojo and you will see no difference in Sightly!
          3. You call Models.adaptToOrThrow which will check the thread local and throw exceptions in case a previous call of adaptTo was not successfull?

          Is that meant here?
          What happens here with other exceptions (e.g. like reflection exceptions, exceptions in the constructor or post construct)? Are those also stored in thread local variables?

          And to answer your question: I use my own Sling Servlet Filter to catch all Exceptions and expose them in a nice fashion to the editor (not at all on component level). That way you can also use the develop layer in AEM6 to expose the errors.

          Show
          kwin Konrad Windszus added a comment - - edited Stefan Seifert I still don't get what your proposal is. Do you mean the following behaviour: call resource/request.adaptTo (e.g. through the CQ Sightly Use Provider). Sling Models will put the missing injections into a thread local and return null for the adapter. What happens now is that CQ Sightly Use Provider will then just instanciate a simple pojo and you will see no difference in Sightly! You call Models.adaptToOrThrow which will check the thread local and throw exceptions in case a previous call of adaptTo was not successfull? Is that meant here? What happens here with other exceptions (e.g. like reflection exceptions, exceptions in the constructor or post construct)? Are those also stored in thread local variables? And to answer your question: I use my own Sling Servlet Filter to catch all Exceptions and expose them in a nice fashion to the editor (not at all on component level). That way you can also use the develop layer in AEM6 to expose the errors.
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          in my example Models would be a helper class in the Sling Models API, this class has the code to throw the exceptions. for collecting the information if a exception has to be thrown or not is implemented would be implemented inside the ModelAdapterFactory.

          why do you want to use a custom sightly use provider - you can already use sling models in sightly without this (in AEM6)?
          and what do you do in this use provider when an exception is thrown e.g. due to missing mandatory fields?

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - in my example Models would be a helper class in the Sling Models API, this class has the code to throw the exceptions. for collecting the information if a exception has to be thrown or not is implemented would be implemented inside the ModelAdapterFactory . why do you want to use a custom sightly use provider - you can already use sling models in sightly without this (in AEM6)? and what do you do in this use provider when an exception is thrown e.g. due to missing mandatory fields?
          Hide
          kwin Konrad Windszus added a comment -

          Whether the method within the ModelFactory is called adaptToOrThrow or createModel I don't care that much to be honest. I need to make that an OSGi service, because otherwise were would you put the code for instanciation and throwing exceptions? What is Models in your example? Is that a new class within the Sling Models API bundle? In most of the cases where you call that ModelFactory, you are in an OSGi component anyhow, and there it is cheap to inject other services.

          Since the discussion around adaptTo didn't come to a conclusion I started with my own interface only dedicated to do instanciation and allowing to throw exceptions for Sling Models.
          The methods isModelClass and canCreateFromAdaptable are necessary for the clients.

          I use those internally for a custom Sightly Use Provider [1]. With Sightly Use the different providers are asked to provide an instance of the requested class. The Sling Model Factory Use Provider is only throwing an exception or providing an instance if
          a) we are dealing with a Sling Model (checked via isModelClass)
          b) that class can be adapted from the given Adaptable (checked via canAdaptFromAdaptable)
          In all other cases it would return null, meaning the other Use Providers would be asked.

          If I would rely on exceptions to figure that out, that Use Provider would come with a severe performance drawback (as this is asked always first).

          The same logic could be implemented with a custom JSP tag.

          [1] - https://dev.day.com/docs/en/aem/6-0/develop/ref/javadoc/io/sightly/java/api/UseProvider.html

          Show
          kwin Konrad Windszus added a comment - Whether the method within the ModelFactory is called adaptToOrThrow or createModel I don't care that much to be honest. I need to make that an OSGi service, because otherwise were would you put the code for instanciation and throwing exceptions? What is Models in your example? Is that a new class within the Sling Models API bundle? In most of the cases where you call that ModelFactory, you are in an OSGi component anyhow, and there it is cheap to inject other services. Since the discussion around adaptTo didn't come to a conclusion I started with my own interface only dedicated to do instanciation and allowing to throw exceptions for Sling Models. The methods isModelClass and canCreateFromAdaptable are necessary for the clients. I use those internally for a custom Sightly Use Provider [1] . With Sightly Use the different providers are asked to provide an instance of the requested class. The Sling Model Factory Use Provider is only throwing an exception or providing an instance if a) we are dealing with a Sling Model (checked via isModelClass ) b) that class can be adapted from the given Adaptable (checked via canAdaptFromAdaptable ) In all other cases it would return null, meaning the other Use Providers would be asked. If I would rely on exceptions to figure that out, that Use Provider would come with a severe performance drawback (as this is asked always first). The same logic could be implemented with a custom JSP tag. [1] - https://dev.day.com/docs/en/aem/6-0/develop/ref/javadoc/io/sightly/java/api/UseProvider.html
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          having a separate ModelFactory interface is not so elegant. and the interfaces mixes different concerns in my opinion - creating a model with validation, and inspecting model classes and possible adaptions (which is of more use for the internal implementation than for the consumer code).

          coming back the the "adaptTo and results ..." discussion - what about a variant like:

                  // adaption with exception handling
                  try {
                      model = Models.adaptToOrThrow(resource, MyModel.class);
                  }
                  catch (ValidationException ex) {
                      // handle exception
                  }
          

          in this case the ModelAdapterFactory.getAdapter method never throws an exception, but collects information about validation errors or other problems in a thread local. this thread local is read inside the helper method Models.adaptToOrThrow and an exception is thrown if required.

          in general the extensions of this ticket are a bit problematic because they only work if the consumer code changes the way to get models (and does not use standard adaptTo semantics). that means that other layers that work with models (e.g. inject a model into sightly view, injects models in other models) will still swallow these exceptions because they will use only adaptTo. but in scenarios where the consumer code wants to handle explicit exception a helper method like Models.adaptToOrThrows is more lightweight than having to get a reference to an OSGi services for this.

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - having a separate ModelFactory interface is not so elegant. and the interfaces mixes different concerns in my opinion - creating a model with validation, and inspecting model classes and possible adaptions (which is of more use for the internal implementation than for the consumer code). coming back the the "adaptTo and results ..." discussion - what about a variant like: // adaption with exception handling try { model = Models.adaptToOrThrow(resource, MyModel.class); } catch (ValidationException ex) { // handle exception } in this case the ModelAdapterFactory.getAdapter method never throws an exception, but collects information about validation errors or other problems in a thread local. this thread local is read inside the helper method Models.adaptToOrThrow and an exception is thrown if required. in general the extensions of this ticket are a bit problematic because they only work if the consumer code changes the way to get models (and does not use standard adaptTo semantics). that means that other layers that work with models (e.g. inject a model into sightly view, injects models in other models) will still swallow these exceptions because they will use only adaptTo. but in scenarios where the consumer code wants to handle explicit exception a helper method like Models.adaptToOrThrows is more lightweight than having to get a reference to an OSGi services for this.
          Hide
          kwin Konrad Windszus added a comment -

          I updated the PR. Now exceptions are only thrown in the adaptTo codepath if they are caused by invalid models (and therefore must be fixed by developers). The overhead for throwing and catching is negligible because those errors need to be fixed within the code (and therefore should never happen on production instances). I also enriched the exception which is being thrown in case of no injector can inject a reasonable value with additional information. Can you review again?

          Show
          kwin Konrad Windszus added a comment - I updated the PR. Now exceptions are only thrown in the adaptTo codepath if they are caused by invalid models (and therefore must be fixed by developers). The overhead for throwing and catching is negligible because those errors need to be fixed within the code (and therefore should never happen on production instances). I also enriched the exception which is being thrown in case of no injector can inject a reasonable value with additional information. Can you review again?
          Hide
          kwin Konrad Windszus added a comment -

          Thanks for the review. Regarding your comments:

          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.

          I agree, I am gonna change that.

          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.

          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.

          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.

          I am gonna refactor that so that reasonable exceptions are thrown in case:

          1. instanciation fails due to errors in post construct (custom PostConstructException, wrapping the original exception)
          2. instanciation fails due to invalid constructors (InstanciationException)
          3. injection fails due to some reflection issues (not sure yet which Exception to throw here)

          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

          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

          "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."
          

          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.

          Show
          kwin Konrad Windszus added a comment - Thanks for the review. Regarding your comments: 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. I agree, I am gonna change that. 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. 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. 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. 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) 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 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 "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." 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.
          Hide
          justinedelson Justin Edelson added a comment -

          My comments:

          • 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.
          • 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.
          • 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.

          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.

          Show
          justinedelson Justin Edelson added a comment - My comments: 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. 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. 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. 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.
          Hide
          kwin Konrad Windszus added a comment -

          I enriched the PR to also use the same mechanism for instanciating submodels. The ValidationException for now will stay as is. Later on we can improve that.

          Show
          kwin Konrad Windszus added a comment - I enriched the PR to also use the same mechanism for instanciating submodels. The ValidationException for now will stay as is. Later on we can improve that.
          Hide
          kwin Konrad Windszus added a comment - - edited

          Justin Edelson The PR contains a first draft for that. Can you have a look and give me feedback? Also I included a small bugfix in the unbindInjectAnnotationProcessorFactory method (copy and paste mistake).

          What I want to extend is

          • Enrich the ValidationException by making it possible to list the fields/methods which could not be injected separately, to make it possible to evaluate those exceptions more easily
          • Use the mechanism for composed models as well
          Show
          kwin Konrad Windszus added a comment - - edited Justin Edelson The PR contains a first draft for that. Can you have a look and give me feedback? Also I included a small bugfix in the unbindInjectAnnotationProcessorFactory method (copy and paste mistake). What I want to extend is Enrich the ValidationException by making it possible to list the fields/methods which could not be injected separately, to make it possible to evaluate those exceptions more easily Use the mechanism for composed models as well
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user kwin opened a pull request:

          https://github.com/apache/sling/pull/24

          SLING-3709, add another exception-aware mechanism of instanciating Sling

          SLING-3709, add another way of instanciating a Sling Model which throws meaningful exceptions in case of errors

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/kwin/sling SLING-3709-Throw-Runtime-Exceptions

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/sling/pull/24.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #24


          commit 07eb45087c7c14c8b6788816794b8356a9178728
          Author: Konrad Windszus <konrad.windszus@netcentric.biz>
          Date: 2014-08-06T13:25:41Z

          SLING-3709, add another exception-aware mechanism of instanciating Sling
          Models


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user kwin opened a pull request: https://github.com/apache/sling/pull/24 SLING-3709 , add another exception-aware mechanism of instanciating Sling SLING-3709 , add another way of instanciating a Sling Model which throws meaningful exceptions in case of errors You can merge this pull request into a Git repository by running: $ git pull https://github.com/kwin/sling SLING-3709 -Throw-Runtime-Exceptions Alternatively you can review and apply these changes as the patch at: https://github.com/apache/sling/pull/24.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #24 commit 07eb45087c7c14c8b6788816794b8356a9178728 Author: Konrad Windszus <konrad.windszus@netcentric.biz> Date: 2014-08-06T13:25:41Z SLING-3709 , add another exception-aware mechanism of instanciating Sling Models
          Hide
          justinedelson Justin Edelson added a comment -

          Konrad Windszus I'm not sure I understand what you are proposing - how would an AdapterFactory (ModelAdapterFactory in this case) know that it was being invoked from Sightly vs. Java vs. a JSP? By inspecting the call stack?

          I would be more inclined to introduce a direct method of accessing the ModelAdapterFactory, i.e. a new service interface. This would, to your point, require that the ModelAdapterFactory vary behavior for composed models based on whether or not the child model is adapter by ModelAdapterFactory vs. some other AdapterFactory, but this doesn't strike me as too challenging to implement. Or perhaps this can be skipped and only throw an exception for a single model class (and not children).

          Show
          justinedelson Justin Edelson added a comment - Konrad Windszus I'm not sure I understand what you are proposing - how would an AdapterFactory (ModelAdapterFactory in this case) know that it was being invoked from Sightly vs. Java vs. a JSP? By inspecting the call stack? I would be more inclined to introduce a direct method of accessing the ModelAdapterFactory, i.e. a new service interface. This would, to your point, require that the ModelAdapterFactory vary behavior for composed models based on whether or not the child model is adapter by ModelAdapterFactory vs. some other AdapterFactory, but this doesn't strike me as too challenging to implement. Or perhaps this can be skipped and only throw an exception for a single model class (and not children).
          Hide
          kwin Konrad Windszus added a comment -

          Since the discussion at http://www.mail-archive.com/dev@sling.apache.org/msg31261.html did not really come to a conclusion, I am a bit clueless how to proceed here. We urgently need a way to deal with validation errors but obviously I could not convince everyone that adaptTo needs to be extended/modified.

          I don't think that introducing another mechanism in addition to the adaptTo method only for Sling Models is the right way here because
          a) adaptTo is already used within Sling Models (e.g. for model composition)
          b) adaptTo is widely supported by custom tags/extensions in JSP and Sightly

          Your proposal with wrapping an exception within the adapter does not really work well, because we don't really want to unwrap the exception to see it (that would be very hard to do in Sightly anyways). The approach in http://www.mail-archive.com/dev@sling.apache.org/msg32612.html seems technically feasible but would require a double adaptTo. (not really good because of reasons a) and b) from above)

          I tend to agree with Felix Meschberger: ... technically a RuntimeException may always be thrown. .... (from http://www.mail-archive.com/dev@sling.apache.org/msg31261.html). The problem is backwards-compatibility with that approach.

          I would still prefer to use adaptTo (for the reasons listed above), but make it configurable whether Sightly throws RuntimeExceptions or not within an OSGi configuration (by default not) and then let the developers decide, whether to use globally either the new behaviour or the old one!

          If that is not acceptable, then we would need to invent a completely different method of instanciating a Sling Model which right from the beginning allows to throw RuntimeExceptions. The only questions then is how to deal with composed models (injecting one into the other).

          Justin Edelson WDYT?

          Show
          kwin Konrad Windszus added a comment - Since the discussion at http://www.mail-archive.com/dev@sling.apache.org/msg31261.html did not really come to a conclusion, I am a bit clueless how to proceed here. We urgently need a way to deal with validation errors but obviously I could not convince everyone that adaptTo needs to be extended/modified. I don't think that introducing another mechanism in addition to the adaptTo method only for Sling Models is the right way here because a) adaptTo is already used within Sling Models (e.g. for model composition) b) adaptTo is widely supported by custom tags/extensions in JSP and Sightly Your proposal with wrapping an exception within the adapter does not really work well, because we don't really want to unwrap the exception to see it (that would be very hard to do in Sightly anyways). The approach in http://www.mail-archive.com/dev@sling.apache.org/msg32612.html seems technically feasible but would require a double adaptTo. (not really good because of reasons a) and b) from above) I tend to agree with Felix Meschberger : ... technically a RuntimeException may always be thrown. .... (from http://www.mail-archive.com/dev@sling.apache.org/msg31261.html ). The problem is backwards-compatibility with that approach. I would still prefer to use adaptTo (for the reasons listed above), but make it configurable whether Sightly throws RuntimeExceptions or not within an OSGi configuration (by default not) and then let the developers decide, whether to use globally either the new behaviour or the old one! If that is not acceptable, then we would need to invent a completely different method of instanciating a Sling Model which right from the beginning allows to throw RuntimeExceptions. The only questions then is how to deal with composed models (injecting one into the other). Justin Edelson WDYT?
          Hide
          justinedelson Justin Edelson added a comment -

          No. Such a change violates the contract of AdapterFactory and goes against Java best practices.

          The current behavior must stay as is unless we have a way for the caller to explicitly indicate that she wants something else to happen. A configuration switch is not appropriate as that leaves the decision in the hands of the administrator, not the caller.

          Show
          justinedelson Justin Edelson added a comment - No. Such a change violates the contract of AdapterFactory and goes against Java best practices. The current behavior must stay as is unless we have a way for the caller to explicitly indicate that she wants something else to happen. A configuration switch is not appropriate as that leaves the decision in the hands of the administrator, not the caller.
          Hide
          kwin Konrad Windszus added a comment -

          It seems that throwing a RuntimeException from an AdapterFactory should be fine (http://www.mail-archive.com/dev@sling.apache.org/msg31420.html).
          Justin Edelson Would you accept a patch which lets the ModelAdapterFactory throw RuntimeExceptions if

          1. Required methods/fields could not be injected (for whatever reason)
          2. Model class does not have a usable constructor
          3. Invoking PostConstruct does fail for whatever reason

          Do you think we would need a switch for that behaviour (as configurable OSGi property) or we can just change to throwing RuntimeException instead of returning null in that case?

          Show
          kwin Konrad Windszus added a comment - It seems that throwing a RuntimeException from an AdapterFactory should be fine ( http://www.mail-archive.com/dev@sling.apache.org/msg31420.html ). Justin Edelson Would you accept a patch which lets the ModelAdapterFactory throw RuntimeExceptions if Required methods/fields could not be injected (for whatever reason) Model class does not have a usable constructor Invoking PostConstruct does fail for whatever reason Do you think we would need a switch for that behaviour (as configurable OSGi property) or we can just change to throwing RuntimeException instead of returning null in that case?
          Hide
          justinedelson Justin Edelson added a comment -

          Created SLING-3714 for the generic concept of extending the Adaptable interface. Personally, I would prefer the approach I put in there to throwing an exception as it doesn't require an API change per se.

          Show
          justinedelson Justin Edelson added a comment - Created SLING-3714 for the generic concept of extending the Adaptable interface. Personally, I would prefer the approach I put in there to throwing an exception as it doesn't require an API change per se.
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          thinking about extending the adaptTo API for this context is a good idea.
          in our projects we already use internally an additional method "adaptToNotNull" method alternatively to "adaptTo". using this methods the caller signals that he expects the adaption "to work", and does not care with null-checks. it if fails an exception is thrown.
          your proposal could be matched by such an additional method (with a better and more generic name than "adaptToNotNull") as well.

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - thinking about extending the adaptTo API for this context is a good idea. in our projects we already use internally an additional method "adaptToNotNull" method alternatively to "adaptTo". using this methods the caller signals that he expects the adaption "to work", and does not care with null-checks. it if fails an exception is thrown. your proposal could be matched by such an additional method (with a better and more generic name than "adaptToNotNull") as well.
          Hide
          justinedelson Justin Edelson added a comment -

          I think this needs to be handled at the Adaptable API. Having a separate API for Sling Models violates one of the key design principals (and, as you note, won't actually work for complex object graphs). Of course, changing the Adaptable API is non-trival, but if it worth doing for Sling Models-based adaptables, it is worth doing for all adaptables.

          Show
          justinedelson Justin Edelson added a comment - I think this needs to be handled at the Adaptable API. Having a separate API for Sling Models violates one of the key design principals (and, as you note, won't actually work for complex object graphs). Of course, changing the Adaptable API is non-trival, but if it worth doing for Sling Models-based adaptables, it is worth doing for all adaptables.

            People

            • Assignee:
              kwin Konrad Windszus
              Reporter:
              kwin Konrad Windszus
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development