[Django] #21740: client.py uses mutable default arguments, which is bad practice
Django
--------------------------------------+--------------------
Reporter: sleepydragon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 1 | UI/UX: 0
--------------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21740>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: sleepydragon | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: => 0
* needs_docs: => 0
* version: 1.6 => master
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
It looks like the `dict` creation should be moved at the `RequestFactory`
level which also suffer from the same issues.
--
Ticket URL: <https://code.djangoproject.com/ticket/21740#comment:1>
Django
Reporter: sleepydragon | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Testing framework | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by Atala):
* owner: nobody => anonymous
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/21740#comment:2>
Django
Reporter: sleepydragon | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Testing framework | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by Atala):
This should do. Could you have a look? Is the test I added any use?
--
Ticket URL: <https://code.djangoproject.com/ticket/21740#comment:3>
Django
Reporter: sleepydragon | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Testing framework | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (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).
--
Ticket URL: <https://code.djangoproject.com/ticket/21740#comment:4>
Django
Reporter: sleepydragon | Owner: anonymous
Cleanup/optimization | Version: master
Component: Testing framework | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"2a31d00933fd1d06ae3013f2c0eb8f4982fa7db6"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21740#comment:5>
Django
Reporter: sleepydragon | Owner: anonymous
Cleanup/optimization | Version: master
Component: Testing framework | Resolution:
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
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 ""
--
Ticket URL: <https://code.djangoproject.com/ticket/21740#comment:6>
Django
Reporter: sleepydragon | Owner: anonymous
Type: | Status: new
Cleanup/optimization | Version: master
Component: Testing framework | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by claudep):
https://github.com/django/django/pull/3397 should fix the regression.
--
Ticket URL: <https://code.djangoproject.com/ticket/21740#comment:7>
Django
Reporter: sleepydragon | Owner: anonymous
Type: | Status: new
Cleanup/optimization | Version: master
Component: Testing framework | Resolution:
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
Comment:
@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21740#comment:8>
Django
Reporter: sleepydragon | Owner: anonymous
Cleanup/optimization | Version: master
Component: Testing framework | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Claude Paroz <claude@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"f0bb3c98cc9e128cb0c1622be9eb41a26794c91f"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21740#comment:9>
Django
Reporter: sleepydragon | Owner: anonymous
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Testing framework | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Claude Paroz <claude@…>):
In [changeset:"53bc81dca3d785d0b399cacbf84cc660355895fc"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21740#comment:10>