Opened 5 years ago

Closed 5 years ago

#30024 closed New feature (fixed)

The test client request methods should raise an error when passed None as a data value

Reported by: Jon Dufresne Owned by: nobody
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: no UI/UX: no

Description

Both GET and POST encoded data do not have a concept of None or NULL. The closest approximation is an empty string value or omitting the key. For example, in a GET request this could be either /my-url/?my_field= or simply /my-url/ but not /my-url/?my_field=None)

When onboarding new developers to projects, this can cause confusion to those less familiar with these details. For example, a new developer may try the following:

def test_setting_value_to_none(self):
    self.client.post('/my-url/', {'my_field': None})
    self.assertIsNone(...)

In current versions of Django, behind the scenes, this None gets coerced to the string 'None' by the test client. The Django form field classes don't recognize the string 'None' as an empty value (good) and so this test doesn't pass. Where the new developer thought a field would be assigned None they instead get a form error. Depending on the developers' knowledge of these details, this could take much debugging or consulting a colleague.

I think we can recognize this pattern as a programming mistake and raise an informative error to guide the developer. I propose something like the following, but am open to suggestions:

TypeError: Cannot encode None as POST data. Did you mean to pass an empty string or omit the value?

For GET requests, the query string data is processed by django.utils.http.urlencode(). So perhaps this same check can be done there as encoding None in a URL query string as 'None' is rarely the intended behavior. For those that really want the string 'None' in the query string, they can pass the string 'None'.

Change History (5)

comment:1 by Jon Dufresne, 5 years ago

Has patch: set

comment:2 by Carlton Gibson, 5 years ago

I think this is fair. There's no case where None is the right thing to use. (As Jon says, maybe 'None' if you really mean that.)

comment:3 by Carlton Gibson, 5 years ago

Triage Stage: UnreviewedAccepted

comment:7 by Carlton Gibson, 5 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Tim Graham <timograham@…>, 5 years ago

Resolution: fixed
Status: newclosed

In 6fe9c45b:

Fixed #30024 -- Made urlencode() and Client raise TypeError when None is passed as data.

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