Fix for 974

[PetrDlouhy]

Problem

#974

Solution

This is concept of a solution of the problem with unnecessary queries for IDs.

I would like to get some feedback on this. I am not sure about which other cases could this affect and whether we are able to fix more cases.

The current solution is mostly bypassing the logic of Widget and leaking it to the Fields. But I am not sure how to make this correctly, because the data in Widgets are not sufficient for this and also when the Widget.render() is called the demage is already done and the query is called.

Acceptance Criteria

It includes a clear test case scenario documenting the problem.

[matthewhegarty]

Hi @PetrDlouhy it might be worth you making this change on the release-4 branch (v4 feature branch). There are a lot of changes coming in v4 so this type of fix would be better suited to that branch IMO.

[PetrDlouhy]

Now I rewrote this to a cleaner solution - instead of changing Widget and Fields logic I tried to change the way the default fields are derived from Model.
I find this solution to be much cleaner and I think I am satisfied with this approach although there is still some tuning needed.

@matthewhegarty Thanks, I rebased against v4.

[coveralls]

coverage: 100.0%. remained the same
when pulling 7f9d31f on PetrDlouhy:fix_974
into b63ddd6 on django-import-export:main.

[PetrDlouhy]

@matthewhegarty

I have got the following error with the newly written test even on pure release-4 branch. Does that mean that there is something not fully resolved there?

======================================================================
ERROR: test_export_with_foreign_keys (core.tests.test_resources.test_resources.ModelResourceTest)
Test that export() containing foreign keys doesn't generate
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/django-import-export/django-import-export/tests/core/tests/test_resources/test_resources.py", line 305, in test_export_with_foreign_keys
    dataset = self.resource.export(Book.objects.prefetch_related("categories"))
  File "/home/runner/work/django-import-export/django-import-export/./tests/../import_export/resources.py", line 1043, in export
    dataset.append(self.export_resource(obj))
  File "/home/runner/work/django-import-export/django-import-export/./tests/../import_export/resources.py", line 992, in export_resource
    return [
  File "/home/runner/work/django-import-export/django-import-export/./tests/../import_export/resources.py", line 993, in <listcomp>
    self.export_field(field, instance) for field in self.get_export_fields()
  File "/home/runner/work/django-import-export/django-import-export/./tests/../import_export/resources.py", line 986, in export_field
    return field.export(instance)
  File "/home/runner/work/django-import-export/django-import-export/./tests/../import_export/fields.py", line 149, in export
    return self.widget.render(value, instance)
  File "/home/runner/work/django-import-export/django-import-export/./tests/../import_export/widgets.py", line 88, in render
    self._obj_deprecation_warning(obj)
  File "/home/runner/work/django-import-export/django-import-export/./tests/../import_export/widgets.py", line 68, in _obj_deprecation_warning
    warn(
DeprecationWarning: The 'obj' parameter is deprecated and will be removed in a future release

[matthewhegarty]

Yes, in v4 you have to add the decorator to your test:

@ignore_widget_deprecation_warning

[matthewhegarty]

Also make me think that this fix could help with #1209

[matthewhegarty]

@PetrDlouhy I tested imports / exports with this fix in place and it seems fine.

One thing I noticed is that we can remove the additional queries using select_related(). This is mentioned in the issue, and most Django users will be familiar with this pattern. Therefore I'm wondering whether I could update the documentation to help users solve this issue. However, your fix would work for all users without them having to add their own code, so should make exports faster for all.

I added some comments - see what you think.

[PetrDlouhy]

@matthewhegarty The select_related() makes the query slower (usually not too much) and in the case of getting object IDs it is not necessary. I would be a bit careful about telling users to add select_related() as temporary fix, because they will forget about it after proper fix.

Of course it is good idea to add select_related() in all other cases of getting fields from foreign key. The IDs are the only case where it is not necessary and also unexpected.

[matthewhegarty]

Thanks for clarifying.
I am happy to merge this. Would you mind updating the changelog?

[PetrDlouhy]

I have added new entry to changelog.

[matthewhegarty]

Thanks! 💯

[19greg96]

Correct me if I am wrong, please, but to me, it feels like this belongs more in widget_kwargs_for_field than here.