Prevent error comparing m2m field of the new objects (#1558)

[carlosal0ns0]

Problem

When importing a resource for a new instance with a many-to-many relation and non-filled fields have been evaluated for differences before the many-to-many field, an error occurs when attempting to retrieve the list of related entities from the original entity to get the differences. This happens because Django returns None when trying to retrieve a related manager for a model with a None primary key.

The specific error message is: AttributeError: 'NoneType' object has no attribute 'all'. in File /venv/lib/python3.9/site-packages/import_export/resources.py", line 630, in skip_row

For more information, you can refer to issue 1558
#1558

Solution

The solution is to modify the code to not try to retrieve the list of the many-to-many relation for objects with no primary key (new objects), as they will always have an empty list.

Acceptance Criteria

I have added a new test, core.tests.test_resources.ManyToManyWidgetDiffTest, to ensure that the code change works correctly.

[coveralls]

Coverage: 100.0%. Remained the same when pulling 2f45b12 on carlosal0ns0:fix/1558 into 4ac05ae on django-import-export:main.

[matthewhegarty]

Hi Carlos - this has failed because it has caused the test coverage to drop. Please could you take a look? Most likely the condition you have added is not being covered by the tests.

[carlosal0ns0]

I added extra tests to achieve 100% coverage on resources.py

[matthewhegarty]

Looks good - thanks for submitting.
I have some comments for minor improvements.

[matthewhegarty]

Looks good - a couple more changes and then I will merge.
Feel free to add your name to the AUTHORS file.

[carlosal0ns0]

@matthewhegarty thank you for your comments. One of the previous comments, regarding the class name you suggested earlier, left me thinking that perhaps the error scenario was not entirely clear, since the value of m2m does not necessarily have to be null to reproduce the error. Additionally, I realized that the coverage problems were due to having two classes with the same name, and some tests I added had already been tested. Having said that, I think it is clearer with the test

def test_many_to_many_widget_create_with_m2m_being_compared(self):
        # issue 1558 - when the object is a new instance and m2m is evaluated for differences
        dataset_headers = ["categories"]
        dataset_row = ["1"]
        dataset = tablib.Dataset(headers=dataset_headers)
        dataset.append(dataset_row)
        book_resource = BookResource()

        result = book_resource.import_data(dataset, dry_run=False)

        self.assertFalse(result.has_errors())
        self.assertEqual(len(result.rows), 1)

in the ManyToManyWidgetDiffTest class, and we can delete the ManyToManyNullFieldTest.

Additionally, note that this approach will result in testing the bug (and the fix) while still achieving 100% coverage. I believe it will be more clear, what are your thoughts?

[carlosal0ns0]

@matthewhegarty thank you for your comments. One of the previous comments, regarding the class name you suggested earlier, left me thinking that perhaps the error scenario was not entirely clear, since the value of m2m does not necessarily have to be null to reproduce the error. Additionally, I realized that the coverage problems were due to having two classes with the same name, and some tests I added had already been tested. Having said that, I think it is clearer with the test

def test_many_to_many_widget_create_with_m2m_being_compared(self):
        # issue 1558 - when the object is a new instance and m2m is evaluated for differences
        dataset_headers = ["categories"]
        dataset_row = ["1"]
        dataset = tablib.Dataset(headers=dataset_headers)
        dataset.append(dataset_row)
        book_resource = BookResource()

        result = book_resource.import_data(dataset, dry_run=False)

        self.assertFalse(result.has_errors())
        self.assertEqual(len(result.rows), 1)

in the ManyToManyWidgetDiffTest class, and we can delete the ManyToManyNullFieldTest.

Additionally, note that this approach will result in testing the bug (and the fix) while still achieving 100% coverage. I believe it will be more clear, what are your thoughts?

I have pushed this approach so that it is easier to review. However, if it is preferred, I can always revert to the old commits.

[matthewhegarty]

Hi - yes I think that is fine but your new test passes but doesn't test the bug (that code is skipped). You need to add:

book_resource._meta.skip_unchanged = True

Also you should prove that the result is a 'skip':

self.assertEqual(1, result.totals["skip"])

[carlosal0ns0]

you are right, i pushed new change including
book_resource._meta.skip_unchanged = True

however its a new book, so its not skipped, its a new row, so i added
self.assertEqual(result.rows[0].import_type, results.RowResult.IMPORT_TYPE_NEW)

[matthewhegarty]

Thanks for all your work on this!