[Django] #29178: Mutable default in `Index` constructor

[Django]

#29178: Mutable default in `Index` constructor
-------------------------------------+-------------------------------------
Reporter: Flavio | Owner: nobody
Curella |
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: master
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
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:

* http://effbot.org/zone/default-values.htm
* http://docs.python-guide.org/en/latest/writing/gotchas/

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/29178>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#29178: Mutable default in `Index` constructor
-------------------------------------+-------------------------------------

Reporter: Flavio Curella | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:1>

[Django]

#29178: Index.__init__()'s fields parameter shouldn't be mutable

-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: nobody

Type: | Status: new
Cleanup/optimization |

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

An older [https://github.com/django/django/pull/9368 PR] suggests
accepting a list or tuple and defaulting to `fields=()`. Thoughts?

--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:2>

[Django]

#29178: Index.__init__()'s fields parameter shouldn't be mutable
-------------------------------------+-------------------------------------

Reporter: Flavio Curella | Owner: Marc
Type: | Kirkwood
Cleanup/optimization | Status: assigned

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Marc Kirkwood):

* owner: nobody => Marc Kirkwood
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:3>

[Django]

#29178: Index.__init__()'s fields parameter shouldn't be mutable
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Marc
Type: | Kirkwood
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Flavio Curella):

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:4>

[Django]

#29178: Index.__init__()'s fields parameter shouldn't be mutable
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Marc
Type: | Kirkwood
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Marc Kirkwood):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:5>

[Django]

#29178: Index.__init__()'s fields parameter shouldn't be mutable
-------------------------------------+-------------------------------------

Reporter: Flavio Curella | Owner: (none)
Type: | Status: new
Cleanup/optimization |

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Marc Kirkwood):

* owner: Marc Kirkwood => (none)
* status: assigned => new

--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:6>

[Django]

#29178: Index.__init__()'s fields parameter shouldn't be mutable
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)

Type: | Status: new
Cleanup/optimization |

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (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

--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:7>

[Django]

#29178: Index.__init__()'s fields parameter shouldn't be mutable
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: (none)

Type: | Status: new
Cleanup/optimization |

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (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.

--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:8>

[Django]

#29178: Index.__init__()'s fields parameter shouldn't be mutable
-------------------------------------+-------------------------------------

Reporter: Flavio Curella | Owner: atmishra
Type: | Status: assigned

Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by atmishra):

* status: new => assigned

* owner: (none) => atmishra

Comment:

I would like to work on this ticket.

--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:9>

[Django]

#29178: Index.__init__()'s fields parameter shouldn't be mutable
-------------------------------------+-------------------------------------

Reporter: Flavio Curella | Owner: Atul
Type: | mishra
Cleanup/optimization | Status: assigned

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham):

* has_patch: 0 => 1
* stage: Accepted => Ready for checkin

Comment:

There's already a [https://code.djangoproject.com/ticket/29178 PR].

--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:10>

[Django]

#29178: Index.__init__()'s fields parameter shouldn't be mutable
-------------------------------------+-------------------------------------
Reporter: Flavio Curella | Owner: Atul
Type: | mishra

Cleanup/optimization | Status: closed

Component: Database layer | Version: master
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham <timograham@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"10c0fe528a2089f4ba206caa50f9a98f8d9c8a15" 10c0fe5]:
{{{
#!CommitTicketReference repository=""
revision="10c0fe528a2089f4ba206caa50f9a98f8d9c8a15"
Fixed #29178 -- Allowed Index.fields to accept a tuple.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/29178#comment:11>