#22163 closed Cleanup/optimization (fixed)

select_for_update should take nowait directly

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

Description

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 17 months ago by ceaess

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I think the fix for this is pretty simple. Could modify query.py 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 17 months ago by ceaess

  • Cc ceaess added

comment:3 Changed 17 months ago by numerodix

  • 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 17 months 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 17 months ago by timmartin

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 17 months ago by timmartin

  • Cc timmartin added

comment:7 Changed 17 months ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/optimization

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

comment:8 Changed 17 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 222262ca2354abadf259f4186a0b43f583ed1bd1:

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

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