-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Add option to override property constraints in child class #36155
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
[Validator] Add option to override property constraints in child class #36155
Conversation
This would solve #15950, right? |
I'll start by saying I'm not a fan of inheritance, because it so often causes these kinds of issues, so I'd usually look for a way of solving this in a different way. That said, I appreciate some codebases might not be able to avoid it. So my opinions:
|
There are a few of that type already, e.g. AtLeastOneOf, EnableAutoMapping.
Yes. Currently - apart from my PR - constraints are merged even for private properties. |
Good point that this is already a pattern used elsewhere. For me personally, it would help if I could understand how properties and constraints behave with inheritance. Could I help with that, or have you already got something you could share? |
The validation process creates metadata classes, that contain information like constraints against which an object and its properties should be validated, TraversalStrategy, etc. PropertyMetadata, which contains metadata regarding object's property, uses |
@przemyslaw-bogusz can you please update the description and link to the fixed issue? Note that we recommend keeping all rows in the template table. |
4521d60
to
3a230f2
Compare
What about making this an option in the base |
@przemyslaw-bogusz Do you have time to take feedback into account? |
@przemyslaw-bogusz Do you think you will have time to finish this PR in the next few weeks? |
I've been thinking about @xabbuh 's suggestion and I can see one problem with this approach. In the current proposal, if you choose to override constraints, you can either give the child class same set of constraints but with different options or a completely different set of constraints. For example, if the parent class has
If we make this an option in the Please correct me if I'm wrong. Nevertheless, I will gladly amend the PR, if it's the only way for it to be approved. |
We've just moved away from |
What can I do to this PR to merge/finish? |
Can we please have this PR merged? |
Currently, property constraints defined in a child class are merged with property constraints of parent class. I believe it's sometimes necessary to override them instead. This PR allows this option to be configurable.
Example