Fix for Reverse Manager update_or_create calls

[jpulec]

This attr list appears to be missing update_or_create which means that reverse related managers throw an error when trying to use them like self.<MANAGER>.update_or_create().

[coveralls]

Pull Request Test Coverage Report for Build 1336

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 86.867%

---

Totals
Change from base Build 1335:	0.0%
Covered Lines:	721
Relevant Lines:	830

---

💛 - Coveralls

[atodorov]

Patch looks good but needs a test. func_noerror_model_methods.py looks like a good pace to me.

Unfortunately CI fails with what looks like an Astroid issue (could be several issues as well). The cron build yesterday passed so IDK what's causing it to break. We can't merge until this is solved though.

[jpulec]

I went to create a test for this, but I believe I discovered some deeper problems with the message suppression code. I noticed that when I try to add a update_or_create attribute access to func_noerror_model_methods.py, it doesn't appear to actually check attributes correctly. As far as I can tell, ANY attribute access off the default manager seems to not raise issues., i.e. SomeModel.objects.non_existing_method().

I tracked this down to be related to the call suppress_message(linter, TypeChecker.visit_attribute, 'no-member', is_model_attribute). It seems that this call somehow also suppresses no-member issues that should arise from SomeModel.objects.fake_method().

It also looks like the foreign_key_sets() augmentation is causing weird behavior as well.

Basically, if I comment out all the lines from __main__ in func_noerror_model_methods.py, except for SomeModel.objects.get_or_create() AND I disable the foreign_key_sets augmentation as well as the call to suppress_message(linter, TypeChecker.visit_attribute, 'no-member', is_model_attribute), I would expect that test to still pass, since there is a separate check to suppress manager attributes. However the test fails.

Am I way off base or missing something here? From what I can tell, there appear to be some deeper problems.

[atodorov]

I went to create a test for this, but I believe I discovered some deeper problems with the message suppression code. I noticed that when I try to add a update_or_create attribute access to func_noerror_model_methods.py, it doesn't appear to actually check attributes correctly. As far as I can tell, ANY attribute access off the default manager seems to not raise issues., i.e. SomeModel.objects.non_existing_method().

I tracked this down to be related to the call suppress_message(linter, TypeChecker.visit_attribute, 'no-member', is_model_attribute). It seems that this call somehow also suppresses no-member issues that should arise from SomeModel.objects.fake_method().

It also looks like the foreign_key_sets() augmentation is causing weird behavior as well.

Basically, if I comment out all the lines from __main__ in func_noerror_model_methods.py, except for SomeModel.objects.get_or_create() AND I disable the foreign_key_sets augmentation as well as the call to suppress_message(linter, TypeChecker.visit_attribute, 'no-member', is_model_attribute), I would expect that test to still pass, since there is a separate check to suppress manager attributes. However the test fails.

Am I way off base or missing something here? From what I can tell, there appear to be some deeper problems.

I still need the test code before I can answer any of this. It looks like astroid had been updated recently and it could be that our suppression machinery stopped working. Or it could be something completely different.

[jpulec]

I went ahead and added the test code, but in func_noerror_foreign_key_sets.py since this issue only appears for reverse related managers. My changes appear to solve this issue.

Separately, I noticed that this isn't the only missing method from the MANAGER_ATTRS. It looks like these sets of errors are handled by the foreign_key_sets() augmentation, which only checks MANAGER_ATTRS, unlike is_manager_attribute which takes the union of MANAGER_ATTRS and QS_ATTRS.

Is there a reason the union isn't also used in foreign_key_sets()?

[atodorov]

Can you rebase onto latest master so we can see the test results now? Thanks.

[atodorov]

To answer the question:

• is_manager_attribute() is a helper function used to decide whether or not to silence the no-member message from pylint. The comment above the line where it is used says supress errors when accessing magical class attributes - e.g. the entire block of suppressions and helper functions are used for anything automatically generated by Django.

• foreign_key_sets() is a function (which I frankly don't understand all of its details) used for augmentation. From augment_visit() doc-string:

    Augmenting a visit enables additional errors to be raised (although that case is
    better served using a new checker) or to suppress all warnings in certain circumstances.

    Augmenting functions should accept a 'chain' function, which runs the checker method
    and possibly any other augmentations, and secondly an Astroid node. "chain()" can be
    called at any point to trigger the continuation of other checks, or not at all to
    prevent any further checking.

My understanding is that when pylint is inspecting a piece of code like <something>_set.YYYY the augmentation kicks in and if YYYY is a Manager, Model or ForeignObject then we don't raise an error. Otherwise call the rest of the checker functions for this node.

Separately, I noticed that this isn't the only missing method from the MANAGER_ATTRS. It looks like these sets of errors are handled by the foreign_key_sets() augmentation, which only checks MANAGER_ATTRS, unlike is_manager_attribute which takes the union of MANAGER_ATTRS and QS_ATTRS.

Is there a reason the union isn't also used in foreign_key_sets()?

With the explanation above <something>_set is most likely a model manager, not a query set hence foreign_key_sets() uses only MANAGER_ATTRS.

Separately, I noticed that this isn't the only missing method from the MANAGER_ATTRS

Yap, probably there are many others missing. The historical reason for this is documented as a comment:

# Note: it would have been nice to import the Manager object from Django and
# get its attributes that way - and this used to be the method - but unfortunately
# there's no guarantee that Django is properly configured at that stage, and importing
# anything from the django.db package causes an ImproperlyConfigured exception.
# Therefore we'll fall back on a hard-coded list of attributes which won't be as accurate,
# but this is not 100% accurate anyway.

That said we already require that Django be properly configured so it is probably feasible to try and get all attributes automatically instead of listing them one by one. The reason it's not done by now is lack of time & attention (and lack of many bugs reported against this area). I will open an issue to take a look into this.