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

Removing relationship through empty PATCH request #244

Closed
tpilara opened this issue May 30, 2016 · 8 comments · Fixed by #788
Closed

Removing relationship through empty PATCH request #244

tpilara opened this issue May 30, 2016 · 8 comments · Fixed by #788

Comments

@tpilara
Copy link
Contributor

tpilara commented May 30, 2016

According to the JSON API spec, a PATCH request made to the relationship view with a null data object (None in the case of a to-one relationship, [] in the case of a to-many) should result in the relationship being cleared. Currently passing either in a PATCH request to a relationship view throws the error: "Received document does not contain primary data".

Problem
This line in the parse() method grabs the 'data' key from the JSON request and then checks to make sure the data evaluates to True. The problem is that both None and [] will evaluate to False.

Solution
I think this can be solved by moving the RelationshipView check above the line that checks the data key. I believe that returning either None or [] from this function for a RelationshipView will result in the relationship being cleared correctly.

@jerel
Copy link
Member

jerel commented Jun 7, 2016

@tpilara a PR with a test would be very welcome

@tpilara
Copy link
Contributor Author

tpilara commented Jun 10, 2016

@jerel I'm definitely going to try, I just wanted to document the issue in case someone gets to it first. I realized it'll require a little more than I wrote here, and will probably require a fix for issue #242 first too. I'll see if I can devote some time to these soon and get them resolved.

@Koed00
Copy link

Koed00 commented Dec 15, 2016

I'm now running into a similar issue. Patching the relationship data to null gives me the error "The resource object's type (None) is not the type that constitute the collection represented by the endpoint (user)." This means I can't delete any relationships once they are set.
@tpilara did you make any headway with this?

@tpilara
Copy link
Contributor Author

tpilara commented Dec 16, 2016

@Koed00 Unfortunately I didn't. We don't currently patch to relationship routes, partially because I'm still worried about #242 and I never got time to circle back to this issue.

If you have time to take a stab at this I'd be happy to take a look at the code.

@Alig1493
Copy link
Contributor

Alig1493 commented Oct 7, 2018

Is it possible to make a contribution to this? If it is then some guidance would be helpful.

@sliverc
Copy link
Member

sliverc commented Oct 8, 2018

@Alig1493 I guess best way to start is to write a in test_views.py to reproduce this issue and try to fix it from there.

@jennifer-richards
Copy link

I have been poking at this and may have a fix. Before cleaning it up and submitting a PR I want to check that I am fixing the right issue. (I'm new to JSON API.) The failing test that passes with my patch is:

    def test_patch_to_one_relationship_with_null(self):
        url = '/comments/{}/relationships/author'.format(self.second_comment.id)  # second_comment has an author
        request_data = {
            'data': None
        }
        response = self.client.patch(url, data=request_data)
        assert response.status_code == 200, response.content.decode()
        assert response.data is None
        self.second_comment.refresh_from_db()
        assert self.second_comment.author is None

Does this look like the right thing?

Incidentally, I have also noticed that going the other way - patching related resource to an empty to-one relationship - seems not to be working. I get Incorrect model type. Expected <class 'NoneType'>, received authors. (using the models intest_views.py).

@sliverc
Copy link
Member

sliverc commented Apr 10, 2020

@jennifer-richards It would be great if you could work on this issue. So the JSON API reference to this issue is here

And when I look at your test code snippet this looks like it follows the spec.

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