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

Support custom annotations with Sling Models

    Details

      Description

      To support custom annotations the API needs to be extended.
      The reasons for custom annotations are listed in http://www.mail-archive.com/dev%40sling.apache.org/msg27918.html. Also it is much more comfortable for developers, since they can use code completion in the IDE to see which options are available for each injector-specific annotation, apart from that it is less code to write (instead of multiple annotations on one field/method I would only have to write one annotation with some attributes).

      1. SLING-3499-Documentation-v1.patch
        4 kB
        Konrad Windszus
      2. SLING-3499-Documentation-v2.patch
        4 kB
        Konrad Windszus

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user kwin opened a pull request:

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

        SLING-3499, add injector-specific annotations

        Pull request for injector-specific annotations. The according JIRA bug is at https://issues.apache.org/jira/browse/SLING-3499.

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

        $ git pull https://github.com/kwin/sling SLING-3499

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

        https://github.com/apache/sling/pull/13.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 #13


        commit 7e5fea59e855f935a4a0b2839b5983415288b4e0
        Author: Konrad Windszus <konrad.windszus@netcentric.biz>
        Date: 2014-04-22T08:47:50Z

        SLING-3499, add injector-specific annotations


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user kwin opened a pull request: https://github.com/apache/sling/pull/13 SLING-3499 , add injector-specific annotations Pull request for injector-specific annotations. The according JIRA bug is at https://issues.apache.org/jira/browse/SLING-3499 . You can merge this pull request into a Git repository by running: $ git pull https://github.com/kwin/sling SLING-3499 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/sling/pull/13.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 #13 commit 7e5fea59e855f935a4a0b2839b5983415288b4e0 Author: Konrad Windszus <konrad.windszus@netcentric.biz> Date: 2014-04-22T08:47:50Z SLING-3499 , add injector-specific annotations
        Hide
        kwin Konrad Windszus added a comment - - edited

        The pull request listed above adds the following annotations (with the optional attributes listed in brackets)

        • OSGiService (optional, filter)
        • ChildResource (optional, name, via)
        • ScriptVariable (optional, name)
        • Value (optional, name, via)
        • RequestAttribute (optional, name)

        Those annotations make Sling Models inject the field/method only from the mentioned injector.
        Also there are two interfaces added to the API bundle (ModelAnnotationProcessor and ModelAnnotationProcessorFactory) to make it possible to add more custom annotations in the future.

        Show
        kwin Konrad Windszus added a comment - - edited The pull request listed above adds the following annotations (with the optional attributes listed in brackets) OSGiService (optional, filter) ChildResource (optional, name, via) ScriptVariable (optional, name) Value (optional, name, via) RequestAttribute (optional, name) Those annotations make Sling Models inject the field/method only from the mentioned injector. Also there are two interfaces added to the API bundle (ModelAnnotationProcessor and ModelAnnotationProcessorFactory) to make it possible to add more custom annotations in the future.
        Hide
        justinedelson Justin Edelson added a comment -

        Hi Konrad Windszus, sorry for the delay looking at this. I went through and did some refactoring. Please reivew http://codereview.appspot.com/95250047

        Here's a summary of the changes:ere's a summary of the changes:
        1) Moved new SPI interfaces/classes into separate package
        2) Renamed Value annotation to ValueMapValue
        3) Changed ModelInject to InjectAnnotation (which is somewhat repetitive, but since end users never use it, this seemed OK)
        4) Made all the annotation processors inner classes.
        5) Reduced the use of annonymous inner classes to avoid extra object creation.
        6) Moved some things around inside injectFieldOrMethod() so that createAnnotationProcessor was only called if the source matches.

        Show
        justinedelson Justin Edelson added a comment - Hi Konrad Windszus , sorry for the delay looking at this. I went through and did some refactoring. Please reivew http://codereview.appspot.com/95250047 Here's a summary of the changes:ere's a summary of the changes: 1) Moved new SPI interfaces/classes into separate package 2) Renamed Value annotation to ValueMapValue 3) Changed ModelInject to InjectAnnotation (which is somewhat repetitive, but since end users never use it, this seemed OK) 4) Made all the annotation processors inner classes. 5) Reduced the use of annonymous inner classes to avoid extra object creation. 6) Moved some things around inside injectFieldOrMethod() so that createAnnotationProcessor was only called if the source matches.
        Hide
        kwin Konrad Windszus added a comment -

        Hi Justin, the patch looks fine to me, but please make sure that the changed handling of default values is not overwritten (SLING-3547). I definitely appreciate your changes. There is only one thing which should be changed which is the Javadoc for InjectAnnotationProcessor.isOptional(). There is some copy paste mistake in there. It should rather state that three values are supported as return values: true, means the injection can be left out, false means it is mandatory and null means the value from the Optional annotation is taken.

        Show
        kwin Konrad Windszus added a comment - Hi Justin, the patch looks fine to me, but please make sure that the changed handling of default values is not overwritten ( SLING-3547 ). I definitely appreciate your changes. There is only one thing which should be changed which is the Javadoc for InjectAnnotationProcessor.isOptional(). There is some copy paste mistake in there. It should rather state that three values are supported as return values: true, means the injection can be left out, false means it is mandatory and null means the value from the Optional annotation is taken.
        Hide
        justinedelson Justin Edelson added a comment -

        Hi Konrad Windszus, I tried to keep the SLING-3547 changes in place. And hope I succceeded

        One question - in your original patch, the hasDefaultValue() and getDefaultAnnotationValue() are not implemented in any of the annotation processors (just the abstract base one). Was that intentional?

        Show
        justinedelson Justin Edelson added a comment - Hi Konrad Windszus , I tried to keep the SLING-3547 changes in place. And hope I succceeded One question - in your original patch, the hasDefaultValue() and getDefaultAnnotationValue() are not implemented in any of the annotation processors (just the abstract base one). Was that intentional?
        Hide
        kwin Konrad Windszus added a comment -

        Hi Justin Edelson, my intent was to make it possible to define defaults also within the custom annotations. I didn't leverage that for my custom annotations, because it is just a lot of effort due to type limitations on annotation attributes (http://stackoverflow.com/questions/1458535/which-types-can-be-used-for-java-annotation-members). Therefore I decided to still leverage the default annotation for that. Otherwise a lot of boilerplate code would be necessary, which wouldn't add too much value IMHO.
        I still would leave it within the annotation processor, because I can image that there might be use cases for that (in other annotations).

        Show
        kwin Konrad Windszus added a comment - Hi Justin Edelson , my intent was to make it possible to define defaults also within the custom annotations. I didn't leverage that for my custom annotations, because it is just a lot of effort due to type limitations on annotation attributes ( http://stackoverflow.com/questions/1458535/which-types-can-be-used-for-java-annotation-members ). Therefore I decided to still leverage the default annotation for that. Otherwise a lot of boilerplate code would be necessary, which wouldn't add too much value IMHO. I still would leave it within the annotation processor, because I can image that there might be use cases for that (in other annotations).
        Hide
        kwin Konrad Windszus added a comment -

        Hi Justin Edelson, any progress on this? Do you need more information from me?

        Show
        kwin Konrad Windszus added a comment - Hi Justin Edelson , any progress on this? Do you need more information from me?
        Hide
        justinedelson Justin Edelson added a comment -

        Konrad Windszus the only thing left to be done is that I want to write a test or two for the default capability. I should be able to do that in the next week. Just been busy.

        Show
        justinedelson Justin Edelson added a comment - Konrad Windszus the only thing left to be done is that I want to write a test or two for the default capability. I should be able to do that in the next week. Just been busy.
        Hide
        justinedelson Justin Edelson added a comment -

        OK. I moved that along. Commited in r1600469.

        Konrad Windszus any chance you feel like submitting a patch for the documentation?

        Show
        justinedelson Justin Edelson added a comment - OK. I moved that along. Commited in r1600469. Konrad Windszus any chance you feel like submitting a patch for the documentation?
        Hide
        kwin Konrad Windszus added a comment -

        Hi Justin Edelson, thanks for committing that. I attached a patch which adds some documentation around the injector-specific annotations.

        Show
        kwin Konrad Windszus added a comment - Hi Justin Edelson , thanks for committing that. I attached a patch which adds some documentation around the injector-specific annotations.
        Hide
        kwin Konrad Windszus added a comment -

        Updated patch to also include the table of contents (since the page is now pretty long).

        Show
        kwin Konrad Windszus added a comment - Updated patch to also include the table of contents (since the page is now pretty long).
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user kwin closed the pull request at:

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

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

        With the changes from SLING-3683 the attached documentation patch needs to be adjusted (in the section "Custom Annotations"). Justin Edelson Can you take care of that?

        Show
        kwin Konrad Windszus added a comment - With the changes from SLING-3683 the attached documentation patch needs to be adjusted (in the section "Custom Annotations"). Justin Edelson Can you take care of that?
        Hide
        justinedelson Justin Edelson added a comment -

        Yes, I'll take care of the documentation updates.

        Show
        justinedelson Justin Edelson added a comment - Yes, I'll take care of the documentation updates.
        Hide
        kwin Konrad Windszus added a comment -

        Hi Justin Edelson, now that 1.0.6 is released, would it be possible to merge my documentation changes?
        Thanks a lot!

        Show
        kwin Konrad Windszus added a comment - Hi Justin Edelson , now that 1.0.6 is released, would it be possible to merge my documentation changes? Thanks a lot!
        Hide
        justinedelson Justin Edelson added a comment -

        Konrad Windszus Sorry for the delay; I have pushed the documentation changes. I did remove the bit about performance improvements as, AFAIK, we don't have any tests to show what the performance difference is. That would be interesting to add at a later point.

        Show
        justinedelson Justin Edelson added a comment - Konrad Windszus Sorry for the delay; I have pushed the documentation changes. I did remove the bit about performance improvements as, AFAIK, we don't have any tests to show what the performance difference is. That would be interesting to add at a later point.
        Hide
        kwin Konrad Windszus added a comment -

        Justin Edelson Thanks a lot. Indeed I have not measured the performance impact. Also that would matter a lot on which injector is taken in the end (if the previous ones make an expensive lookup and all fail it is obviously bigger). But of course I am fine with removing that statement for now.

        Show
        kwin Konrad Windszus added a comment - Justin Edelson Thanks a lot. Indeed I have not measured the performance impact. Also that would matter a lot on which injector is taken in the end (if the previous ones make an expensive lookup and all fail it is obviously bigger). But of course I am fine with removing that statement for now.

          People

          • Assignee:
            justinedelson Justin Edelson
            Reporter:
            kwin Konrad Windszus
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development