Skip to content

OD103 cursor-commit

Business code commits the cursor, breaking the request's transaction.

Category Severity Series Needs the index
Correctness Warning all no

What it reports

A call to commit() on the request's cursor: cr.commit(), self.env.cr.commit() or self._cr.commit(), a cr parameter included.

It stays silent on a commit on a cursor the function opens with a .cursor() call, which is a transaction of its own: a name it binds to the call, with with registry.cursor() as cr: or cr = registry.cursor(), whether on self.pool, self.env.registry or db_connect(dbname), or the cursor of an environment built on such a name or on the call itself, with api.Environment(cr, ...), self.env(cr=cr) or with_env(). A name bound to anything else, such as cr = new_cr or self.env.cr, may hold the request's cursor and is reported.

After an if or a loop, a name holds such a cursor only when every path reaching the commit binds it to one. The paths out of an if are its branches that do not end in return, raise, continue, break or sys.exit(), and the path past them when it has no else. A branch, or a loop body, also ends in a block that leaves on every way through it: a with whose own last statement does, even under a manager such as contextlib.suppress that may swallow the exception, an if with an else, a match whose last case, such as case _:, matches anything and has no guard, and a try whose finally leaves, or that has no finally and whose handlers leave along with its body or its else. Each run of a loop body starts from every path reaching it: the one before the loop, the end of a run that gets there and each continue. The paths out of the loop are each break in its body outside a nested loop, wherever it sits, and, unless it is while True:, the point where a run would start, through the loop's else. So cr = self.env.cr, then if new: cr = registry.cursor(), then cr.commit() is reported. A path on which the function never bound the name adds nothing, since Python leaves it unbound there, and neither does one binding it to None or False, since code tests such a name before using it. The handlers of a try and the cases of a match are read in the order of the source.

A function nested in it sees the names as they stand on the paths reaching its def, each binding after the def taking over since the function may run later, unless a parameter of its own hides them. A lambda sees none of them.

Why it matters

Odoo runs each request, cron job and RPC call in one transaction: it commits when the call ends and rolls everything back when it raises. A commit in the middle makes the work done so far permanent, so an error later leaves half of the operation applied and the user is told it failed. Odoo also retries a whole request when PostgreSQL reports a concurrent update, and the retry repeats the committed work, sending the same message or creating the same record twice.

Committing on purpose has its place, such as a cron job that processes a large batch and keeps its progress when a later record fails; from 19.0, ir.cron's _commit_progress() does that. Review such a call and suppress the finding on its line.

Example

models/product_template.py
class ProductTemplate(models.Model):
    _inherit = "product.template"

    def action_push_prices(self):
        for template in self:
            template._send_price_to_shop()
            template.x_price_pushed_on = fields.Datetime.now()
            self.env.cr.commit()

If the third template fails, the first two stay marked as pushed while the user sees an error.

models/product_template.py
class ProductTemplate(models.Model):
    _inherit = "product.template"

    def action_push_prices(self):
        for template in self:
            template._send_price_to_shop()
            template.x_price_pushed_on = fields.Datetime.now()

Suppressing

self.env.cr.commit()  # odooscan: ignore[OD103]

See suppressing a finding.