Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make Range and FieldRange validators have consistent logic and use default message key #930

Closed
3 tasks done
sleberknight opened this issue Mar 31, 2023 · 0 comments · Fixed by #955
Closed
3 tasks done

Make Range and FieldRange validators have consistent logic and use default message key #930

sleberknight opened this issue Mar 31, 2023 · 0 comments · Fixed by #955
Assignees
@sleberknight
Labels
enhancement A request for change or improvement to an existing feature
Milestone
2.6.0

Comments

@sleberknight
Copy link
Member

sleberknight commented Mar 31, 2023 •
edited
Loading

Both Range and FieldRange use "{org.kiwiproject.validation.Range|FieldRange.between.message}" as the default for message(). This is problematic because:

  • RangeValidator and FieldRangeValidator never use that message key
  • Our ValidationMessages.properties files does not contain this key

These two validators actually use the above keys but suffixed with minMaxValues or minMaxLabels. For example Range uses:

  • org.kiwiproject.validation.Range.between.message.minMaxValues
  • org.kiwiproject.validation.Range.between.message.minMaxLabels

It chooses which one based on whether there are labels or not.

Last, there is some weird logic in both these validators that appears to permit someone using them without specifying any of min, minLabel, max, or maxLabel, in that case it seems that the validator (at least @Range) will then consider any value as valid. This is probably not what we want. @FieldRange is different since it will compare two separate fields. However, its logic should be checked as well.

So, the things in this task are to:

  • Evaluate the logic in both these validators
  • Make any changes necessary to make the logic consistent
  • Make sure we don't reference a non-existing message key in the annotations

There might be more, but these are some first high-level tasks.
@sleberknight sleberknight added the enhancement A request for change or improvement to an existing feature label Apr 1, 2023
@sleberknight sleberknight self-assigned this Apr 2, 2023
@sleberknight sleberknight added this to the 2.6.0 milestone Apr 22, 2023
sleberknight added a commit that referenced this issue Apr 22, 2023
@sleberknight
Make Range and FieldRange use an existing message key
d0243f7 
* Add custom "unknownError" property to ValidationMessages.properties
  to be used by both RangeValidator and FieldRangeValidator
* Replace message in FieldRange with new "unknownError" property
* Change FieldRangeValidator to use "unknownError" when adding
  constraint violations for unknown errors
* Update javadocs in Range to explain that it must have at least a
  min or max attribute, and why this can only be checked at runtime
* Replace message in Range with a property that actually exists in
  ValidationMessages.properties
* In RangeValidator#initialize check that a min or max exists; if not
  throw IllegalStateException
* Minor code cleanup in RangeValidator: cast value to Comparable<Object>
  and remove the (now redundant) 'rawtypes' warning suppression

Closes #930
@sleberknight sleberknight mentioned this issue Apr 22, 2023
Merged
chrisrohr pushed a commit that referenced this issue Apr 24, 2023
@sleberknight
Make Range and FieldRange use an existing message key (#955)
88dad2f 
* Add custom "unknownError" property to ValidationMessages.properties
  to be used by both RangeValidator and FieldRangeValidator
* Replace message in FieldRange with new "unknownError" property
* Change FieldRangeValidator to use "unknownError" when adding
  constraint violations for unknown errors
* Update javadocs in Range to explain that it must have at least a
  min or max attribute, and why this can only be checked at runtime
* Replace message in Range with a property that actually exists in
  ValidationMessages.properties
* In RangeValidator#initialize check that a min or max exists; if not
  throw IllegalStateException
* Minor code cleanup in RangeValidator: cast value to Comparable<Object>
  and remove the (now redundant) 'rawtypes' warning suppression

Closes #930
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sleberknight sleberknight

Labels
enhancement A request for change or improvement to an existing feature
Projects
None yet
Milestone
2.6.0
Development

Successfully merging a pull request may close this issue.

Make Range and FieldRange use an existing message key kiwiproject/kiwi
1 participant
@sleberknight