Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#29138 closed Bug (fixed)

Add ModelAdmin.autocomplete_fields support for ForeignKeys that use to_field

Reported by: Jonathan Nye Owned by: Johannes Maron
Component: contrib.admin Version: 2.0
Severity: Normal Keywords: autocomplete
Cc: Johannes Maron, Tobi, Constantino Schillebeeckx, Tyler Schwartz, Carsten Fuchs Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
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 (33)

comment:1 by Tim Graham, 7 years ago

Cc: Johannes Maron 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 by Johannes Maron, 7 years ago

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

in reply to:  2 comment:3 by Jonathan Nye, 7 years ago

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 by Johannes Maron, 7 years ago

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 by Basu Dubey, 7 years ago

Owner: changed from nobody to Basu Dubey
Status: newassigned

I wish to take this up.

comment:6 by Tobi, 7 years ago

Cc: Tobi added

comment:7 by Johannes Maron, 7 years ago

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

comment:8 by Kirill Fries-Raevskiy, 6 years ago

Is there any progress?

comment:9 by Constantino Schillebeeckx, 6 years ago

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.

in reply to:  9 ; comment:10 by Johannes Maron, 6 years ago

Owner: Basu Dubey removed
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 by Constantino Schillebeeckx, 6 years ago

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

in reply to:  10 comment:12 by Constantino Schillebeeckx, 6 years ago

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 6 years ago by Constantino Schillebeeckx (previous) (diff)

comment:13 by Simon Charette, 6 years ago

Patch needs improvement: set

comment:14 by Johannes Maron, 6 years ago

Owner: changed from Constantino Schillebeeckx to Johannes Maron

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

comment:15 by Johannes Maron, 6 years ago

Patch needs improvement: unset

comment:16 by Florian Apolloner, 6 years ago

Patch needs improvement: set

comment:17 by Tyler Schwartz, 5 years ago

Cc: Tyler Schwartz added
Has patch: set
Owner: changed from Johannes Maron to Tyler Schwartz
Patch needs improvement: unset

This work went stale, so I re-forked, upmerged from master, and resolved the merge conflict and isort/flake8 issues. This new PR should be ready for review/acceptance.

New PR

comment:18 by Johannes Maron, 5 years ago

Patch needs improvement: set

Hi schwtyl,

thanks for your commitment. I must admit, I did push this issue aside a bit in favor of some other patches I have been working on.

Sadly, you did base your work on GH@10494 not on GH@11026, therefore my critique stays the same. It will only solve this issue but make it harder to solve #29138 which is equally important.

Might I also suggest next time, to comment on the ticket first, before fixing someone else's patch. I might save you some trouble.

Best
-Joe

comment:19 by Tyler Schwartz, 5 years ago

Joe,

Constantino developed this patch for the project that I succeeded him on, so I'm hardly a random bystander. Everytime our project builds for the past year, it pulls it from this GitHub commit, and I'd love to see that eventually go back to using an official Django release again. I'm not sure how I'd know about PR 11026, since you didn't reference it here at all. This issue *is* #29138 - perhaps you mean #29010? The patch fixes this issue - can you provide feedback as to what can be done for a fix to be accepted? What specifically *needs improvement*?

Thanks,

Tyler

Last edited 5 years ago by Tyler Schwartz (previous) (diff)

comment:20 by Johannes Maron, 5 years ago

Hey Tyler,

thanks for your swift response. I see, I wasn't ware of your background, but also didn't mean to offend you. I my comment really was only trying to save you some effort. I know first hand that the Django development process can be lengthy and demotivating.

Anyhow, I see where you are coming from, and yes, you shouldn't absolutely use a mainstream Django version, to avoid security vulnerabilities.
Might I suggest to use django-select2 until then? I developed and maintain that package, it was the blue print for Django's autocomplete field implementation, which I implemented too. It does work slightly different, but it will at least allow you to jump to a stable Django release.

Regarding the patch: As the initial developer of this feature I have an elevated interest to solve all bugs, not just this one. That aside – IMHO – the best solution actually happens to fix both problems.

I will do a review of your PR, to give you a bit better feeling of the work that would need to be done on the PR to fix only this issue. I'll let you decide, if you still think it's worth the effort given that all changes would need to be revered with the fix for #29138.

Best
-Joe

comment:21 by Johannes Maron, 5 years ago

Scratch everything I said. I had another look at #29138. It will require a lot of work and probably some mailing list discussion. Therefore, I do believe it's worth the effort to fix them separately even if it involves revering some parts later.

I already did a first review of your patch. I'll give it another go tomorrow. I believe we can push this over the finish line.

comment:22 by Tyler Schwartz, 5 years ago

Thanks Joe!

I appreciate the background you were able to fill in, and I wasn't aware that you were so involved in the implementation of Django Admin's autocomplete feature. I am new to Django development, though I am interested in helping take this forward if I can be of help, even if it takes a while. I don't have any emotional attachment to this current patch, other than having used it in production for this past year (yikes!) - so if you think a different direction is preferable, I won't be offended. I'll take a look at your feedback shortly.

Regardless, you make a good point - I am working to transition our code back to a stable Django release (I inherited this setup when I started earlier this year).

-Tyler

comment:23 by Manel Clos, 5 years ago

We have been using this in Django 2.1.5. As the to_field contains a code, it is a valid integer, and no error is produced. This issue should be marked as DATA LOSS and be urgently addressed.

comment:24 by Carsten Fuchs, 4 years ago

Cc: Carsten Fuchs added

comment:25 by Johannes Maron, 4 years ago

Owner: changed from Tyler Schwartz to Johannes Maron
Patch needs improvement: unset

Since we have a working patch, that's been reviewed a bunch, I'm snatching this ticket.

comment:26 by Carlton Gibson, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:27 by GitHub <noreply@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 3071660a:

Fixed #29010, Fixed #29138 -- Added limit_choices_to and to_field support to autocomplete fields.

  • Fixed #29010 -- Added limit_choices_to support to autocomplete fields.
  • Fixed #29138 -- Allowed autocomplete fields to target a custom to_field rather than the PK.

comment:28 by Carlton Gibson <carlton.gibson@…>, 4 years ago

In 0b120f5:

Refs #29138 -- Added extra tests for autocomplete FK to_field variations.

comment:29 by 007, 4 years ago

AutocompleteMixin is incompatible in django 3.1 and 3.2. and there is no tip like RemovedInDjango32Warning in django 3.1. It's not user friendly.

comment:30 by Johannes Maron, 4 years ago

Hi 007,

Thank you for reaching out. I understand your frustration. However, the AutocompleteMixin was never documented, which means it is not considered a public API. Private APIs are not subject to deprecation. That is the risk you take, when using a private API.

All that being said, is there anything I could be of help with? If you have a 3rd party package, that needs assistance, I would be delighted to help.

Best,
Joe

in reply to:  30 comment:31 by 007, 4 years ago

Replying to Johannes Maron:

Hi 007,

Thank you for reaching out. I understand your frustration. However, the AutocompleteMixin was never documented, which means it is not considered a public API. Private APIs are not subject to deprecation. That is the risk you take, when using a private API.

All that being said, is there anything I could be of help with? If you have a 3rd party package, that needs assistance, I would be delighted to help.

Best,
Joe

In my package to achieve a similar function of this submission. I'm going to try to re implement it in the way of 3.2, and compatible with earlier versions

https://github.com/007gzs/django-cool/blob/f13d52b571eb77c02b062523bc1d79cdeed31b5a/cool/admin/widgets.py#L184-L234

https://github.com/007gzs/django-cool/blob/f13d52b571eb77c02b062523bc1d79cdeed31b5a/cool/admin/views.py#L14-L76

comment:33 by Johannes Maron, 4 years ago

I am happy you found a solution. And this should be the last big change on this feature for a while.

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