handle crash on start-up when change_list_template is a property

[matthewhegarty]

Problem

Fix for issue #1521. The server crashes when attempting to assign change_list_template if this has been decorated as a property in another library. This is a workaround for issues which can arise when integrating import-export along with other libraries.

Solution

The AttributeError is caught and an exception is logged. This doesn't crash the process and will give the user the opportunity to investigate and resolve. I investigated testing if the attribute is implemented as a property, but this did not work. It never evaluated the attribute as a property, only as a list.

Acceptance Criteria

• test included

[coveralls]

Coverage remained the same at 100.0% when pulling 566196f on matthewhegarty:issue-1521 into 9058392 on django-import-export:main.

[pokken-magic]

Nice!

[ShaheedHaque]

I hand-applied the patch and, as expected, my runserver does not bail out, but my log file contains entries like this for each affected model:

2022-12-08 10:01:20,758 [ERROR] import_export.admin: failed to assign change_list_template attribute
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/dist-packages/import_export/admin.py", line 52, in __init__
    self.change_list_template = getattr(

Here is the thing: prior to release 3, the Admin support for exporting this model worked just fine, producing files like this:

Now, I get the exception, and these buttons don't show up either:

From my point of view (i.e. not as the developer of this code :-)), ideally:

• It would work like before.
• There would be no error (or worse, exception traceback) message cluttering my logs.

So should failing to set this attribute really be fatal? Can the old behaviour not work?

[matthewhegarty]

Thanks for testing. All very good points. I'll take a look.

[matthewhegarty]

hi @ShaheedHaque I had another look, and have come up with the following

• The reason it doesn't work as it did in 2.x is because of the changes made in Allow custom change_list_template in admin views using mixins #1483, this was implemented so that import-export plays nicely with other plugins which define change list templates (ironically it has broken in this case).
• The issue we have is that change_list_template is a defined as a property in the polymorphic library, hence we cannot change that value and if we try to do so we get the error, so all we can do is catch that and continue.
• I have modified the log level from exception to error - I think this is valid - it is an unexpected error outside of our control, so we should log it. If you don't want this in your logs you can configure the logging level for import-export in your application (i.e. set it to 'warning' level).
• I added a init_change_list_template() method for this functionality, so users can override as they see fit, if need be.
• To restore the 2.x changelist template, you can declare in your mixin:

class CompanyPostalDetailAdmin(ImportExportMixin, poly_parentadmin.PolymorphicParentModelAdmin):
    change_list_template = 'admin/import_export/change_list_export.html'

I think this is a fair compromise between handling errors generated from integration alongside other libraries, and allowing flexibility for customisation.

Please could you try the latest changes and let me know what you think.

[ShaheedHaque]

Thanks again for you prompt assistance.

I tried your suggestion to override the attribute by subclassing your mixin like this (I tried both versions as per the commented out line):

class ImportExportMixin(i_e_admin.ImportExportMixin):
    # change_list_template = 'admin/import_export/change_list_export.html'
    change_list_template = 'admin/import_export/change_list_import_export.html'

but Django crapped out saying TemplateDoesNotExist and producing this postmortem:

Template-loader postmortem
Django tried loading these templates, in this order:

Using engine django:

django.template.loaders.filesystem.Loader: /main/srhaque/bootstrap/source/templates/admin/import_export/change_list_import_export.html (Source does not exist)
django.template.loaders.app_directories.Loader: /usr/local/lib/python3.10/dist-packages/django/contrib/admin/templates/admin/import_export/change_list_import_export.html (Source does not exist)
django.template.loaders.app_directories.Loader: /usr/local/lib/python3.10/dist-packages/django/contrib/auth/templates/admin/import_export/change_list_import_export.html (Source does not exist)
django.template.loaders.app_directories.Loader: /usr/local/lib/python3.10/dist-packages/django/contrib/postgres/templates/admin/import_export/change_list_import_export.html (Source does not exist)
django.template.loaders.app_directories.Loader: /usr/local/lib/python3.10/dist-packages/bootstrapform_jinja/templates/admin/import_export/change_list_import_export.html (Source does not exist)
vvv recursion error here vvv
django.template.loaders.app_directories.Loader: /usr/local/lib/python3.10/dist-packages/import_export/templates/admin/import_export/change_list_import_export.html (Skipped to avoid recursion)
django.template.loaders.app_directories.Loader: /usr/local/lib/python3.10/dist-packages/polymorphic/templates/admin/import_export/change_list_import_export.html (Source does not exist)
django.template.loaders.app_directories.Loader: /usr/local/lib/python3.10/dist-packages/formtools/templates/admin/import_export/change_list_import_export.html (Source does not exist)
django.template.loaders.app_directories.Loader: /usr/local/lib/python3.10/dist-packages/rest_framework/templates/admin/import_export/change_list_import_export.html (Source does not exist)
django.template.loaders.app_directories.Loader: /usr/local/lib/python3.10/dist-packages/oauth2_provider/templates/admin/import_export/change_list_import_export.html (Source does not exist)
django.template.loaders.app_directories.Loader: /usr/local/lib/python3.10/dist-packages/drf_spectacular/templates/admin/import_export/change_list_import_export.html (Source does not exist)

Surprised? I was! I feel I am missing something obvious...but I just cannot see it.

[matthewhegarty]

Sounds similar to #1514. I have been able to reproduce this now. admin/import_export/change_list.html extends base_change_list_template, so if this is somehow assigned to admin/import_export/change_list.html (or any template which extends it) you'll see the issue.

To resolve, try this:

class CompanyPostalDetailAdmin(ImportExportMixin, poly_parentadmin.PolymorphicParentModelAdmin):
    import_export_change_list_template = "admin/import_export/change_list_export.html"

[ShaheedHaque]

OK, based on your last observation, I first tried your suggestion ~verbatim, and of course that gave me the original exception because the @property-decorated attribute was still in play. I then thought a bit harder, and realised that if I did this:

from import_export import admin as i_e_admin
.
.
.
class ImportExportMixin(i_e_admin.ImportExportMixin):
    # Override @property-decorated attribute on PolymorphicParentModelAdmin.
    change_list_template = None
.
.
.
class CompanyPostalDetailAdmin(ImportExportMixin, PolymorphicParentModelAdmin):
    ...

then, hey-presto, the @property-decorated attribute is no longer in play (and of course, there is actually no need to muck about with import_export_change_list_template at all). Even better, it actually works with the currently shipping django-import-export-3.0.1...that is, without this patch.

So, I guess it is your call whether to bother with this patch or not...I'm happy for the issue to be closed as you see fit.

Either way, many many thanks for patiently walk me to this point.

[matthewhegarty]

that gave me the original exception because the @property-decorated attribute was still in play

but presumably would be ok now because we are catching the exception and logging. I guess you want to avoid the log statement altogether.

So, I guess it is your call whether to bother with this patch or not

I think it will be useful to add it in - it will be much better for anyone in future not to have the app crash on start up.

I've added the method init_change_list_template() so there is always the option for users to override this and handle the change_list assignment anyway they want.

Either way, many many thanks for patiently walk me to this point.

No problem - it's been useful for me to understand this issue in more detail, so thanks for providing the updates.