Opened 20 months ago

Closed 10 months ago

Last modified 10 months 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 20 months ago.
client_mutable_argument_fix_02.patch (3.4 KB) - added by Atala 20 months ago.

Download all attachments as: .zip

Change History (12)

Changed 20 months ago by sleepydragon

comment:1 Changed 20 months ago by charettes

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.6 to master

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

comment:2 Changed 20 months ago by Atala

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

Changed 20 months ago by Atala

comment:3 Changed 20 months ago by Atala

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

comment:4 Changed 20 months ago by claudep

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 20 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 10 months ago by tony-zhu

  • Resolution fixed deleted
  • Status changed from closed to new

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 10 months ago by claudep

comment:8 Changed 10 months ago by timgraham

  • Triage Stage changed from Accepted to Ready 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 10 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

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 10 months 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