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

Proper handling of custom m2m relationships #1093

Conversation

PiotrKurnik
Copy link

@PiotrKurnik PiotrKurnik commented Jan 3, 2023

django-simple-history does not work with custom through tables for m2m relations.

Description

Code snippet below illustrates the issue. There are assumptions to the name of the fields in through tables for m2m tables.
For the default through tables it works, but for the custom ones it does not.

If you try to save object of PageArticle you will see:

django.core.exceptions.FieldError: Cannot resolve keyword 'pagearticle' into field. Choices are: article, article_id, id, tag, tag_id
from django.db import models
from simple_history.models import HistoricalRecords

class Tag(models.Model):
    name = models.CharField(max_length=100)

class ArticleTag(models.Model):
    article = models.ForeignKey("PageArticle", on_delete=models.CASCADE)
    tag = models.ForeignKey(Tag, on_delete=models.CASCADE)

class PageArticle(models.Model):
    title = models.CharField(max_length=100)
    article_tags = models.ManyToManyField(Tag, through=ArticleTag)

    history = HistoricalRecords(m2m_fields=[
        article_tags,
    ])


Related Issue

#1079

Motivation and Context

We need to use model introspection to find the correct field name and not to relate on model name.

How Has This Been Tested?

Unit test has been added.

Screenshots (if appropriate):

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have run the pre-commit run command to format and lint.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have added my name and/or github handle to AUTHORS.rst
  • I have added my change to CHANGES.rst
  • All new and existing tests passed.

@PiotrKurnik PiotrKurnik changed the title Added _get_through_field_name method which handles custom through tab… Proper handling of custom m2m relationships Jan 3, 2023
@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Merging #1093 (eb732e1) into master (309609e) will decrease coverage by 0.15%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1093      +/-   ##
==========================================
- Coverage   97.23%   97.08%   -0.16%     
==========================================
  Files          23       23              
  Lines        1231     1236       +5     
  Branches      199      202       +3     
==========================================
+ Hits         1197     1200       +3     
  Misses         16       16              
- Partials       18       20       +2     
Impacted Files Coverage Δ
simple_history/models.py 96.34% <66.66%> (-0.38%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@PiotrKurnik PiotrKurnik force-pushed the bug-with-custom-through-table-in-m2m-field branch from d7cefbb to 2f759b2 Compare January 3, 2023 10:34
@PiotrKurnik PiotrKurnik marked this pull request as ready for review January 3, 2023 10:40
Find the name of the field in the through table. This is necessary for the
custom through tables where Django conventions don't apply.
"""
foreign_keys = filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

@lazarust
Copy link

lazarust commented Jan 3, 2023

@PiotrKurnik I saw you tagged #1079 on this issue but this PR but it doesn't seem to allow adding custom table names for each of the m2m fields. Am I missing how to set the custom table names?

@ddabble ddabble linked an issue Sep 26, 2023 that may be closed by this pull request
@ddabble
Copy link
Member

ddabble commented Sep 26, 2023

Looks like #1218 fixes this issue - at least the tests added in this PR and the example provided in #1094 now work without any errors, and they stop working when reverting the changes made in models.py - so I'll close this.

Please reopen if it turns out that the issue was not resolved 🙂

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

Successfully merging this pull request may close these issues.

Custom m2m field are not handled correctrly
4 participants