Fixed #9475 -- Allowed RelatedManager.add(), create(), etc. for m2m with a through model.#8981

Merged

timgraham merged 1 commit intodjango:masterfrom

collinanderson:ticket9475

Jan 15, 2019

Contributor

collinanderson commented Aug 26, 2017 •

edited

Loading

https://code.djangoproject.com/ticket/9475

collinanderson force-pushed the ticket9475 branch 2 times, most recently from 066b8ae to e42d74d Compare

August 26, 2017 15:56

Member

claudep commented Aug 28, 2017

Very promising!
Do we have a test where the through table requires values and through_defaults is empty?

collinanderson force-pushed the ticket9475 branch 2 times, most recently from 967256d to e0be1e9 Compare

September 7, 2017 01:02

Contributor Author

collinanderson commented Sep 7, 2017

Ok I added some docs and added a test for a case where the through table requires a value.

collinanderson force-pushed the ticket9475 branch 2 times, most recently from 55f841d to 151e5c1 Compare

January 16, 2018 16:20

airstandley commented Jan 16, 2018

Shouldn't line 955 be self.add(*objs, through_defaults=through_defaults)?

airstandley pushed a commit to LinearSystemsInc/django that referenced this pull request


          Applied changes from django#8981. Added 2.0's set() method to ManytoM…

e88da2a

…anyManager because that seems easier than trying to fix the problem in __set__ which is deprecated for 2.0

collinanderson force-pushed the ticket9475 branch from 151e5c1 to f84e32e Compare

January 16, 2018 19:46

Contributor Author

collinanderson commented Jan 16, 2018

@airstandley Nice catch! Fixed.

auvipy reviewed

View reviewed changes

Contributor

auvipy left a comment •

edited

Loading

nice!!

collinanderson force-pushed the ticket9475 branch 3 times, most recently from b674ee5 to d1ddf46 Compare

January 9, 2019 19:19

Contributor Author

collinanderson commented Jan 9, 2019

I just rebased this patch, updated version numbers, and added fail and success tests for add, create, set, get_or_create, and update_or_create. The failed mariadb test looks like a fluke.

Member

timgraham commented Jan 9, 2019

How about some helpful message if through_defaults is required but not specified (or specified insufficiently or incorrectly)?

Member

claudep commented Jan 9, 2019

Thanks Collin! This would be a nice addition for 2.2, hopefully we can commit it before feature freeze, even if we have to polish some aspects during the alpha phase.

Contributor Author

collinanderson commented Jan 9, 2019 •

edited

Loading

Hmm... I don't think it's possible to detect before the fact if through_defaults is needed, because the field could get auto filled in via a custom save() method, so the only way to know if it will work is to catch the error. But, it seems wrong to me to catch the IntegrityError and then raise a different error.

I'm hoping that letting the error message IntegrityError: NOT NULL constraint failed: myapp_mymodel.myrequiredfield bubble up is clear enough.

But maybe catching and raising a new error is helpful? Do you think something like the code below would help?

try:
    self.through._default_manager.using(db).bulk_create(**etc)
except IntegrityError:
    if rel.through._meta.auto_created:
        raise
    raise IntegrityError("Try specifying through_defaults or giving your field a default value")

Contributor Author

collinanderson commented Jan 9, 2019 •

edited

Loading

Ohh, I see now we're using bulk_create so values can't be filled in using a custom save() method...

Even still, I don't see a clean way of detecting the issue beforehand.

collinanderson mentioned this pull request

Fixed #12203 -- Allow ManyToManyField with through model in admin #10829

Closed

auvipy approved these changes

View reviewed changes

timgraham reviewed

View reviewed changes

tests/m2m_through_regress/tests.py Outdated

Member

timgraham Jan 14, 2019

Are these tests checking something that the m2m_through tests aren't? It looks odd that the results of these calls aren't verified.

Contributor Author

collinanderson Jan 15, 2019

Yeah, good point. I was mostly just converting "can't" test into "can" tests. Should I just delete them these?

Member

timgraham Jan 15, 2019

I didn't look closely but I don't think they add anything (but please audit test coverage as I said in my other comment).

Contributor Author

collinanderson Jan 15, 2019

Ok, I removed these tests.

timgraham changed the title ~~Fixed #9475 -- allow add(), create(), etc for m2m with through~~ Fixed #9475 -- Allowed RelatedManager.add(), create(), etc. for m2m with a through model.

timgraham force-pushed the ticket9475 branch from d1ddf46 to 85d3a30 Compare

January 15, 2019 01:08

timgraham reviewed

View reviewed changes

django/db/models/fields/related_descriptors.py Outdated

Member

timgraham Jan 15, 2019

Please check test coverage carefully. I commented out through_defaults here and don't see a test failure.

Contributor Author

collinanderson Jan 15, 2019 •

edited

Loading

Ok, turns out there's still a "fields.E332 Many-to-many fields with intermediate tables must not be symmetrical." check which i'll just leave in place. So, I removed the through_defaults on this code path.

django/db/models/fields/related_descriptors.py Outdated Show resolved Hide resolved

timgraham mentioned this pull request

Fixed #9475 -- Allowed add(), create(), etc for m2m with through #9609

Closed

collinanderson force-pushed the ticket9475 branch from 85d3a30 to 38e1e60 Compare

January 15, 2019 15:48

timgraham force-pushed the ticket9475 branch from 38e1e60 to f2f379f Compare

January 15, 2019 16:10


          Fixed django#9475 -- Allowed RelatedManager.add(), create(), etc. for…

769355c

… m2m with a through model.

timgraham force-pushed the ticket9475 branch from f2f379f to 769355c Compare

January 15, 2019 16:12

timgraham merged commit 769355c into django:master

ngnpope reviewed

View reviewed changes

Member

ngnpope left a comment

A few last minute observations...

django/db/models/fields/related_descriptors.py

+                              # If this is a symmetrical m2m relation to self, add the mirror
+                              # entry in the m2m table. `through_defaults` aren't used here
+                              # because of the system check error fields.E332: Many-to-many
+                              # fields with intermediate tables mus not be symmetrical.

Member

ngnpope Jan 15, 2019

mus → must.

django/db/models/fields/related_descriptors.py

                                   # Add the ones that aren't there already
                                   self.through._default_manager.using(db).bulk_create([
-                                      self.through(**{
+                                      self.through(**dict(through_defaults, **{

Member

ngnpope Jan 15, 2019

Can replace **dict(through_defaults, **{…}) and use unpacking generalisations: **{**through_defaults, **{…}}.

Member

charettes Jan 16, 2019 •

edited

Loading

I guess **through_defaults, **{…} would work as well given surfacing a TypeError if any of the ids properties are provided through through_defaults is probably a better idea than simply dropping them.

django/db/models/fields/related_descriptors.py

-                                  "intermediary model. Use %s.%s's Manager instead." %
-                                  (opts.app_label, opts.object_name)
-                              )
+                      def add(self, *objs, through_defaults=None):

Member

ngnpope Jan 15, 2019

I noticed that we use keyword-only arguments for def set(self, objs, *, clear=False, through_defaults=None):

Do we want to so something similar for other methods:

def add(self, *objs, *, through_defaults=None):
def create(self, *, through_defaults=None, **kwargs):
def get_or_create(self, *, through_defaults=None, **kwargs):
def update_or_create(self, *, through_defaults=None, **kwargs):

Member

timgraham Jan 15, 2019

Sure, I will push those change, except the suggestion for add() gives invalid syntax.

Member

ngnpope Jan 15, 2019

Oh. Strange. Not sure why that doesn't work. Thanks.

charettes reviewed

View reviewed changes

django/db/models/fields/related_descriptors.py

                                   # Add the ones that aren't there already
                                   self.through._default_manager.using(db).bulk_create([
-                                      self.through(**{
+                                      self.through(**dict(through_defaults, **{

Member

charettes Jan 16, 2019 •

edited

Loading

I guess **through_defaults, **{…} would work as well given surfacing a TypeError if any of the ids properties are provided through through_defaults is probably a better idea than simply dropping them.

django/db/models/fields/related_descriptors.py

-                                      self.through(**{
+                                      self.through(**dict(through_defaults, **{
                                           '%s_id' % source_field_name: self.related_val[0],
                                           '%s_id' % target_field_name: obj_id,

Member

charettes Jan 16, 2019

Also the %s_id is probably going to break for fields that define different db_columns. This wasn't an issue before because this branch could only be reached with auto-created intemediary models but now that we allow all forms of through we should use attname or directly *_name here.

Member

charettes Jan 16, 2019

My assumptions here seems to be wrong, this should work fine.

django/django/db/models/fields/related.py

Lines 916 to 917 in d212bc0

    
           def get_attname(self): 
        
               return '%s_id' % self.name

mihow mentioned this pull request

AttributeError when ParentalManyToManyField is specified with a through model wagtail/django-modelcluster#83

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet