Skip to content

Fix mutable default arguments in OrderingFilter methods #9742

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

killerdevildog
Copy link

@killerdevildog killerdevildog commented Jul 18, 2025

Note: Before submitting a code change, please review our contributing guidelines.

Description

  • Fixed get_default_valid_fields() and get_valid_fields() methods in filters.py
  • Changed context={} default parameter to context=None to prevent mutable default anti-pattern
  • Added proper None checking with context = {} assignment inside methods

Why this fix is important:

  • Mutable default arguments (context={}) create shared state across function calls
  • Same dict object gets reused, potentially causing unexpected side effects
  • This is a well-known Python anti-pattern that can lead to bugs

What was changed:

  • Line 249: get_default_valid_fields(self, queryset, view, context=None)
  • Line 285: get_valid_fields(self, queryset, view, context=None)
  • Added 'if context is None: context = {}' in both methods

Testing results:

  • All existing filter tests pass (pytest tests/test_filters.py)
  • Custom verification script confirms fix works correctly
  • Maintains backward compatibility
  • No breaking changes to API

Fix #9741

- Fixed get_default_valid_fields() and get_valid_fields() methods in filters.py
- Changed context={} default parameter to context=None to prevent mutable default anti-pattern
- Added proper None checking with context = {} assignment inside methods

Why this fix is important:
- Mutable default arguments (context={}) create shared state across function calls
- Same dict object gets reused, potentially causing unexpected side effects
- This is a well-known Python anti-pattern that can lead to bugs

What was changed:
- Line 249: get_default_valid_fields(self, queryset, view, context=None)
- Line 285: get_valid_fields(self, queryset, view, context=None)
- Added 'if context is None: context = {}' in both methods

Testing results:
- All existing filter tests pass (pytest tests/test_filters.py)
- Custom verification script confirms fix works correctly
- Maintains backward compatibility
- No breaking changes to API

Addresses GitHub issue encode#9741
@browniebroke browniebroke added this to the 3.16 milestone Aug 9, 2025
Copy link
Member

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

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

Due to how filter are used, I don't think that's ever a problem in practice (we seem to always pass the context), but shouldn't hurt to fix anyway. The only thing that could break is if None is valid value for context, and the user explicitly passes None. In which case, this would override it with an empty dict {}. Can't think of a case where that would be a problem though...

Fix looks good to me, but would be nice to add a test or 2 to make sure this doesn't regress.

Thanks

@killerdevildog
Copy link
Author

@browniebroke I'll look into this this week.

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

Successfully merging this pull request may close these issues.

Do not use mutable data structures for argument defaults
3 participants