Skip to content
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

Validate nested sub-properties #13986

Merged
merged 8 commits into from Sep 4, 2019
Merged

Validate nested sub-properties #13986

merged 8 commits into from Sep 4, 2019

Conversation

pranavkm
Copy link
Contributor

No description provided.

@guardrex
Copy link
Collaborator

guardrex commented Aug 23, 2019

@pranavkm Where does HomeAddress come from? Is that supposed to be Address?

@pranavkm
Copy link
Contributor Author

Fixed

@Rick-Anderson
Copy link
Contributor

@SteveSandersonMS one minute review please.

@SteveSandersonMS
Copy link
Member

I'm not sure this approach is quite right. The IValidatableObject technique is going to end up registering error messages with the wrong keys. I think @pranavkm also saw this happening.

Making this work cleanly involves some logic changes inside <DataAnnotationsValidator> to make it recognize the concept of nested objects. I posted a sample of this here: https://gist.github.com/SteveSandersonMS/86a0bd3be0826f57af1ae2d3bd5fa95c. However that is a bit complicated to have in docs! Our intention is to bake this functionality into the framework in 3.1 (probably - cc @danroth27 @mkArtakMSFT).

As for what we should document for 3.0, I'm not sure.

@pranavkm What problems occur due to the incorrect dictionary keys? Is it good enough that we can tell people to do this in 3.0 then change the advice in 3.1?

@pranavkm
Copy link
Contributor Author

@pranavkm What problems occur due to the incorrect dictionary keys? Is it good enough that we can tell people to do this in 3.0 then change the advice in 3.1?

I should have kept a gif of an app showing the repro. But since the keys are different, you can get field validaton and submit validation duplicated for the same field. In addition, submit validation would not "clear" the state of field validation. Both of these are not issues for top level properties where the keys match.

My plan was to suggest this, since it's a well-known DataAnnotations primitive until we have a better solution in 3.1.

@SteveSandersonMS
Copy link
Member

I don't think we should suggest an approach that leads to buggy behavior (e.g., duplicated validation error messages). I'd prefer to hold off on giving any advice until we have a reasonable story for it.

@pranavkm
Copy link
Contributor Author

Updated the text to point to a sample.

@guardrex
Copy link
Collaborator

guardrex commented Aug 27, 2019

Is it ready? I'd like to change that so that the sample link is under suitable link text.

... and a few other nits: 2nd person, contraction, "need."

@pranavkm
Copy link
Contributor Author

The sample is still in PR: aspnet/samples#44. I just want some eyes on it before it's merged.

@guardrex
Copy link
Collaborator

guardrex commented Aug 27, 2019

I'll stand by until the dust has settled on everything. The changes are nits:

  • Proper link for the sample
  • Remove 2nd person
  • Use a contraction
  • Drop 'needs' (i.e., inanimate objects don't have 'needs,' so we use 'requirements or 'specifies or something like that)

I'll fix it up at the end.

@pranavkm
Copy link
Contributor Author

pranavkm commented Aug 29, 2019

@guardrex the sample was merged - https://github.com/aspnet/samples/tree/master/samples/aspnetcore/blazor/Validation. All yours

@guardrex
Copy link
Collaborator

@pranavkm Tough to describe! 😄 See if the update is sane. If so, :shipit:.

@pranavkm
Copy link
Contributor Author

pranavkm commented Sep 4, 2019

Let's get this merged in 👍

@guardrex guardrex merged commit bdc13df into master Sep 4, 2019
@guardrex guardrex deleted the pranavkm-patch-3 branch September 4, 2019 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants