Opened 4 years ago

Closed 2 years ago

#16134 closed Bug (fixed)

django.db.models.fields.Field instance can not be compared with None

Reported by: amcharg@… Owned by: adamsc64
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: hongshuning@…, stefano.borgia@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

Instances of django.db.models.fields.Field can not be compared with None. Eg: field == None

Repro: Compare a field instance with None. This fails since cmp assumes the other operand is a field instance and eq and ne are not defined.

Attachments (2)

16134_fix.diff (1.3 KB) - added by RoboHamburger 4 years ago.
Patch that adds regression test and fix.
16134_fix2.diff (702 bytes) - added by hongshuning 3 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by lukeplant

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

Accepted. (BTW, the standard Python idiom for comparing against None is 'x is None' or 'x is not None', rather than == or !=, because it is more efficient and unambiguous. But the ticket is valid nonetheless).

Changed 4 years ago by RoboHamburger

Patch that adds regression test and fix.

comment:2 Changed 4 years ago by RoboHamburger

  • Has patch set

comment:3 Changed 4 years ago by lukeplant

  • Triage Stage changed from Accepted to Ready for checkin

LGTM.

comment:4 Changed 4 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to Accepted
  • UI/UX unset

Hmm... I'm a little unclear on the use-case for this. Instantiating fields is internal to Django -- normal models expose Python attributes and the Field instances are hidden in the Meta class (which is opaque). Making this change "just because we can" isn't necessary. What's the normal code pattern that triggers a problem here?

comment:5 Changed 4 years ago by adamnelson

  • Patch needs improvement set

comment:6 Changed 3 years ago by hongshuning

  • Cc hongshuning@… added

Changed 3 years ago by hongshuning

comment:7 Changed 3 years ago by hongshuning

I modify the test case according to mtredinnick's propose based on dev branch. The test case succeed without modify any code.

comment:8 Changed 2 years ago by antonigno

  • Cc stefano.borgia@… added
  • Owner changed from nobody to antonigno
  • Status changed from new to assigned

comment:9 Changed 2 years ago by antonigno

  • Owner antonigno deleted
  • Status changed from assigned to new

comment:10 Changed 2 years ago by adamsc64

  • Owner set to adamsc64
  • Status changed from new to assigned

comment:11 Changed 2 years ago by adamsc64

Simon Charette's commit at 5cbfb48b (intended to fix #17851) actually resolves this issue also. This commit made its way into stable/1.5.x but is not yet in stable/1.4.x. As this is not a security issue, both Russell Keith-Magee and Andrew Godwin tell me not to bother with a pull request.

comment:12 Changed 2 years ago by adamsc64

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.
Back to Top