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

Support Sling Validation through a new field of the Model annotation

    Details

      Description

      The current way of integrating Sling Validation (SLING-2803) with Sling Models is to inject the validation service and then call it within a PostConstruct method (http://www.slideshare.net/raducotescu/apache-sling-generic-validation-framework/16).
      This has the drawback that

      1. the ValidationService needs to be injected
      2. a PostConstruct needs to be implemented
      3. the other injections need to be marked as optional (otherwise the validation is never triggered in case of e.g. missing required valuemap values)

      Instead it would be good to support this use case with just an additional field on the annotation Model which is named validate. By default this should be false (to be backwards compatible), but if it is true the Sling Validation should be called before any values are injected into the model. If validation fails the ModelAdapterFactory should never instanciate the model and rather return null (or throw a meaningful exception for SLING-3709).

      1. SLING-4161-api-v02.patch
        13 kB
        Konrad Windszus
      2. SLING-4161-impl-v02.patch
        23 kB
        Konrad Windszus

        Issue Links

          Activity

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

          great, thanks - thank we can make an official sling models api/impl 1.2 release soon without having to wait for sling.validation

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - great, thanks - thank we can make an official sling models api/impl 1.2 release soon without having to wait for sling.validation
          Hide
          kwin Konrad Windszus added a comment -

          With r1682946 I moved the validation functionality into a separate bundle. It is called from models-impl through an OSGi service interface.

          Show
          kwin Konrad Windszus added a comment - With r1682946 I moved the validation functionality into a separate bundle. It is called from models-impl through an OSGi service interface.
          Hide
          radu.cotescu Radu Cotescu added a comment -

          Konrad Windszus, although I haven't had a chance to work on that codebase since I've donated it to Sling (due to concentrating my efforts on Sightly) I could help maintain it, therefore I'm ok with moving it into /bundles/extensions.

          Show
          radu.cotescu Radu Cotescu added a comment - Konrad Windszus , although I haven't had a chance to work on that codebase since I've donated it to Sling (due to concentrating my efforts on Sightly) I could help maintain it, therefore I'm ok with moving it into /bundles/extensions.
          Hide
          kwin Konrad Windszus added a comment -

          I would be in favor of moving Sling Validation out of contrib. What do others think?
          Radu Cotescu Since you contributed the code to Sling I applied quite some improvements/bugfixes. How would you feel about moving Sling Validation into /bundles/extensions?

          Show
          kwin Konrad Windszus added a comment - I would be in favor of moving Sling Validation out of contrib. What do others think? Radu Cotescu Since you contributed the code to Sling I applied quite some improvements/bugfixes. How would you feel about moving Sling Validation into /bundles/extensions?
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          I'd be ok with moving the validation module out of contrib if several committers support it and provided it has good automated test coverage.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - I'd be ok with moving the validation module out of contrib if several committers support it and provided it has good automated test coverage.
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          this has to be moved to a separate bundle.

          according to [recent mailing list discussion|apache-sling.73963.n3.nabble.com/Sling-Models-1-2-Validation-tt4048605.html] we should not put any dependency to contrib bundles in a main sling bundle.

          even if it is optional, it is still a hard build dependency.

          another option would be to invest more work into the sling validation bundle so it can moved out of contrib and finalized (needs discussion in mailing list).

          otherwise the next sling models release is blocked.

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - this has to be moved to a separate bundle. according to [recent mailing list discussion|apache-sling.73963.n3.nabble.com/Sling-Models-1-2-Validation-tt4048605.html] we should not put any dependency to contrib bundles in a main sling bundle. even if it is optional, it is still a hard build dependency. another option would be to invest more work into the sling validation bundle so it can moved out of contrib and finalized (needs discussion in mailing list). otherwise the next sling models release is blocked.
          Hide
          kwin Konrad Windszus added a comment -

          In rev. 1644385 I updated the model and validation documentation.

          Show
          kwin Konrad Windszus added a comment - In rev. 1644385 I updated the model and validation documentation.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -
          Show
          bdelacretaz Bertrand Delacretaz added a comment - Are you also updating http://sling.apache.org/documentation/bundles/models.html accordingly?
          Hide
          kwin Konrad Windszus added a comment -

          Fixed in rev. 1644069.

          Show
          kwin Konrad Windszus added a comment - Fixed in rev. 1644069.
          Hide
          kwin Konrad Windszus added a comment -

          I was finally able to execute the Models IT successfully and there it ran fine (without Sling Validation being installed). I locally modified the patch a bit further and removed any dependencies to Sling Validation from the ModelAdapterFacotry. I also tested to deploy Sling Validation Bundle afterwards and now the validation was working (without restarting the Sling Models Bundle) and removing Sling Validation is also working (i.e. Sling Models still work but of course no longer supporting the validation).

          Show
          kwin Konrad Windszus added a comment - I was finally able to execute the Models IT successfully and there it ran fine (without Sling Validation being installed). I locally modified the patch a bit further and removed any dependencies to Sling Validation from the ModelAdapterFacotry. I also tested to deploy Sling Validation Bundle afterwards and now the validation was working (without restarting the Sling Models Bundle) and removing Sling Validation is also working (i.e. Sling Models still work but of course no longer supporting the validation).
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          did you test this patch that it is really able to run without sling models installed? the main class ModelAdapterFactory contains some direct references to classes of sling validation, i assume because of this sling models will not work any longer without validation installed (have not tested it).

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - did you test this patch that it is really able to run without sling models installed? the main class ModelAdapterFactory contains some direct references to classes of sling validation, i assume because of this sling models will not work any longer without validation installed (have not tested it).
          Hide
          kwin Konrad Windszus added a comment - - edited

          The attached patches allow to leverage the Sling Validation through the model annotation. The dependency to Sling Validation should be optional.
          I renamed the former InvalidModelException to ModelClassException to make more clear from the name what is a validation exception and what is not.
          Also the ModelAdapterFactory might now throw two more exceptions:
          SlingValidationException (in case the sling validation model could not be instanciated) and InvalidResourceException (in case the resource is considered invalid by Sling Validation)

          Show
          kwin Konrad Windszus added a comment - - edited The attached patches allow to leverage the Sling Validation through the model annotation. The dependency to Sling Validation should be optional. I renamed the former InvalidModelException to ModelClassException to make more clear from the name what is a validation exception and what is not. Also the ModelAdapterFactory might now throw two more exceptions: SlingValidationException (in case the sling validation model could not be instanciated) and InvalidResourceException (in case the resource is considered invalid by Sling Validation)
          Hide
          kwin Konrad Windszus added a comment -

          Bertrand Delacretaz Good idea, I will consider that.
          Hopefully I can come up with a patch for that this week.

          Show
          kwin Konrad Windszus added a comment - Bertrand Delacretaz Good idea, I will consider that. Hopefully I can come up with a patch for that this week.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          IMO it would be good to specify how to handle the "validation is turned on but no ValidationModel found" case.

          How about using validate=

          {off/optional/required}

          ?

          With optional you'd just ignore the missing ValidationModel, whereas required causes validation to fail in that case.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - IMO it would be good to specify how to handle the "validation is turned on but no ValidationModel found" case. How about using validate= {off/optional/required} ? With optional you'd just ignore the missing ValidationModel, whereas required causes validation to fail in that case.
          Hide
          sseifert@pro-vision.de Stefan Seifert added a comment -

          integrating with sling models is nice, but we should avoid having this validation bundle as a mandatory dependency of the sling models bundle, it should be optional.

          Show
          sseifert@pro-vision.de Stefan Seifert added a comment - integrating with sling models is nice, but we should avoid having this validation bundle as a mandatory dependency of the sling models bundle, it should be optional.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development