Skip to content

Fixed: Do not run ModelState validation for relationships in POST/PATCH #831

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 8 commits into from
Sep 18, 2020
Merged

Fixed: Do not run ModelState validation for relationships in POST/PATCH #831

merged 8 commits into from
Sep 18, 2020

Conversation

bart-degreed
Copy link
Contributor

Fixes #671.

@bart-degreed
Copy link
Contributor Author

@maurei One test is disabled, I found no way to get that one working.
Tagging @sasman0001 because she implemented IsRequired in the past and may be aware of a solution.

Ideas are welcome! :)

If we find no solution, perhaps its better to take that test out.

@sasman0001
Copy link
Contributor

sasman0001 commented Sep 15, 2020

Hey Bart. I branched from your branch to try and see if I could come up with any ideas. But when I tried to push what I came up with, it says I don't have access. So I will just describe it below:

In IsRequiredAttribute.cs I added the following to the ShouldSkipValidationForResource method:

                if (TypeHelper.IsOrImplementsInterface(validationContext.ObjectType, typeof(IHasParent)))
                {
                    var parent = (IHasParent)validationContext.ObjectInstance;
                    return parent.HasParent;
                }

then in Enterprise.cs I use the IHasParent interface and implement it as follows:

       public bool HasParent { get; set; }

        private Enterprise _parent;

        [HasOne]
        public Enterprise Parent
        {
            get => _parent;
            set
            {
                _parent = value;
                _parent.HasParent = true;
            }
        }

The IHasParent.cs only has bool HasParent { get; set; } within it.

When I make these changes and remove the skip on test When_patching_resource_with_self_reference_it_must_succeed all tests succeed.

@maurei maurei self-requested a review September 16, 2020 17:09
Copy link
Member

@maurei maurei left a comment

Choose a reason for hiding this comment

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

The result of @sasman0001 will lead to a passing test. But my objection is that it only shows a workaround that a developer can implement in the case of self-referencing models. It does not introduce a solution at the level of the library itself. I would be in favour of the latter.

We could reflect on to object which is to be validated and detect if it is self-referencing. Caching that result we can then skip the validation when appropriate.

I've made a PR to your branch so you can review my approach.

On the side: is there any reason for your branches to live on your own fork? It would be easier for me to work with your code if your branches lived in this repository.

@sasman0001
Copy link
Contributor

That makes sense. Good point @maurei.

@bart-degreed
Copy link
Contributor Author

@maurei Although creative, I felt the exact same objection about @sasman0001's solution. So I decided to wait a bit and see if you'd come up with an alternative solution. I think your approach is solid and I'm going to integrate that.

About the forking, you asked me that last week too! So let me repeat my reply: This stems from the time I wasn't a JADNC project contributor and had limited permissions like everyone else. At that time, I created some branches on my fork that I cannot close yet. I'm hoping to ditch my fork in the near future (it makes sync easier), but until then I do not want to have two JANDC repositories on my system: it is just too easy to be accidentally working in the wrong one.

@bart-degreed bart-degreed requested a review from maurei September 17, 2020 15:06
@bart-degreed bart-degreed merged commit ca717d0 into json-api-dotnet:master Sep 18, 2020
@bart-degreed bart-degreed deleted the modelstate-relationship-fix branch November 10, 2020 20:22
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.

Relations being validated on POST
3 participants