ModelValidation: Fixed MaxDepth exception when there are more items left then MaxValidationDepth after reaching MaxAllowedErrors #18212
Conversation
src/Mvc/Mvc.Core/test/ModelBinding/Validation/DefaultObjectValidatorTests.cs
Show resolved
Hide resolved
src/Mvc/Mvc.Core/test/ModelBinding/Validation/DefaultObjectValidatorTests.cs
Show resolved
Hide resolved
| @@ -441,8 +448,6 @@ public void Dispose() | |||
| _visitor.Metadata = _metadata; | |||
| _visitor.Model = _model; | |||
| _visitor.Strategy = _strategy; | |||
|
|
|||
| _visitor._currentPath.Pop(_newModel); | |||
There was a problem hiding this comment.
This part seems like a big red flag (in the existing code, not in your changes). Seeing pushes and pops so disconnected from each other is a good clue that we had a bug here, your changes make this much easier to reason about 👍
Pilchie
added
the
area-mvc
|
Hi @rynowak ! |
|
Oops! didn't realize I forgot to update the reference assembly! |
There was a problem hiding this comment.
These changes seem fine to me. All the tests pass and it seems cleaner, so as long as @rynowak (who seems to have some existing knowledge in this area) is happy I'm ready to merge this.
| @@ -3256,7 +3256,7 @@ public partial class ValidationVisitor | |||
| { | |||
| private readonly object _dummy; | |||
| private readonly int _dummyPrimitive; | |||
| public StateManager(Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationVisitor visitor, object newModel) { throw null; } | |||
| public StateManager(Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationVisitor visitor) { throw null; } | |||
There was a problem hiding this comment.
This is a breaking change to a public API (protected nested class of a public class). Can you add back the old constructor even if it's not used? We try to avoid API breaks when they are low value.
There was a problem hiding this comment.
sure, makes sense!
|
Sorry, I didn't realize y'all were waiting on me. We need to address this feedback and then this PR is ready. |
|
and then he clicks the wrong button as he apologizes.... |
|
😂 I often click it as well. |
In the Model Validation, when MaxAllowedErrors is reached, the following models are pushed but not popped from the stack, raising incorrectly a InvalidOperationException if the number of remaining items is more then MaxValidationDepth.
Addresses #13778
Probably is better to look at the commits individually, as given I consolidated a test the diff is not optimal:
1- Consolidated two similar tests to a test theory
2 - Actual fix (later refactored) and related test case to reproduce the issue
3 - Refactored Visitor logic to reduce the risk of a missing Pop
/cc @ryanbrandenburg