Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#32466 closed Bug (fixed)

Ticket #29138 breaks autocomplete for inherited models

Reported by: Dlis Owned by: Johannes Maron
Component: contrib.admin Version: 3.2
Severity: Release blocker Keywords: autocomplete
Cc: Johannes Maron Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Unfortunately, Closed ticket #29138 breaks autocomplete for the case of inherited models. For example, we have the following code:

# models.py
from django.db import models

class Tag(models.Model):
    pass

class Foo(models.Model):
    tags = models.ManyToManyField(Tag)

class Bar(Foo):
    pass


# admin.py
from django.contrib import admin
from . import models

@admin.register(models.Foo)
class Foo(admin.ModelAdmin):
    autocomplete_fields = ('tags',)

@admin.register(models.Bar)
class Bar(admin.ModelAdmin):
    autocomplete_fields = ('tags',)

Now, autocomplete for admin.Foo will work but not for admin.Bar because django.contrib.admin.widgets.AutocompleteMixin.optgroups() calculates a wrong value of a variable to_field_name, namely foo_ptr instead of id, whereupon following look up at self.choices.queryset.using(self.db).filter(**{'%s__in' % to_field_name: selected_choices}) raises an exception because models.Tag does not have foo_ptr.

Change History (20)

comment:1 by Simon Charette, 3 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Thanks for testing out the Django 3.2 beta.

comment:2 by Mariusz Felisiak, 3 years ago

comment:3 by Mariusz Felisiak, 3 years ago

Cc: Johannes Maron added

comment:4 by Johannes Maron, 3 years ago

Owner: changed from nobody to Johannes Maron
Status: newassigned

Thanks, good find. I am on it.

comment:5 by Johannes Maron, 3 years ago

Resolution: invalid
Status: assignedclosed

Hi there,

I tested the reported error first on my projects, where I use MTI and then on the example (which is incomplete). Both didn't yield the error or the to_field_value that was described. That is also unlikely, since the to_field_value is correctly based on the remote model. Therefore, I must conclude this issue is invalid. Feel free to reopen it, preferably with a valid example – I have been wrong in the past.

Should the problem just be in your projects setup, please feel free to reach out. I am happy to help.

Best,
Joe

comment:6 by Dlis, 3 years ago

Resolution: invalid
Status: closednew

Unfortunately, previous code was a little bit abstract. Here you can find a correct application – https://bitbucket.org/dlis/buggy/. Please, try open buggy.admin.Bar in admin. The application works perfectly on Django 3.1.x but raises the following error on Django 3.2.x:

"FieldError at /admin/buggy/bar/add/
Cannot resolve keyword 'foo_ptr' into field. Choices are: bar, id"

comment:7 by Johannes Maron, 3 years ago

Thanks for the code, that usually really helps. I will review it tomorrow, it's family time in my timezone ;)

comment:8 by Johannes Maron, 3 years ago

Triage Stage: AcceptedUnreviewed

comment:9 by Johannes Maron, 3 years ago

Triage Stage: UnreviewedAccepted

comment:10 by Johannes Maron, 3 years ago

@dlis, thanks again for reporting this issue. I could very if and just now created a patch. If you were so kind as to review the patch, that would really help. Once you are done, please consider moving triage to "ready for checkin" so a fellow can clear the release blocker.

comment:11 by Dlis, 3 years ago

Sure! I replaced Django==3.2b1 with git+git://github.com/codingjoe/django.git@711bbfde3e275a857a31cbb56dcf3e362ce7dc60 in my requirements.txt. Inherited models in admin open without raising the exception "FieldError" but, unfortunately, autocompleting stopped working. Log shows that requests to /admin/autocomplete/ raise the following:

development_1  | Forbidden (Permission denied): /admin/autocomplete/
development_1  | Traceback (most recent call last):
development_1  |   File "/usr/local/lib/python3.9/site-packages/django/core/handlers/exception.py", line 47, in inner
development_1  |     response = get_response(request)
development_1  |   File "/usr/local/lib/python3.9/site-packages/django/core/handlers/base.py", line 181, in _get_response
development_1  |     response = wrapped_callback(request, *callback_args, **callback_kwargs)
development_1  |   File "/usr/local/lib/python3.9/site-packages/django/contrib/admin/sites.py", line 250, in wrapper
development_1  |     return self.admin_view(view, cacheable)(*args, **kwargs)
development_1  |   File "/usr/local/lib/python3.9/site-packages/django/utils/decorators.py", line 130, in _wrapped_view
development_1  |     response = view_func(request, *args, **kwargs)
development_1  |   File "/usr/local/lib/python3.9/site-packages/django/views/decorators/cache.py", line 44, in _wrapped_view_func
development_1  |     response = view_func(request, *args, **kwargs)
development_1  |   File "/usr/local/lib/python3.9/site-packages/django/contrib/admin/sites.py", line 232, in inner
development_1  |     return view(request, *args, **kwargs)
development_1  |   File "/usr/local/lib/python3.9/site-packages/django/contrib/admin/sites.py", line 417, in autocomplete_view
development_1  |     return AutocompleteJsonView.as_view(admin_site=self)(request)
development_1  |   File "/usr/local/lib/python3.9/site-packages/django/views/generic/base.py", line 70, in view
development_1  |     return self.dispatch(request, *args, **kwargs)
development_1  |   File "/usr/local/lib/python3.9/site-packages/django/views/generic/base.py", line 98, in dispatch
development_1  |     return handler(request, *args, **kwargs)
development_1  |   File "/usr/local/lib/python3.9/site-packages/django/contrib/admin/views/autocomplete.py", line 20, in get
development_1  |     self.term, self.model_admin, self.source_field, to_field_name = self.process_request(request)
development_1  |   File "/usr/local/lib/python3.9/site-packages/django/contrib/admin/views/autocomplete.py", line 95, in process_request
development_1  |     raise PermissionDenied
development_1  | django.core.exceptions.PermissionDenied
development_1  | [22/Feb/2021 17:57:40] "GET /admin/autocomplete/?app_label=foo&model_name=bar&field_name=baz HTTP/1.1" 403 135

Probably, to_field_name = getattr(source_field.remote_field, 'field_name', 'pk') should be replaced with to_field_name = getattr(source_field.remote_field, 'field_name', 'id'). But it would be better to get the name of pk-field for the case of custom pk.

Last edited 3 years ago by Dlis (previous) (diff)

comment:12 by Dlis, 3 years ago

Yes, I've just checked that replacing to_field_name = getattr(source_field.remote_field, 'field_name', 'pk') with to_field_name = getattr(source_field.remote_field, 'field_name', 'id') solves the autocompleting, but it would be better to get the name of pk-field for the case of custom pk.

comment:13 by Johannes Maron, 3 years ago

Has patch: set
Needs tests: set
Patch needs improvement: set

Thanks for the quick review. In that case, I need to write more tests, to avoid regression and also improve the patch. I'll get on it tomorrow.

comment:14 by Johannes Maron, 3 years ago

Needs tests: unset
Patch needs improvement: unset
Summary: Closed ticket #29138 breaks autocomplete for inherited modelsTicket #29138 breaks autocomplete for inherited models

comment:15 by Johannes Maron, 3 years ago

Would you mind giving it another go? I added a test that should recreate your test scenario, but better safe than sorry.

comment:16 by Carlton Gibson, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:17 by Mariusz Felisiak, 3 years ago

Needs tests: set
Triage Stage: Ready for checkinAccepted

comment:18 by Carlton Gibson, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:19 by Carlton Gibson <carlton.gibson@…>, 3 years ago

Resolution: fixed
Status: newclosed

In 03d0f12c:

Fixed #32466 -- Corrected autocomplete to_field resolution for complex cases.

In MTI or ForeignKey as primary key cases, it is required to fetch the attname
from the field instance on the remote model in order to reliably resolve the
to_field_name.

Co-authored-by: Johannes Maron <info@…>
Co-authored-by: Mariusz Felisiak <felisiak.mariusz@…>
Co-authored-by: Carlton Gibson <carlton.gibson@…>

comment:20 by Carlton Gibson <carlton.gibson@…>, 3 years ago

In a8fef6da:

[3.2.x] Fixed #32466 -- Corrected autocomplete to_field resolution for complex cases.

In MTI or ForeignKey as primary key cases, it is required to fetch the attname
from the field instance on the remote model in order to reliably resolve the
to_field_name.

Backport of ceb4b9ee68dffc6ab0398886f1758f15f037c472 from main Backport of 03d0f12c823239812da21e5180aaa74dc6fd146e from main

Co-authored-by: Johannes Maron <info@…>
Co-authored-by: Mariusz Felisiak <felisiak.mariusz@…>
Co-authored-by: Carlton Gibson <carlton.gibson@…>

Note: See TracTickets for help on using tickets.
Back to Top