Opened 7 years ago
Closed 7 years ago
#29178 closed Cleanup/optimization (fixed)
Index.__init__()'s fields parameter shouldn't be mutable
Reported by: | Flavio Curella | Owned by: | Atul mishra |
---|---|---|---|
Component: | Database layer (models, ORM) | 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: | yes | UI/UX: | no |
Description
the __init__
method of the Index
class has the following signature (https://github.com/django/django/blob/master/django/db/models/indexes.py#L15):
def __init__(self, *, fields=[], name=None, db_tablespace=None):
The fields
argument is set to a mutable object, which is usually considered bad practice as it can lead to unexpected results:
There are some valid uses of mutable defaults, but I can't see any code taking advantage of the mutability by looking at the code of the Index
class.
If there's no good reason for the default to be mutable, it should be changed to:
def __init__(self, *, fields=None, name=None, db_tablespace=None): if fields is None: fields = [] # ...rest of the code
If there's a good reason, I think we should document it in a comment.
Change History (11)
comment:1 by , 7 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 7 years ago
Summary: | Mutable default in `Index` constructor → Index.__init__()'s fields parameter shouldn't be mutable |
---|
comment:3 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
follow-up: 5 comment:4 by , 7 years ago
I think it should be changed to accept a list or a tuple as in https://github.com/django/django/pull/9368
Re: defaulting to ()
vs defaulting to None
, I don't really have a strong opinion. I'm used to see the arg=None
pattern more often, but that could just be cargo-culting.
comment:5 by , 7 years ago
Replying to Flavio Curella: I agree with that; looks like the PR you linked to solves the problem better.
comment:6 by , 7 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:7 by , 7 years ago
@Tim Graham: I think we can just link https://github.com/django/django/pull/9368 to this ticket and use it as it is
comment:8 by , 7 years ago
To elaborate: the only reason I think it should support both list and tuples is because we have other places where we do so: for example, Meta.ordering
comes to mind.
But I'm with you if you'd rather not persevere with that pattern, and push the user to use tuples
whenever possible.
comment:9 by , 7 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
I would like to work on this ticket.
comment:10 by , 7 years ago
Has patch: | set |
---|---|
Triage Stage: | Accepted → Ready for checkin |
There's already a PR.
An older PR suggests accepting a list or tuple and defaulting to
fields=()
. Thoughts?