Code

Opened 3 years ago

Closed 10 months 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 3 years ago.
Patch that adds regression test and fix.
16134_fix2.diff (702 bytes) - added by hongshuning 19 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 3 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 3 years ago by RoboHamburger

Patch that adds regression test and fix.

comment:2 Changed 3 years ago by RoboHamburger

  • Has patch set

comment:3 Changed 3 years ago by lukeplant

  • Triage Stage changed from Accepted to Ready for checkin

LGTM.

comment:4 Changed 3 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 3 years ago by adamnelson

  • Patch needs improvement set

comment:6 Changed 19 months ago by hongshuning

  • Cc hongshuning@… added

Changed 19 months ago by hongshuning

comment:7 Changed 19 months 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 14 months ago by antonigno

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

comment:9 Changed 14 months ago by antonigno

  • Owner antonigno deleted
  • Status changed from assigned to new

comment:10 Changed 10 months ago by adamsc64

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

comment:11 Changed 10 months 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 10 months ago by adamsc64

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.