BVal
  1. BVal
  2. BVAL-38

Groups and payload values must be part of the ConstraintDescriptor attributes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.1-incubating
    • Fix Version/s: 0.1-incubating
    • Component/s: jsr303
    • Labels:
      None

      Description

      When querying the validation metadata, the returned ConstraintDescriptor must have 'groups' and 'payload' attributes.

      1. bval-jsr303-38-setters.patch
        0.7 kB
        Carlos Vara
      2. bval-jsr303-38-extra-test.patch
        3 kB
        Carlos Vara
      3. bval-jsr303-38.patch
        4 kB
        Carlos Vara

        Activity

        Hide
        Carlos Vara added a comment -

        Patch and 1 test.

        1 more test passes.

        Show
        Carlos Vara added a comment - Patch and 1 test. 1 more test passes.
        Hide
        Roman Stumm added a comment -

        Whether groups and payload are expected to appear in the attributes was a question, for which I had AnnotationConstraintBuilder.buildFromAnnotation in mind. Thanks for clarifing this! I decided to fix this in a different way, so that the setters in ConstraintValidation stay simple and the AnnotationConstraintBuilder handles the map of attributes.

        Show
        Roman Stumm added a comment - Whether groups and payload are expected to appear in the attributes was a question, for which I had AnnotationConstraintBuilder.buildFromAnnotation in mind. Thanks for clarifing this! I decided to fix this in a different way, so that the setters in ConstraintValidation stay simple and the AnnotationConstraintBuilder handles the map of attributes.
        Hide
        Carlos Vara added a comment -

        Ah, I missed your comment in that class. It's indeed a better idea to have a cleaner setter. Only thing I'm wondering now is whether it will work properly when overriding the groups (when composing constraints for example), as they are set with that setter after the constraint has been built.

        Show
        Carlos Vara added a comment - Ah, I missed your comment in that class. It's indeed a better idea to have a cleaner setter. Only thing I'm wondering now is whether it will work properly when overriding the groups (when composing constraints for example), as they are set with that setter after the constraint has been built.
        Hide
        Roman Stumm added a comment -

        Good point, then maybe your patch should be still neccessary... I reopen the issue.

        Show
        Roman Stumm added a comment - Good point, then maybe your patch should be still neccessary... I reopen the issue.
        Hide
        Roman Stumm added a comment -

        see comment of Carlos about composing constraints

        Show
        Roman Stumm added a comment - see comment of Carlos about composing constraints
        Hide
        Carlos Vara added a comment -

        A test to check if 'groups' attribute value is correctly updated when inheriting groups.

        Show
        Carlos Vara added a comment - A test to check if 'groups' attribute value is correctly updated when inheriting groups.
        Hide
        Carlos Vara added a comment -

        Test fails with current codebase. It can be fixed changing the setter again, or modifying the attributes hashmap when inheriting. I think changing the setter is a better idea, but both ways are fine with me.

        Also, you decide whether it's you or me the one who fixes it

        Show
        Carlos Vara added a comment - Test fails with current codebase. It can be fixed changing the setter again, or modifying the attributes hashmap when inheriting. I think changing the setter is a better idea, but both ways are fine with me. Also, you decide whether it's you or me the one who fixes it
        Hide
        Carlos Vara added a comment -

        Maybe removing the groups field from ConstraintValidation and using always the attributes map is the cleaner way, so that we don't have to maintain two references for the same data.

        Show
        Carlos Vara added a comment - Maybe removing the groups field from ConstraintValidation and using always the attributes map is the cleaner way, so that we don't have to maintain two references for the same data.
        Hide
        Carlos Vara added a comment -

        Patch that adds functionality to the setters to keep the attributes map up to date.

        I decided to go with that route because removing groups and payload field involves converting types in a method that is probably used a lot, as the groups/payloads are stored as arrays in the attribute map and as sets in the individual fields.

        Test uploaded in this issue passes with this patch applied.

        Of course, if you prefer to fix it any other way, just ignore this patch.

        Btw, I have another patch to fix implicit groups lined up for when this issue is closed

        Show
        Carlos Vara added a comment - Patch that adds functionality to the setters to keep the attributes map up to date. I decided to go with that route because removing groups and payload field involves converting types in a method that is probably used a lot, as the groups/payloads are stored as arrays in the attribute map and as sets in the individual fields. Test uploaded in this issue passes with this patch applied. Of course, if you prefer to fix it any other way, just ignore this patch. Btw, I have another patch to fix implicit groups lined up for when this issue is closed
        Hide
        Roman Stumm added a comment -

        used your changes, reverted my change in AnnotationConstraintBuilder. sorry for the delay, I was busy and out-of-office for some days.

        Show
        Roman Stumm added a comment - used your changes, reverted my change in AnnotationConstraintBuilder. sorry for the delay, I was busy and out-of-office for some days.
        Hide
        Carlos Vara added a comment - - edited

        No problem at all, I have also been a little busier this week.

        I will shortly upload the next fix.

        Show
        Carlos Vara added a comment - - edited No problem at all, I have also been a little busier this week. I will shortly upload the next fix.
        Hide
        Matt Benson added a comment -

        closing issues associated with existing releases

        Show
        Matt Benson added a comment - closing issues associated with existing releases

          People

          • Assignee:
            Unassigned
            Reporter:
            Carlos Vara
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development