-
Notifications
You must be signed in to change notification settings - Fork 169
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
Adds skipDefaultValidator API #18549
Conversation
* Tells Binder to skip executing the default validators (e.g. min/max | ||
* validators in DatePicker) of all bound fields by default. | ||
*/ | ||
public void skipDefaultValidators() { |
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.
for consistency with the other fields I'm kinda missing setSkipDefaultValidators(boolean)
Otherwise it's also impossible to-renable the validators
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.
Noted. We discussed this internally earlier today and concluded to keep it as simple as possible, so (at least for now) you can do one-time setting of skip for either whole binder or individual bindings.
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.
@knoobie Adding this method is possible of course, but I don't understand when exactly this would be needed to change this behaviour dynamically. Swapping this for the whole binder / form sounds as too much heavy logic. I would rather recommend to put the business logic into custom Binder's validators if one really needs to change validation logic on-the-fly. However, I may miss an experience of using Binder in more advanced cases, so could you please share your thoughts how setSkipDefaultValidators(boolean)
could be used?
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.
Only thing that I could think of would be highly dynamic forms or even form builder that do things based on given objects and re-create all bindings.. but not sure how widely that's used.
I'm probably just biased because I Like side-effect free code.. or let's say, code that can be changed back once I messed-up.. now this method is a once used and it's impossible to change back (thinking unit tests for the binder for example) and it does not match all other flags the binder has, where it's possible to change them at "runtime" making it inconsistent to the API design. Personally I don't think there is much logic to consider by just replacing the direct access to the boolean with the getter from it, ensuring you are always getting the correct value the user supplied last - or do I miss something?
TL;DR: I'm not seeing the benefits of an inconsistent API
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.
We discussed with @tepi on daily and it makes sense for a sake in consistent API to add setSkipDefaultValidators(boolean)
methods as you suggested.
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.
Thanks for reconsidering!
flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
Outdated
Show resolved
Hide resolved
flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
Outdated
Show resolved
Hide resolved
flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
Outdated
Show resolved
Hide resolved
@@ -1247,6 +1292,8 @@ public BindingImpl(BindingBuilderImpl<BEAN, FIELDVALUE, TARGET> builder, | |||
this.asRequiredSet = builder.asRequiredSet; | |||
converterValidatorChain = ((Converter<FIELDVALUE, TARGET>) builder.converterValidatorChain); | |||
|
|||
defaultValidatorsSkipped = builder.defaultValidatorsSkipped; |
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.
What happens if you create a new Binder, bind all the fields and only after this call skipDeafultValidators()
. Would it be skipped in this case ?
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.
expectation: yes, it should be skipped - perfect example for a unit test :)
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.
My expectation is either yes, need to be tested.
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.
Yes, changing either the Binder's or Binding's property can be done at anytime. The final value is evaluated at validation time. BindingBuilder's value can not be changed after the binding is built, but it does not matter since it is used only as the initial value.
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.
Good! Could you add one more unit test to cover this?
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.
I'll modify the unit tests to test dynamic property changing and validation as well. Thanks for the input!
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.
Added unit tests for dynamically toggling enabled/disabled state
* @return {@literal true} to skip default validators for this binding, | ||
* {@literal false} to evaluate them | ||
*/ | ||
boolean isDefaultValidatorsSkipped(); |
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.
I'm concerned about the two 's' in the new version of the method names. It negatively affects their readability. Also, the naming like setSomethingSkipped seems a bit uncommon in our codebases.
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.
Good point. Maybe we could go for something more like this existing API:
/**
* Control whether validators including bean level validators are disabled
* or enabled globally for this Binder.
*
* @param validatorsDisabled
* Boolean value.
*/
public void setValidatorsDisabled(boolean validatorsDisabled) {
this.validatorsDisabled = validatorsDisabled;
}
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.
I don't mind a name change, but it's probably important that Default stays in there so that people don't think this would disable all validations.. which could also be interesting for people that wanna store an un-validated draft of a form.. but that's another story..
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.
Changed names in API to setDefaultValidatorsDisabled
etc.
Validation fails because of javadoc error:
|
…m:vaadin/flow into feat/17178-disable-default-validators-api
@vursen I'm ready to merge it, but waiting for your feedback, does it look good to Design System team ? |
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Should we use the singular form for the method name on the Binder instance too? What do you think? |
Then it might kind of clash with this existing API:
Now that I think of it, might have been better to call these |
I personally see no problems with plurals/singulars with the current implementation: on Binder level you configure |
This patch fixes a regression in Binder introduced by adding skipDefaultValidator API (#18549). This issue was missed since it seems to be impossible/difficult to trigger via unit testing using the mock field components in Flow project. Affected flow-components tests: UpdateEditorComponentIT DynamicEditorKBNavigationIT GridViewEditorIT.dynamicNotBufferedEditor_closeEditorUsingKeyboard GridViewEditorIT.dynamicNotBufferedEditor GridViewEditorIT.dynamicNotBufferedEditor_navigateUsingKeyboard Fixes #18608
* Adds skipDefaultValidator API * Simplify API * Add tests * Rename method * Change API to allow toggling skip default validators for Binder and Binding * Cleanup * API naming changes; Add tests for dynamic disable/enable * Fix javadoc error * Change plural validators to validator for binding * Formatting
This patch fixes a regression in Binder introduced by adding skipDefaultValidator API (#18549). This issue was missed since it seems to be impossible/difficult to trigger via unit testing using the mock field components in Flow project. Affected flow-components tests: UpdateEditorComponentIT DynamicEditorKBNavigationIT GridViewEditorIT.dynamicNotBufferedEditor_closeEditorUsingKeyboard GridViewEditorIT.dynamicNotBufferedEditor GridViewEditorIT.dynamicNotBufferedEditor_navigateUsingKeyboard Fixes #18608
This ticket/PR has been released with Vaadin 24.4.0. |
Provides means to skip default validators of bound fields. Skipping can be done either for the Binder instance (skips all), or per-binding via BindingBuilder.
Fixes #17178