Opened 3 years ago

Closed 3 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: master
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 Changed 3 years ago by Simon Charette

Triage Stage: UnreviewedAccepted

comment:2 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Marc Kirkwood

Owner: changed from nobody to Marc Kirkwood
Status: newassigned

comment:4 Changed 3 years ago by Flavio Curella

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.

Last edited 3 years ago by Flavio Curella (previous) (diff)

comment:5 in reply to:  4 Changed 3 years ago by Marc Kirkwood

Replying to Flavio Curella: I agree with that; looks like the PR you linked to solves the problem better.

comment:6 Changed 3 years ago by Marc Kirkwood

Owner: Marc Kirkwood deleted
Status: assignednew

comment:7 Changed 3 years ago by Flavio Curella

@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 Changed 3 years ago by Flavio Curella

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 Changed 3 years ago by Atul mishra

Owner: set to Atul mishra
Status: newassigned

I would like to work on this ticket.

comment:10 Changed 3 years ago by Tim Graham

Has patch: set
Triage Stage: AcceptedReady for checkin

There's already a PR.

comment:11 Changed 3 years ago by Tim Graham <timograham@…>

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