Skip to content

Allow ValidationVisitor.ValidateComplexTypesIfChildValidationFails to be configured via MvcOptions #9312

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

Merged
merged 6 commits into from
May 5, 2019

Conversation

bordecal
Copy link
Contributor

Add MvcOptions.ValidateComplexTypesIfChildValidationFails property to allow configuration of ValidationVisitor generated by DefaultObjectValidator

Addresses #8519

@dnfclas
Copy link

dnfclas commented Apr 12, 2019

CLA assistant check
All CLA requirements met.

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Apr 12, 2019
@bordecal bordecal marked this pull request as ready for review April 14, 2019 20:45
@bordecal
Copy link
Contributor Author

Some notes to reviewers:

If you have concerns about the test modifications and additions, here are a few details regarding why I chose the current approaches:

  • For the integration tests, I chose to replace the inheritance approach since the main goal of this changeset is to remove the need to subclass in order to control the value of ValidationVisitor.ValidateComplexTypesIfChildValidationFails. If you would like the inheritance approach to remain in the tests, I can certainly restore it and add additional tests for the non-inheritance scenario.
  • For the functional tests, I originally put the non-override scenarios in InputObjectValidationTests and created a separate test class for the override scenarios. However, I ended up feeling that the relationship between the tests in the two classes was less than obvious, and the extent of duplication was pretty painful as well. I've moved the tests for both the override and non-override scenarios into a separate test class (InputParentValidationTests), with a nested class for each scenario. I did a bit of duplication reduction by having each of these inherit from a common base class, but not as much as I usually would since the existing functional tests do seem to avoid aggressive duplication reduction (presumably in favour of test readability). If you would prefer an alternate approach (e.g.: no nesting and/or no inheritance, etc.), just let me know.
  • For the functional tests, I also built the request content via JSON serialization rather than a string literal for the sake of readability and ease of traceability. On the other hand, I'm not sure how deliberate the decision to use string literals extensively in the other validation tests may have been, and I can certainly change the approach if you would prefer that string literals be used exclusively for such scenarios.

@pranavkm pranavkm merged commit 2d33c32 into dotnet:master May 5, 2019
@pranavkm
Copy link
Contributor

pranavkm commented May 5, 2019

Thanks foe the PR!

@bordecal
Copy link
Contributor Author

bordecal commented May 6, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants