select_for_update should take nowait directly

[kenny.macdermid@…]

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.

[Cea Stapleton]

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

[Martin Matusiak]

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.

[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.

[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.

[Aymeric Augustin]

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

[Aymeric Augustin <aymeric.augustin@…>]

In 222262ca2354abadf259f4186a0b43f583ed1bd1:

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