Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#21740 closed Cleanup/optimization (fixed)

client.py uses mutable default arguments, which is bad practice

Reported by: sleepydragon Owned by: anonymous
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

In django/test/client.py there are several methods that have data={} in their argument lists. The {} dictionary is a mutable type and it is not recommended to use mutable objects as default arguments. This is because each invocation of the method where data is not specified will use the same dict object. This enables completely unrelated method calls to affect each other.

The fix that I recommend is to instead use data=None in the argument list for the affected methods and then in the method body check for None and create a new dict if it is None. That way each method invocation gets their own, local dict object. I have attached a patch for review.

I'm fairly new to Django so it's possible that data=None is actually a meaningful value. If that is the case then this approach will need to be tweaked a bit.

Attachments (2)

client_mutable_argument_fix_01.patch (3.6 KB ) - added by sleepydragon 11 years ago.
client_mutable_argument_fix_02.patch (3.4 KB ) - added by Aloïs Guillopé 11 years ago.

Download all attachments as: .zip

Change History (12)

by sleepydragon, 11 years ago

comment:1 by Simon Charette, 11 years ago

Triage Stage: UnreviewedAccepted
Version: 1.6master

It looks like the dict creation should be moved at the RequestFactory level which also suffer from the same issues.

comment:2 by Aloïs Guillopé, 11 years ago

Owner: changed from nobody to anonymous
Status: newassigned

by Aloïs Guillopé, 11 years ago

comment:3 by Aloïs Guillopé, 11 years ago

This should do. Could you have a look? Is the test I added any use?

comment:4 by Claude Paroz, 11 years ago

To make sense, a test should fail before applying the patch, which is not the case here. I'll commit an amended patch soon (without tests).

comment:5 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 2a31d00933fd1d06ae3013f2c0eb8f4982fa7db6:

Fixed #21740 -- Stopped using mutable default arguments in test client

Thanks Denver Coneybeare for the report and initial patch, and
Atala for another patch.

comment:6 by tony-zhu, 10 years ago

Resolution: fixed
Status: closednew

This fix actually changed the behavior of the test client. I have some tests about HTTP header handling, which I pass an empty string as the request content. So it is like:

self.client.post(url, '', content_type='application/json',
                                    **{
                                        'CUSTOM_HEADER': header_1,
                                    })

Before this change everything worked fine.

The request.body became "{}" after this change. That is because when data is an empty string

data or {}

returns {} instead of ""

comment:7 by Claude Paroz, 10 years ago

comment:8 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

@tony-zhu FYI, it's preferred to open a new ticket for new bugs after a ticket is released.

PR looks good me; it just needs release notes.

comment:9 by Claude Paroz <claude@…>, 10 years ago

Resolution: fixed
Status: newclosed

In f0bb3c98cc9e128cb0c1622be9eb41a26794c91f:

Fixed #21740 -- Allowed test client data to be an empty string

This fixes a regression introduced by 2a31d00933.
Thanks tony-zhu for the report.

comment:10 by Claude Paroz <claude@…>, 10 years ago

In 53bc81dca3d785d0b399cacbf84cc660355895fc:

[1.7.x] Fixed #21740 -- Allowed test client data to be an empty string

This fixes a regression introduced by 2a31d00933.
Thanks tony-zhu for the report.
Backport of f0bb3c98cc from master.

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