Opened 3 years ago

Closed 2 years ago

Last modified 2 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: master
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 3 years ago.
client_mutable_argument_fix_02.patch (3.4 KB) - added by Aloïs Guillopé 3 years ago.

Download all attachments as: .zip

Change History (12)

Changed 3 years ago by sleepydragon

comment:1 Changed 3 years ago by Simon Charette

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 Changed 3 years ago by Aloïs Guillopé

Owner: changed from nobody to anonymous
Status: newassigned

Changed 3 years ago by Aloïs Guillopé

comment:3 Changed 3 years ago by Aloïs Guillopé

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

comment:4 Changed 3 years ago by Claude Paroz

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 Changed 3 years ago by Claude Paroz <claude@…>

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 Changed 2 years ago by tony-zhu

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 Changed 2 years ago by Claude Paroz

comment:8 Changed 2 years ago by Tim Graham

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 Changed 2 years ago by Claude Paroz <claude@…>

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 Changed 2 years ago by Claude Paroz <claude@…>

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