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 Simon Charette, 7 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Tim Graham, 7 years ago

Summary: Mutable default in `Index` constructorIndex.__init__()'s fields parameter shouldn't be mutable

An older PR suggests accepting a list or tuple and defaulting to fields=(). Thoughts?

comment:3 by Marc Kirkwood, 7 years ago

Owner: changed from nobody to Marc Kirkwood
Status: newassigned

comment:4 by Flavio Curella, 7 years ago

I think it should It should 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.

Version 0, edited 7 years ago by Flavio Curella (next)

in reply to:  4 comment:5 by Marc Kirkwood, 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 Marc Kirkwood, 7 years ago

Owner: Marc Kirkwood removed
Status: assignednew

comment:7 by Flavio Curella, 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 Flavio Curella, 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 Atul mishra, 7 years ago

Owner: set to Atul mishra
Status: newassigned

I would like to work on this ticket.

comment:10 by Tim Graham, 7 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

There's already a PR.

comment:11 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In 10c0fe5:

Fixed #29178 -- Allowed Index.fields to accept a tuple.

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