#29226 closed Bug (fixed)
@modify_settings append/remove/prepend is affected by dictionary order
Reported by: | Manuel Kaufmann | Owned by: | benjaoming |
---|---|---|---|
Component: | Testing framework | Version: | 2.0 |
Severity: | Normal | Keywords: | tests, modify_settings |
Cc: | humitos@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
I found a weird behaviour when using @modify_settings
with append
and remove
at the same time for the same elements. I used this because I needed to modify the order of the MIDDLEWARES classes: inject a middleware before one that it's already defined in that seeing. To do that, it's needed to remove all the middleware after that one (including it) and the re-add them by prepending your own.
Example:
# Original settings MIDDLEWARES = [ '1', '2', '3', '4', '5', ]
and I need this:
# Expected settings MIDDLEWARES = [ '1', '2', '3', 'MyOwnMiddlewareHere', '4', '5', ]
I would use @modify_settings
in this way:
@modify_settings(MIDDLEWARES={ 'append': [ 'MyOwnMiddlewareHere', '4', '5', ], 'remove': [ '4', '5', ], }) class FooBarTest(TestCase): ....
This cause different behaviours depending on the Python version we are using:
- Python < 3.6 this will randmly works since it could
- first remove and then append the items: this will produce the expected result
- first append and then remove the items: this won't produce the expected result
- Python >= 3.6 this will always produce the enexpected result because 3.6 has "insertion ordering" and append will be applied first all the times
- we can change this behaviour in 3.6 by putting the remove key first, and it will always produce the expected result.
NOTE: other Python implementation (not CPython) will also experiment this problem depending on their implementation.
I fixed this in my own code by using collections.OrderedDict
but I think this should be at least mentioned in the documentation to avoid other people having this issue. Otherwise, it could require an OrderedDict.
I wrote some tests that I'm attaching the patch with some comments and explanations on why this is happening. I'm running the tests like this: tox -e py35,py36 -- tests.settings_tests.tests
NOTE: this affects multiple versions of Django
Attachments (1)
Change History (12)
by , 7 years ago
Attachment: | test_modify_settings.patch added |
---|
comment:2 by , 7 years ago
I'm not sure append
/removed
were designed to run in the same modify_settings
and the same setting.
What about documenting to nest modify_settings
calls for this use case?
comment:3 by , 7 years ago
Type: | Uncategorized → Cleanup/optimization |
---|
If I understand correctly, the requirement to nest @modify_settings
isn't present if targeting Python 3.6+ because dicts are ordered there. I'm not sure what would give an expectation of some order of operations in older versions of Python. I'm not sure it's worth documenting anything about this.
comment:4 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Type: | Cleanup/optimization → Uncategorized |
Hi!
Sounds like a time-consuming issue for those who might encounter it. I agree with Tim that hopefully someone would think twice about this, but having dealt with issues related to dict key ordering myself, I have to admit that you get it wrong sometimes.
Also, the example in the docs (1) would directly lead to the issue, except that it does append/remove/prepend the same items, but it stills seems like it would easily misguide someone to do that
The solution that Markus and I just discussed was that you can A) use OrderedDict
or B) add modify_settings
twice like this:
# ... @modify_settings(MIDDLEWARES={ 'remove': [ '4', '5', ], }) @modify_settings(MIDDLEWARES={ 'append': [ 'MyOwnMiddlewareHere', '4', '5', ], })
Two suggestions:
- Updating the (misguiding) docs example (1) so it applies
modify_settings
several times without elaborating lots about why- - Add a warning box about doing append+reject on the same items (re-ordering)
As I understand Tim's comment, the latter would be a bit over the top, but the former could be preferable as it wouldn't complicate the docs much?
comment:5 by , 7 years ago
Another option could simply be to change Django's code to sort before iterating over the dict (perhaps only in older versions of Python). I use this practice in personal projects so that behavior will always be deterministic. Also, I personally don't think relying on dict's implicit ordering should be encouraged. It seems too tricky, and there are other patterns whose behavior is more obvious (e.g. the nesting approach).
comment:6 by , 7 years ago
I think users should know dict is not ordered before python3.6. I don't think we should docs this in django
comment:7 by , 7 years ago
Has patch: | set |
---|
@Windson yang
This issue itself is evidence that it's easy to get wrong, and that it will cost a lot of time to understand.. especially if the docs promote a pattern that doesn't work in Python <3.6.
There's a PR now with a note about it, and examples are updated.
comment:8 by , 7 years ago
Patch needs improvement: | set |
---|
comment:9 by , 6 years ago
Type: | Uncategorized → Bug |
---|
Tests to demostrate the inconsisten behaviour of @modify_settings