[Django] #29226: @modify_settings append/remove/prepend is affected by dictionary order
Django
-------------------------------------+-------------------------------------
Reporter: Manuel | Owner: nobody
Kaufmann |
Type: | Status: new
Uncategorized |
Component: Testing | Version: 2.0
framework | Keywords: tests,
Severity: Normal | modify_settings
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/29226>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: tests, | Triage Stage:
modify_settings | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "test_modify_settings.patch" added.
Tests to demostrate the inconsisten behaviour of @modify_settings
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
modify_settings |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Sounds like a legitimate problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/29226#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: tests, | Triage Stage: Accepted
modify_settings |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/29226#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: tests, | Triage Stage: Accepted
modify_settings |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Uncategorized => Cleanup/optimization
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29226#comment:3>
Django
-------------------------------------+-------------------------------------
| benjaoming
Type: Uncategorized | Status: assigned
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: tests, | Triage Stage: Accepted
modify_settings |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => benjaoming
* status: new => assigned
* type: Cleanup/optimization => Uncategorized
Comment:
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 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:
https://docs.djangoproject.com/en/2.0/topics/testing/tools/#django.test.SimpleTestCase.modify_settings
The solution that Markus and I just discussed was that you can A) use
`OrderedDict` or B) add `modify_settings` twice like this:
{{{#!python
# ...
@modify_settings(MIDDLEWARES={
'remove': [
'4',
'5',
],
})
@modify_settings(MIDDLEWARES={
'append': [
'MyOwnMiddlewareHere',
'4',
'5',
],
})
}}}
Two suggestions:
1. Updating the (misguiding) example that does the above without
elaborating lots about why it's applied several times:
https://docs.djangoproject.com/en/2.0/topics/testing/tools/#django.test.SimpleTestCase.modify_settings
1. 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/29226#comment:4>
Django
-------------------------------------+-------------------------------------
| benjaoming
Type: Uncategorized | Status: assigned
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: tests, | Triage Stage: Accepted
modify_settings |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
Another option could simply be to sort anytime iterating over a dict. 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/29226#comment:5>
Django
-------------------------------------+-------------------------------------
| benjaoming
Type: Uncategorized | Status: assigned
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: tests, | Triage Stage: Accepted
modify_settings |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Windson yang):
I think users should know dict is not ordered before
[https://docs.python.org/3.6/whatsnew/3.6.html#new-dict-implementation/
python3.6]. I don't think we should docs this in django
--
Ticket URL: <https://code.djangoproject.com/ticket/29226#comment:6>
Django
-------------------------------------+-------------------------------------
| benjaoming
Type: Uncategorized | Status: assigned
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: tests, | Triage Stage: Accepted
modify_settings |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29226#comment:7>
Django
-------------------------------------+-------------------------------------
| benjaoming
Type: Uncategorized | Status: assigned
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: tests, | Triage Stage: Accepted
modify_settings |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29226#comment:8>
Django
-------------------------------------+-------------------------------------
| benjaoming
Type: Bug | Status: assigned
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution:
Keywords: tests, | Triage Stage: Accepted
modify_settings |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Uncategorized => Bug
--
Ticket URL: <https://code.djangoproject.com/ticket/29226#comment:9>
Django
-------------------------------------+-------------------------------------
| benjaoming
Component: Testing framework | Version: 2.0
Keywords: tests, | Triage Stage: Accepted
modify_settings |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"08f788b169a30d26f50433083aca253a4e4031b2" 08f788b1]:
{{{
#!CommitTicketReference repository=""
revision="08f788b169a30d26f50433083aca253a4e4031b2"
Fixed #29226 -- Doc'd modify_settings() ordering considerations for Python
< 3.6.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29226#comment:10>
Django
-------------------------------------+-------------------------------------
| benjaoming
Type: Bug | Status: closed
Component: Testing framework | Version: 2.0
Severity: Normal | Resolution: fixed
Keywords: tests, | Triage Stage: Accepted
modify_settings |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"fb9c1f4b46623208ed8dd587a020ae5fc1b2a87e" fb9c1f4]:
{{{
#!CommitTicketReference repository=""
revision="fb9c1f4b46623208ed8dd587a020ae5fc1b2a87e"
[2.1.x] Fixed #29226 -- Doc'd modify_settings() ordering considerations
for Python < 3.6.
Backport of 08f788b169a30d26f50433083aca253a4e4031b2 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29226#comment:11>