Opened 15 months ago

Last modified 6 weeks ago

#29138 assigned Bug

Add ModelAdmin.autocomplete_fields support for ForeignKeys that use to_field

Reported by: Jonathan Nye Owned by: Johannes Hoppe
Component: contrib.admin Version: 2.0
Severity: Normal Keywords: autocomplete
Cc: Johannes Hoppe, Tobi, Constantino Schillebeeckx Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Hi

I have encountered an issue where I have specified a 'to_field' in my foreign key and the autocomplete widget tries to enter the primary key of the related model instead. This means I cannot submit the form and I get the "Select a valid choice. That choice is not one of the available choices." error.

In the AutocompleteJsonView:

def get(self, request, *args, **kwargs):
        """
        Return a JsonResponse with search results of the form:
        {
            results: [{id: "123" text: "foo"}],
            pagination: {more: true}
        }
       ....
       ....
       return JsonResponse({
            'results': [
                {'id': str(obj.pk), 'text': str(obj)}
                for obj in context['object_list']
            ],
            'pagination': {'more': context['page_obj'].has_next()},
        })

Is there a way to replace the id manually when saving the form?

Thanks.

Change History (16)

comment:1 Changed 15 months ago by Tim Graham

Cc: Johannes Hoppe added
Summary: Admin autocomplete field doesn't support the 'to_field' on Foreign KeysAdd ModelAdmin.autocomplete_fields support for ForeignKeys that use to_field
Triage Stage: UnreviewedAccepted

I think the correct solution is to modify AutocompleteJsonView to return the to_field value instead of the primary key. To prevent improper data leakage, ModelAdmin.to_field_allowed() should be called.

comment:2 Changed 15 months ago by Johannes Hoppe

Hi Jonathan,

very good catch. This is certainly a bug. Custom to_fields should be supported.

Do you want to fix this yourself or should I?

Best
-Joe

comment:3 in reply to:  2 Changed 15 months ago by Jonathan Nye

Replying to Johannes Hoppe:

Hi Jonathan,

very good catch. This is certainly a bug. Custom to_fields should be supported.

Do you want to fix this yourself or should I?

Best
-Joe

Hi Joe,

I can give it a shot. I haven't contributed to Django before so I would appreciate some pointers to get me looking in the right direction.

What would need to be updated?

Regards,
Jonathan

comment:4 Changed 15 months ago by Johannes Hoppe

Hey Jonathan,

well that sounds like the perfect opportunity to get into contributing to Django.

Of course the is a contributing guide, but it long. I would recommend to read at least the section for new contributors.
https://docs.djangoproject.com/en/dev/internals/contributing/

Anyhow, this is how I would go about this:
Write your test first. You should write a test, that exploits the bug you found and fails. You will need to add one at the end anyways, if you start with it actually fixing the issue will become a lot easier. You might want to add the test here tests.admin_views.test_autocomplete_view.AutocompleteJsonViewTests.

Once that is out of the way there is this line in django.contrib.admin.views.autocomplete.AutocompleteJsonView#get. It returns a JsonResponse that includes the obj.pk at some point. What it should return is the value of the to_field. This is where things might become tricky. Since there is only once view per model and probably multiple other models having foreign keys to that one, you will need to get the to_field from the relation or pass it as a GET parameter. In the latter case you should definitely validate the field to avoid leakage as Tim said earlier.

Anyhow, just get started and let me know if you hit an obstacle and need help :)

Best
-Joe

comment:5 Changed 14 months ago by Basu Dubey

Owner: changed from nobody to Basu Dubey
Status: newassigned

I wish to take this up.

comment:6 Changed 14 months ago by Tobi

Cc: Tobi added

comment:7 Changed 14 months ago by Johannes Hoppe

Cool, let me know when you have patch. I am happy to review it :)

comment:8 Changed 8 months ago by Kirill Fries-Raevskiy

Is there any progress?

comment:9 Changed 8 months ago by Constantino Schillebeeckx

I've put together a bug fix for this.

Note I've been able to update all unit tests and create some new ones, however I haven't been able to leverage to_field_allowed to prevent data leaks. I've tried to implement it (see here), however I can't get it to play nicely with the unit tests. When uncommented, the id_field isn't properly being considered as to_field_allowed. I'm not familiar with this function, so could use some help troubleshooting.

comment:10 in reply to:  9 ; Changed 8 months ago by Johannes Hoppe

Owner: Basu Dubey deleted
Status: assignednew

Replying to Constantino Schillebeeckx:

I've put together a bug fix for this.

Note I've been able to update all unit tests and create some new ones, however I haven't been able to leverage to_field_allowed to prevent data leaks. I've tried to implement it (see here), however I can't get it to play nicely with the unit tests. When uncommented, the id_field isn't properly being considered as to_field_allowed. I'm not familiar with this function, so could use some help troubleshooting.

Hey there! Thanks for the work. It is a little hard to review since you only posted a branch and I don't know what commits to review. Could you maybe assign yourself and open up a pull-request to the Django repo? I will gladly do the first review.

Best
-Joe

comment:11 Changed 8 months ago by Constantino Schillebeeckx

Cc: Constantino Schillebeeckx added
Owner: set to Constantino Schillebeeckx
Status: newassigned

comment:12 in reply to:  10 Changed 7 months ago by Constantino Schillebeeckx

Replying to Johannes Hoppe:

Hey there! Thanks for the work. It is a little hard to review since you only posted a branch and I don't know what commits to review. Could you maybe assign yourself and open up a pull-request to the Django repo? I will gladly do the first review.

All set: https://github.com/django/django/pull/10517

Last edited 7 months ago by Constantino Schillebeeckx (previous) (diff)

comment:13 Changed 7 months ago by Simon Charette

Patch needs improvement: set

comment:14 Changed 3 months ago by Johannes Hoppe

Owner: changed from Constantino Schillebeeckx to Johannes Hoppe

This has been inactive and I stumbled upon a solution while fixing another issue, I am assigning this to me.

comment:15 Changed 2 months ago by Johannes Hoppe

Patch needs improvement: unset

comment:16 Changed 6 weeks ago by Florian Apolloner

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top