Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing test for ChoiceField eagerly evaluating choices #1648

Merged

Conversation

craigds
Copy link
Contributor

@craigds craigds commented Mar 12, 2024

refs #1625

@craigds
Copy link
Contributor Author

craigds commented Mar 12, 2024

@carltongibson that should help^

In case it helps, that test passes when backported on top of 23.2 and fails with 23.3

@carltongibson
Copy link
Owner

Great. Thanks. Let me have a look. 👀

Copy link
Owner

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @craigds.

Since the callable is wrapped by the iterator, it's being cast to a list in super class (forms.ChoiceField):

  File ".../django-filter/django_filters/fields.py", line 284, in choices
    super(ChoiceIteratorMixin, self.__class__).choices.__set__(self, value)
  File ".../django/forms/fields.py", line 880, in _set_choices
    value = list(value)

(Django stable/4.2.x)

In order avoid that I think we need to bypass the superclass setter for Django<5.0.

Something like:

diff --git a/django_filters/fields.py b/django_filters/fields.py
index 3a34365..d6ac79c 100644
--- a/django_filters/fields.py
+++ b/django_filters/fields.py
@@ -275,13 +275,13 @@ class ChoiceIteratorMixin:
     def choices(self, value):
         if DJANGO_50:
             value = self.iterator(self, value)
+            # Simple `super()` syntax for calling a parent property setter is
+            # unsupported. See https://github.com/python/cpython/issues/59170
+            super(ChoiceIteratorMixin, self.__class__).choices.__set__(self, value)
         else:
             super()._set_choices(value)
             value = self.iterator(self, self._choices)
-
-        # Simple `super()` syntax for calling a parent property setter is
-        # unsupported. See https://github.com/python/cpython/issues/59170
-        super(ChoiceIteratorMixin, self.__class__).choices.__set__(self, value)
+            self._choices = self.widget.choices = value

Fancy trying that?

cc @ngnpope

@ngnpope
Copy link
Contributor

ngnpope commented Mar 13, 2024

Thanks for the ping @carltongibson.

So, yes, it looks as though I was a little too optimistic there - your proposed patch looks like the right approach.

Copy link

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  django_filters
  fields.py
Project Total  

This report was generated by python-coverage-comment-action

@carltongibson carltongibson merged commit 0311653 into carltongibson:main Mar 27, 2024
9 checks passed
@craigds
Copy link
Contributor Author

craigds commented Mar 28, 2024

thanks, sorry I didn't get around to submitting a proper fix

@craigds craigds deleted the choicefield-evaluates-choices-eagerly branch March 28, 2024 02:44
@carltongibson
Copy link
Owner

No problem @craigds. The reproduce was the key thing. 🎁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants