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

JADNC: Required Input validation disabled for partial patching / relationships #781

Conversation

sasman0001
Copy link
Contributor

Fixes #472

This feature allows the Required validator to be disabled. This will allow partial patching in the case that the attribute is excluded from a patch. Validation is applied when the attribute is present in post or patch.

Required validation also disabled for a relationships attributes (in attempt to fix #472 (comment)).

@sasman0001
Copy link
Contributor Author

FYI. Integration / unit tests succeed locally, but for some reason are failing according to the "AppVeyor build." Will investigate.

Copy link
Contributor

@bart-degreed bart-degreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I agree this new approach is an improvement over what you submitted before. Thanks.

@bart-degreed
Copy link
Contributor

The cibuild failure is likely unrelated to your work. Some tests are still not properly isolated, which makes them fail occasionally (tracked at #715). Usually pushing another empty commit makes the build succeed.

-mock httpContextAccessor
-remove duplicated code
-add documentation
@sasman0001 sasman0001 marked this pull request as ready for review June 11, 2020 18:08
@bart-degreed
Copy link
Contributor

Please merge the latest master into your branch and see if all still works before we take this PR.

-Move integration tests to ModelStateValidationTests
-formatting
-documentation
-comments
@sasman0001
Copy link
Contributor Author

Bart, I pulled the latest master, but then thought since this is my fork, it may not actually be pulling the JADNC master. I am not familiar with "forking" and am unsure how to merge JADNC master into my fork.

I have made the other changes you requested. Thank you for all the great feedback.

@bart-degreed
Copy link
Contributor

bart-degreed commented Jun 15, 2020 via email


```c#
public class Person : Identifiable<int>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can inherit from the non-generic Identifiable, which is easier to understand for newcomers (see my example). Also this is a nice opportunity to show usage of the AllowEmptyStrings parameter so users will know it exists. Can you use my example from earlier comments instead?

Never mind, I'll change this.

@bart-degreed bart-degreed merged commit 423f45f into json-api-dotnet:master Jun 16, 2020
@bart-degreed
Copy link
Contributor

Thanks a lot, Sarah!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

PATCH with ValidateModelState enabled and [Required] attribute
2 participants