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

Fixed #3674 -- Refactored _get_reverse_relationships() to use correct to_field #3696

Closed
wants to merge 3 commits into from

Conversation

benred42
Copy link

@benred42 benred42 commented Dec 2, 2015

Fixed #3674.

Changed _get_reverse_relationships() in model_meta.py to use the pk of the related object instead of the current object's pk when assigning to_field. This corrected an issue in serializers.py where build_relational_field() would try to call _meta.get_field() in django with the wrong field.

@benred42
Copy link
Author

benred42 commented Dec 2, 2015

I'm not at all sure this is the correct or best fix. Had a little trouble wrapping my head around everything that's going on as these parameters get passed around, so I don't know if this patch will adversely affect any existing behaviour.

@tomchristie
Copy link
Member

tomchristie commented Dec 3, 2015

Okay seems valid - able to provide a test case to demonstrate the issue/fix?

@xordoquy
Copy link
Collaborator

xordoquy commented Dec 3, 2015

I really have doubts about this.
It looks to me that it'll restrict relations to the PK only.
Will this still work if there's a relation to another model with to_field being different from the PK ? That's what the to_field initially is for, isn't it ?

@xordoquy
Copy link
Collaborator

xordoquy commented Dec 3, 2015

I just realize we may need a test case with a non PK relation here (not directly related to the issue here though linked to).

@benred42
Copy link
Author

benred42 commented Dec 3, 2015

@xordoquy: That's essentially what I was worried about. I'm going to try and get a decent test set up and I'll work on making the fix less restrictive at the same time.

@xordoquy
Copy link
Collaborator

xordoquy commented Dec 3, 2015

@benred42 let me know about your progress on this.

…ship is reverse vs forward to ensure correct field checking
@benred42
Copy link
Author

benred42 commented Dec 3, 2015

Still working on the test, but here's what I have so far for a modified fix. I figured the real issue was actually in build_relational_field() in serializers.py, so I refactored it to change its behaviour depending on whether it was passed a forward or reverse relationship. Let me know what you think and I'll keep working on a test.

EDIT: I can already see that I need to modify the fix so it will work on 1.9

@xordoquy
Copy link
Collaborator

xordoquy commented Dec 3, 2015

@benred42 no worries :)

@benred42
Copy link
Author

benred42 commented Dec 7, 2015

Okay, the fix works on Django1.9 now and I have an initial test in. Looks like more test coverage is needed on the patch, however.

@xordoquy
Copy link
Collaborator

xordoquy commented Dec 7, 2015

Not sure how codecov does work here, doesn't seem to point to the right commits...

@xordoquy xordoquy added this to the 3.3.2 Release milestone Dec 7, 2015
@xordoquy
Copy link
Collaborator

xordoquy commented Dec 7, 2015

Milestoning this issue to keep it on our radars.

@tomchristie
Copy link
Member

tomchristie commented Dec 9, 2015

I think we should probably turn off the patch-level coverage threshold.

@tomchristie
Copy link
Member

tomchristie commented Dec 9, 2015

Okay - I've turned off the patch-level threshold.
Don't consider the coverage to be a block on this.

@benred42
Copy link
Author

benred42 commented Dec 9, 2015

I feel like the test could use some work. Right now it does fail without the fix and passes with it, but it seems like it should be more directly testing the issue instead of making a tangential assertion that will cause an exception to be raised if the issue is present. I could also just be being too picky.

@xordoquy xordoquy modified the milestones: 3.3.2 Release, 3.3.3 Release Dec 14, 2015
@benred42
Copy link
Author

benred42 commented Dec 14, 2015

Okay, so I refactored the test and I feel it is a better test now, but I'm not sure how consistent it is with the way the existing tests are set up, so I figured I'd paste it here and get feedback on it before pushing it up: http://dpaste.com/0DGMJ18

@xordoquy
Copy link
Collaborator

xordoquy commented Jan 20, 2016

I went ahead and merged master / added your test in #3852 which overtakes this PR.
Thanks a lot @benred42 !

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

Successfully merging this pull request may close these issues.

None yet

3 participants