Opened 4 years ago

Closed 4 years ago

#22163 closed Cleanup/optimization (fixed)

select_for_update should take nowait directly

Reported by: kenny.macdermid@… Owned by: Aymeric Augustin
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Cea Stapleton, numerodix@…, Tim Martin Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no


select_for_update() accepts any number of arguments which it then completely ignores. This can lead you situations where you may type: Model.objects.select_for_update(id=1) and expect to be changing one object when in reality you're changing all the objects.

Change History (8)

comment:1 Changed 4 years ago by Cea Stapleton

I think the fix for this is pretty simple. Could modify method select_for_update(self, **kwargs) to select_for_update(self, nowait=False). This yields the reasonable error TypeError: select_for_update() got an unexpected keyword argument 'id' when passing in (id=1).

This will prevent the misunderstanding that filtering can happen within the method.

    def select_for_update(self, nowait=False):
        Returns a new QuerySet instance that will select objects with a
        FOR UPDATE lock.
        obj = self._clone()
        obj._for_write = True
        obj.query.select_for_update = True
        obj.query.select_for_update_nowait = nowait
        return obj

comment:2 Changed 4 years ago by Cea Stapleton

Cc: Cea Stapleton added

comment:3 Changed 4 years ago by Martin Matusiak

Cc: numerodix@… added

Hm. I would be inclined to say that you shouldn't assume that select_for_update contains the semantics of a filter when that's not stated in the documentation. There are other reasons why functions may take **kwargs after all.

comment:4 Changed 4 years ago by kenny.macdermid@…

@numerodix While I agree and the documentation shows it used correctly it still violates the principle of least astonishment, making it very easy to modify the wrong set of objects. I could understand if **kwargs was then passed to something else, but that didn't seem to be the case here.

I doubt I'm the only one to have made or that will make the same mistake.

comment:5 Changed 4 years ago by Tim Martin

I agree that nobody should assume that just because a function takes **kwargs that it will have filter semantics. But there doesn't seem to be any good reason for the function as it stands to use **kwargs. Use of arbitrary keyword arguments should be the exception, not the rule. The public documentation doesn't say that arbitrary arguments are accepted, so any code that would be broken by removal of the **kwargs was already relying on undocumented behaviour.

Is the **kwargs left in for backward compatibility? If so, perhaps a note should be added to the docstring.

comment:6 Changed 4 years ago by Tim Martin

Cc: Tim Martin added

comment:7 Changed 4 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin
Status: newassigned
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

This appears to be an oversight in the original implementation. I'm just going to change it.

comment:8 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: assignedclosed

In 222262ca2354abadf259f4186a0b43f583ed1bd1:

Fixed #22163 -- Stopped ignoring unhandled kwargs in select_for_update.

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