Skip to content

Add tests for M2M relations with inheritance and a through model #3918

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

Closed
wants to merge 1 commit into from

Conversation

zorun
Copy link

@zorun zorun commented Feb 10, 2016

This particular setup worked fine up to DRF 3.2.5, but is broken in DRF
3.3.0. Bisecting indicates that 322bda8
first introduced the regression.

This commit adds a few testcases that pass with DRF 3.2.5 but fail with
DRF 3.3.0 and newer.

This particular setup worked fine up to DRF 3.2.5, but is broken in DRF
3.3.0.  Bisecting indicates that 322bda8
first introduced the regression.

This commit adds a few testcases that pass with DRF 3.2.5 but fail with
DRF 3.3.0 and newer.
@zorun
Copy link
Author

zorun commented Feb 14, 2016

Note that the CI checks have failed for this PR. This is perfectly normal, since the new tests exercise a regression :)

@tomchristie
Copy link
Member

Thanks!

@tomchristie
Copy link
Member

Could you also outline the simplest possible example case, what you would expect the behavior to be, and what the behavior currently is?

@zorun
Copy link
Author

zorun commented Feb 15, 2016

The simplest possible example case is unfortunately not so simple, it's the one in the new tests introduced by this PR. Basically:

The expected behaviour is that DRF is able to serialize the objects (for all three models). The current behaviour (starting with 322bda8) is that DRF throws an exception when serializing:

django.core.exceptions.FieldDoesNotExist: ManyToManyThrough has no field named 'manytomanythroughtarget_ptr'

ManyToManyThrough in the test corresponds to model C in the description above. It seems that the reverse related name between model C and model A is not correctly detected by DRF (but I don't know enough about the internals of DRF to debug this).

See https://travis-ci.org/tomchristie/django-rest-framework/jobs/108339903 for an example of failure.

@tomchristie
Copy link
Member

Blimey - that's a pretty obscure use-case corner you've found there.

@zorun
Copy link
Author

zorun commented Jul 1, 2016

Bump? I still get the same error with DRF 3.3.3 and Django 1.9.7.

As a reminder, I had bisected the issue to this commit: 322bda8

@tomchristie
Copy link
Member

tomchristie commented Jul 4, 2016

Still not high enough priority just now, given edge-case-ness. Focus is on 3.4.0 first.
Proposed solutions / pull requests most welcome (might help raise the priority as further work on it then has better effort/reward ratio), but we've 179 open issues and pull requests, so there's plenty in the queue.

@tomchristie tomchristie added this to the 3.4.5 Release milestone Aug 10, 2016
@tomchristie tomchristie modified the milestones: 3.4.7 Release, 3.4.8 Release Sep 21, 2016
@tomchristie tomchristie modified the milestones: 3.4.8 Release, 3.5.1 Release Sep 30, 2016
@carltongibson
Copy link
Collaborator

I'm going to de-milestone for now. We can reassess after v3.7

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

It would be great to know, if this change is still needed with django 2.0+ versions and latest drf versions? hope you would take the time to consider verifying the validity of proposed change with newer releases.

@tomchristie
Copy link
Member

I'm just going to close this off as stale at this point - can reassess if it gets bumped sometime.

@tomchristie tomchristie closed this Sep 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants