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.

Sign up for GitHub

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

Jump to bottom

Try to invalidate m2m in more robust way #94

Merged
merged 2 commits into from
Jul 11, 2014
Merged

Try to invalidate m2m in more robust way #94

merged 2 commits into from
Jul 11, 2014

Conversation

ttyS15
Copy link
Contributor

@ttyS15 ttyS15 commented Jun 11, 2014

Hi!

I faced with bug when m2m is used with multi-table inheritance model. But I think that similar bug can occur, when the handmade through-model is used in m2m relation (i.e. through-model field names are not correspond to the names of related models)

The changes contain fix for such cases. Tests are included. Please review these.

Thanks!

@Suor
Copy link
Owner

Suor commented Jun 11, 2014

First, multitable inheritance is not supported at all, it's stated in README, see CAVEATS, 8. Also, take a look at #31 and these expected failures. Your patch won't fix that.

Regarding m2m invalidation for explicit through models, it works. Regular post_save and post_delete signals are sent for them, which makes everything work with new related invalidation. The m2m machinery, which you fear will not work on customly named fields, is entirely skipped for them.

@Suor Suor closed this Jun 11, 2014
@ttyS15 ttyS15 mentioned this pull request Jul 1, 2014
Closed
@Suor
Copy link
Owner

Suor commented Jul 3, 2014

Please rebase so that there are no merge commits. Also note that there is no requirements.txt anymore.

@Suor Suor reopened this Jul 3, 2014
@ttyS15
Copy link
Contributor Author

ttyS15 commented Jul 3, 2014

Hi!

Sure, I'll do it tomorrow.
On Jul 3, 2014 2:17 PM, "Alexander Schepanovski" [email protected]
wrote:

Please rebase so that there are no merge commits. Also note that there is
no requirements.txt anymore.

Reply to this email directly or view it on GitHub
#94 (comment).

@ttyS15
Copy link
Contributor Author

ttyS15 commented Jul 4, 2014

Rebased. Please check.

Suor
Suor reviewed Jul 4, 2014
View reviewed changes
cacheops/query.py
@@ -536,19 +536,24 @@ def invalidate_m2m(sender=None, instance=None, model=None, action=None, pk_set=N
if not sender._meta.auto_created:
return

for m2m in instance._meta.many_to_many:
if (m2m.rel.through == sender
and m2m.rel.to == model):
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any point in second check? I think it's impossible to use single through model for several m2ms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so too, but I`m not sure. I can eliminate it if you wish.

Suor added a commit that referenced this pull request Jul 11, 2014
@Suor
Merge pull request #94 from ttyS15/master
90ba500 
Try to invalidate m2m in more robust way
@Suor Suor merged commit 90ba500 into Suor:master Jul 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ttyS15 @Suor