fix: MultipleChoiceField use ordered sort #9735
Conversation
auvipy
left a comment
auvipy
left a comment
There was a problem hiding this comment.
please also add unit tests to verify this and make the CI green
I’ve added the tests and the CI is green now. |
browniebroke
left a comment
browniebroke
left a comment
There was a problem hiding this comment.
Seems sensible to me 👍🏻
However, it will have to wait for 3.17 as I see this as potentially disruptive for some downstream users. Left a small suggestion
|
|
||
| assert serializer.is_valid() | ||
| assert serializer.validated_data['nested']['example'] == {1, 2} | ||
| assert serializer.validated_data['nested']['example'] == [1, 2] |
There was a problem hiding this comment.
Hum, that's a bit unfortunate that this data type is exposed at this level. I'm thinking of potential users testing at the serializer level might get some new test failures with this change.
There was a problem hiding this comment.
I think we just need to note in the new version's documentation that the return type is a list, so that if a user upgrades and their tests fail, they'll know exactly what to look at.
Since the input, as mentioned in the signature, is already an ordered sequence (a list or a tuple), it would be much more user-friendly if the output keeps that same order.
Good point — waiting for 3.17 sounds like the right call. Appreciate the suggestion! |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
Pull request overview
This PR modifies MultipleChoiceField to return ordered lists instead of sets, ensuring JSON serialization compatibility and maintaining the order of input values. The change uses dict.fromkeys() to deduplicate values while preserving insertion order.
Key Changes
- Changed
to_internal_value()andto_representation()methods to return lists usinglist(dict.fromkeys([...])) - Updated all test expectations from sets to lists
- Added JSON serialization tests to prevent regression
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rest_framework/fields.py | Modified MultipleChoiceField methods to return ordered lists instead of sets using dict.fromkeys() for deduplication |
| tests/test_fields.py | Updated test expectations from sets to lists, added order-preserving test case, and added JSON serialization validation tests |
| tests/test_serializer_nested.py | Updated nested serializer test assertions to expect lists instead of sets |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| valid_inputs = { | ||
| (): set(), | ||
| ('aircon',): {'aircon'}, | ||
| ('aircon', 'manual'): {'aircon', 'manual'}, | ||
| (): list(), | ||
| ('aircon',): ['aircon'], | ||
| ('aircon', 'manual'): ['aircon', 'manual'], | ||
| ('manual', 'aircon'): ['manual', 'aircon'], | ||
| } |
There was a problem hiding this comment.
Missing test case for duplicate values. The change to use dict.fromkeys() deduplicates values, but there's no test case to verify this behavior. Consider adding a test case like ('aircon', 'aircon'): ['aircon'] to ensure duplicates are properly handled and deduplicated while maintaining order.
| outputs = [ | ||
| (['aircon', 'manual', 'incorrect'], {'aircon', 'manual', 'incorrect'}) | ||
| (['aircon', 'manual', 'incorrect'], ['aircon', 'manual', 'incorrect']), | ||
| (['manual', 'aircon', 'incorrect'], ['manual', 'aircon', 'incorrect']), | ||
| ] |
There was a problem hiding this comment.
Missing test case for duplicate values in the outputs. The to_representation method now deduplicates values while maintaining order, but there's no test to verify this behavior. Consider adding a test case with duplicate values like (['aircon', 'manual', 'aircon'], ['aircon', 'manual']) to ensure the deduplication logic works correctly.
3 participants
This modification has the following advantages: