Opened 8 years ago
Closed 8 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 , 8 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:2 by , 8 years ago
| Summary: | Mutable default in `Index` constructor → Index.__init__()'s fields parameter shouldn't be mutable |
|---|
comment:3 by , 8 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
follow-up: 5 comment:4 by , 8 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 , 8 years ago
Replying to Flavio Curella: I agree with that; looks like the PR you linked to solves the problem better.
comment:6 by , 8 years ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
comment:7 by , 8 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 , 8 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 , 8 years ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
I would like to work on this ticket.
comment:10 by , 8 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?