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

DefaultInjectionStrategy not considered for injector-specific annotations

    Details

      Description

      The default injection strategy (being implemented in SLING-3696) is only considered, in case there is no injector-specific annotation being used.
      Otherwise it is just ignored.

      The logic should be like this:
      if annotationProcessor.isOptional() returns null
      -> the default injection strategy should be used
      in any other case the boolean value should be used.

        Issue Links

          Activity

          Hide
          kwin Konrad Windszus added a comment -

          Actually it is not that simple because:
          annotations only allow boolean but not Boolean (http://stackoverflow.com/questions/1458535/which-types-can-be-used-for-java-annotation-members) and also null is not allowed as a default value (http://stackoverflow.com/questions/1178104/error-setting-a-default-null-value-for-an-annotations-field).

          Therefore I propose the following (not backwards-compatible!!!) change about all injector specific annotations.
          Instead of having a boolean field optional we should rely on an enum with the following three allowed values: TRUE, FALSE, DEFAULT.

          The default should be DEFAULT in which case the DefaultInjectionStrategy is used.

          I would propose not adding a new attribute for that, because the previous definition of the field optional does just not behave correctly if a DefaultInjectionStrategy is used. WDYT?

          Show
          kwin Konrad Windszus added a comment - Actually it is not that simple because: annotations only allow boolean but not Boolean ( http://stackoverflow.com/questions/1458535/which-types-can-be-used-for-java-annotation-members ) and also null is not allowed as a default value ( http://stackoverflow.com/questions/1178104/error-setting-a-default-null-value-for-an-annotations-field ). Therefore I propose the following (not backwards-compatible!!!) change about all injector specific annotations. Instead of having a boolean field optional we should rely on an enum with the following three allowed values: TRUE, FALSE, DEFAULT . The default should be DEFAULT in which case the DefaultInjectionStrategy is used. I would propose not adding a new attribute for that, because the previous definition of the field optional does just not behave correctly if a DefaultInjectionStrategy is used. WDYT?
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user kwin opened a pull request:

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

          SLING-4155, support default injections strategy for injector specific annotations

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

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

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

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


          commit 99352b0048cb1c69524753cfbb1b1f41fa47254e
          Author: Konrad Windszus <konrad.windszus@netcentric.biz>
          Date: 2014-11-12T09:11:30Z

          SLING-4155, modify optional field within all Injector-specific
          annotations to support three values: TRUE, FALSE and DEFAULT


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user kwin opened a pull request: https://github.com/apache/sling/pull/33 SLING-4155 , support default injections strategy for injector specific annotations You can merge this pull request into a Git repository by running: $ git pull https://github.com/kwin/sling SLING-4155 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/sling/pull/33.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 #33 commit 99352b0048cb1c69524753cfbb1b1f41fa47254e Author: Konrad Windszus <konrad.windszus@netcentric.biz> Date: 2014-11-12T09:11:30Z SLING-4155 , modify optional field within all Injector-specific annotations to support three values: TRUE, FALSE and DEFAULT
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          such a change on the annotations will indeed break both runtime and compile-time backward compatibility - we cannot do this unless we raise the version to v2.0.0 to comply to semantic versioning. i'm not sure if it's worth only for this issue, although i agree the current functionality is broken when injector-specific annotations are used.

          for a first step we should document this issue (SLING-4156), and perhaps create a new version in JIRA to sling models 2.0.0 and collect there tickets like this so we do not forget it when time is ready for v2.

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - such a change on the annotations will indeed break both runtime and compile-time backward compatibility - we cannot do this unless we raise the version to v2.0.0 to comply to semantic versioning. i'm not sure if it's worth only for this issue, although i agree the current functionality is broken when injector-specific annotations are used. for a first step we should document this issue ( SLING-4156 ), and perhaps create a new version in JIRA to sling models 2.0.0 and collect there tickets like this so we do not forget it when time is ready for v2.
          Hide
          kwin Konrad Windszus added a comment - - edited

          In my pull-request I already raised the exported package version number to 2.0 (thanks to the new baseline feature of the maven-bundle-plugin).

          I don't want that feature to be broken until we release 2.0 so what about the following approach:

          1. add a new field to all injector-specific annotations named isOptional with the default value DEFAULT (which means following the default injection strategy)
          2. deprecate the old field optional (which still has false as its default value)

          With v2.0 of Sling Models we could then finally remove the field optional

          Now the default injection strategy should be followed if

          1. isOptional = DEFAULT and
          2. optional = false (either explicitly set or just because it is the default - it is impossible to distinguish those two cases)

          Now if we look at the following usecases this is the new vs. the old behaviour:

          optional DefaultInjectionStrategy old behaviour new behaviour
          false(default) Required(default) required required
          false(default) Optional required optional
          true Required(default) optional optional
          true Optional optional optional

          Although this actually changes the second of the four usecases I don't think this will ever affect someone, because the DefaultInjectionStrategy was probably never set on models using injector-specific annotations (due to this bug).
          WDYT?

          Show
          kwin Konrad Windszus added a comment - - edited In my pull-request I already raised the exported package version number to 2.0 (thanks to the new baseline feature of the maven-bundle-plugin). I don't want that feature to be broken until we release 2.0 so what about the following approach: add a new field to all injector-specific annotations named isOptional with the default value DEFAULT (which means following the default injection strategy) deprecate the old field optional (which still has false as its default value) With v2.0 of Sling Models we could then finally remove the field optional Now the default injection strategy should be followed if isOptional = DEFAULT and optional = false (either explicitly set or just because it is the default - it is impossible to distinguish those two cases) Now if we look at the following usecases this is the new vs. the old behaviour: optional DefaultInjectionStrategy old behaviour new behaviour false (default) Required (default) required required false (default) Optional required optional true Required (default) optional optional true Optional optional optional Although this actually changes the second of the four usecases I don't think this will ever affect someone, because the DefaultInjectionStrategy was probably never set on models using injector-specific annotations (due to this bug). WDYT?
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          yes, from my perspective that would be a good way.
          the only drawback are the to annotation fields optional and isOptional - but that could be documented in the API.

          an alternative would be another name like injectionStrategy, with values INHERIT, OPTIONAL, REQUIRED. this would be closer to the naming of defaultInjectionStrategy of the @Model annotation.

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - yes, from my perspective that would be a good way. the only drawback are the to annotation fields optional and isOptional - but that could be documented in the API. an alternative would be another name like injectionStrategy , with values INHERIT, OPTIONAL, REQUIRED. this would be closer to the naming of defaultInjectionStrategy of the @Model annotation.
          Hide
          kwin Konrad Windszus added a comment -

          Stefan Seifert Thanks for your input. I agree that injectionStrategy is a better name. I would still need to increase the version to 2.0.0 on org.apache.sling.models.spi.injectorspecific, because I would need to add the getInjectionStrategy() method on the interface InjectAnnotationProcessor. To make that backwards compatible one would need to add an additional interface InjectAnnotationProcessor2 which could then have a method to retrieve the value from injectionStrategy.
          That will still support the old InjectAnnotationProcessor with the known limitations. But for all annotations being supported by Sling Models I could implement the new interface, leveraging that additional field on the annotation. WDYT?

          Show
          kwin Konrad Windszus added a comment - Stefan Seifert Thanks for your input. I agree that injectionStrategy is a better name. I would still need to increase the version to 2.0.0 on org.apache.sling.models.spi.injectorspecific , because I would need to add the getInjectionStrategy() method on the interface InjectAnnotationProcessor . To make that backwards compatible one would need to add an additional interface InjectAnnotationProcessor2 which could then have a method to retrieve the value from injectionStrategy . That will still support the old InjectAnnotationProcessor with the known limitations. But for all annotations being supported by Sling Models I could implement the new interface, leveraging that additional field on the annotation. WDYT?
          Hide
          kwin Konrad Windszus added a comment -

          I updated the PR (https://github.com/apache/sling/pull/33).
          Stefan Seifert,Justin Edelson Can you both have a look at that?

          Show
          kwin Konrad Windszus added a comment - I updated the PR ( https://github.com/apache/sling/pull/33 ). Stefan Seifert , Justin Edelson Can you both have a look at that?
          Hide
          kwin Konrad Windszus added a comment -

          I committed the changes in rev. 1640710.

          Show
          kwin Konrad Windszus added a comment - I committed the changes in rev. 1640710.
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          sorry, i was quite busy the last days.

          the current solution solves the backwards compatibility problem. of course it's not so nice have all those *2 classses now in there, but as long as it is only the SPI and not the API i assume its tolerable.

          one small point: the interface InjectAnnotationProcessorFactory should be marked as deprecated as well?

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - sorry, i was quite busy the last days. the current solution solves the backwards compatibility problem. of course it's not so nice have all those *2 classses now in there, but as long as it is only the SPI and not the API i assume its tolerable. one small point: the interface InjectAnnotationProcessorFactory should be marked as deprecated as well?
          Hide
          kwin Konrad Windszus added a comment -

          Stefan Seifert Thanks for the review. I just deprecated InjectAnnotationProcessorFactory in rev. 1640931.

          Show
          kwin Konrad Windszus added a comment - Stefan Seifert Thanks for the review. I just deprecated InjectAnnotationProcessorFactory in rev. 1640931.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user kwin closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user kwin closed the pull request at: https://github.com/apache/sling/pull/33
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          Completed: At revision: 1678937

          fixed a typo in an annotation attribute: injectonStrategy => injectionStrategy

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - Completed: At revision: 1678937 fixed a typo in an annotation attribute: injectonStrategy => injectionStrategy

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development