Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#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)

test_modify_settings.patch (2.6 KB ) - added by Manuel Kaufmann 7 years ago.
Tests to demostrate the inconsisten behaviour of @modify_settings

Download all attachments as: .zip

Change History (12)

by Manuel Kaufmann, 7 years ago

Attachment: test_modify_settings.patch added

Tests to demostrate the inconsisten behaviour of @modify_settings

comment:1 by Markus Holtermann, 7 years ago

Triage Stage: UnreviewedAccepted

Sounds like a legitimate problem.

comment:2 by Claude Paroz, 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 Tim Graham, 7 years ago

Type: UncategorizedCleanup/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 benjaoming, 7 years ago

Owner: changed from nobody to benjaoming
Status: newassigned
Type: Cleanup/optimizationUncategorized

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:

  1. Updating the (misguiding) docs example (1) so it applies modify_settings several times without elaborating lots about why-
  2. 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?

(1) https://docs.djangoproject.com/en/2.0/topics/testing/tools/#django.test.SimpleTestCase.modify_settings

Last edited 7 years ago by benjaoming (previous) (diff)

comment:5 by Chris Jerdonek, 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).

Last edited 7 years ago by Chris Jerdonek (previous) (diff)

comment:6 by Windson yang, 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 benjaoming, 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 Carlton Gibson, 7 years ago

Patch needs improvement: set

comment:9 by Tim Graham, 6 years ago

Type: UncategorizedBug

comment:10 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: assignedclosed

In 08f788b1:

Fixed #29226 -- Doc'd modify_settings() ordering considerations for Python < 3.6.

comment:11 by Tim Graham <timograham@…>, 6 years ago

In fb9c1f4:

[2.1.x] Fixed #29226 -- Doc'd modify_settings() ordering considerations for Python < 3.6.

Backport of 08f788b169a30d26f50433083aca253a4e4031b2 from master

Note: See TracTickets for help on using tickets.
Back to Top