-
Notifications
You must be signed in to change notification settings - Fork 490
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.
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
8531 refactor validators #8534
8531 refactor validators #8534
Conversation
…dValueValidator IQSS#8503 The context parameter has been removed, need to add the violation message ourselfs. Also adding a true test for email validation.
- Simplify the code for URLValidator - Make it nullsafe - Make allowed schemes configurable from annotation - Rewrite tests to JUnit5, more examples and test with real subject classes - Move message string to validation bundle
- Reuse existing constraints instead of creating our own validator - Provide programmatic method to use the same from code like OAuth2 etc - Add much more tests, also for actual bean validation
1c53f7f
to
9589d3f
Compare
@donsizemore what does "Ansible run terminated abnormally, failing build." on Jenkins mean? |
it means Ansible exited 1 - EC2 instance fell over, something timed out or generally went splat. |
If you can click on details, the artifacts tab has the pipeline.log and in the one for this run: |
@qqmyers I see a failing test
I don't think this is related to my changes, as I see the same failures at #8532 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
(is there any thought to allowing the min and max username size to be configurable?)
Thx ❤️
Well it hasn't been configurable for a looooong time, so maybe this is not necessary? Btw this would be hard to do with annotations, as these require constants at compile time. And IIRC the schema is generating limitations for SQL fields from this, so it might be necessary to create a schema migration. |
I wasn't saying that this was desirable or necessary - just wondering if the change here was for something beyond readability and maintainability at some point in the future. |
Nope, it maintains status quo. Guess if you'd want to do this at a later point in time, you'd remove the |
@poikilotherm would you mind refreshing from develop branch? thanks |
✔️ |
As always thx for merging @kcondon 🤩 |
What this PR does / why we need it:
This pull request pays back some technical debt around validators.
Which issue(s) this PR closes:
Closes #8531
Special notes for your reviewer:
None. I really put effort into keeping things working. I did draw a line: I did not refactor the JSF validation parts. They are as messy as before (it would make things cleaner by reusing bean validation 🤷 - less duplicated code, less different error messages.). And I did not cleanup all the code in
DatasetFieldValueValidator
, which could use some love...Suggestions on how to test this:
There is a large amount of unit tests added with this PR. I tried very hard not to introduce unexpected behavior changes, but better watch out for those.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.
Is there a release notes update needed for this change?:
Nope.
Additional documentation:
None so far.