Skip to content

[16.0][FIX] server_action_mass_edit: stop poisoning the registry on onchange#1292

Draft
DarioLodeiros wants to merge 1 commit into
OCA:16.0from
DarioLodeiros:16.0-fix-mass-edit-onchange-poison-fields
Draft

[16.0][FIX] server_action_mass_edit: stop poisoning the registry on onchange#1292
DarioLodeiros wants to merge 1 commit into
OCA:16.0from
DarioLodeiros:16.0-fix-mass-edit-onchange-poison-fields

Conversation

@DarioLodeiros

Copy link
Copy Markdown
Member

Symptom

In production we have been seeing random KeyError: None traces every few minutes, fired from unrelated views (bank_rec_widget, knowledge.article, ...) and surfaced to the web client as RPC_ERROR:

File ".../odoo/models.py", line 6347, in _has_onchange
    for dep in self.pool.get_dependent_fields(field.base_field)
File ".../odoo/modules/registry.py", line 351, in get_dependent_fields
    if field not in self._field_triggers:
File ".../odoo/modules/registry.py", line 439, in _field_triggers
    dependencies = list(field.resolve_depends(self))
File ".../odoo/fields.py", line 775, in resolve_depends
    Model0 = registry[self.model_name]
File ".../odoo/modules/registry.py", line 186, in __getitem__
    return self.models[model_name]
KeyError: None

The trace never points at this module on the surface, but the only thing that has been planting Field instances with model_name = None into the registry is MassEditingWizard.onchange.

Root cause

MassEditingWizard.onchange injects dynamic Selection fields by assigning them straight into self._fields:

dynamic_fields["selection__" + line.field_id.name] = fields.Selection(
    [()], default="ignore"
)
self._fields.update(dynamic_fields)

These Field objects never go through __set_name__, so model_name and name stay at the default None. Any later access to registry._field_triggers — for example from _has_onchange while building a view for any other model — calls field.resolve_depends(self)registry[None]KeyError: None.

The for field in dynamic_fields: self._fields.pop(field) cleanup is also not wrapped in try/finally. If anything inside super().onchange() raises, the orphan Field instances are left in mass.editing.wizard._fields (a class-level dict, shared by every request on that worker). The worker stays poisoned for the rest of its lifetime: every request that touches _field_triggers then explodes randomly on completely unrelated views.

I could not find an open issue/PR covering this exact scenario in OCA/server-ux. PR #1203 deals with the view cache mechanism on 17.0+, which is unrelated.

Fix

  • Call Field.__set_name__ on each dynamic field before inserting it into _fields, so model_name / name are populated and registry lookups resolve to mass.editing.wizard as expected.
  • Wrap the super().onchange() call in try/finally, so the dynamic fields are always removed from _fields — even when the parent onchange raises.

Regression test

test_onchange_dynamic_fields_are_bound_and_cleaned asserts both invariants:

  1. While super().onchange() is running, each dynamic Field has model_name == 'mass.editing.wizard' (without the fix the test fails with None != 'mass.editing.wizard': Dynamic field selection__bank_ids was injected with model_name=None).
  2. _fields is restored to its original state on both the success path and a failure path that forces super().onchange() to raise.

Affected versions

The same pattern is present on 17.0, 18.0 and the 19.0 migration PRs. I will open companion PRs once this one is reviewed.

``MassEditingWizard.onchange`` injects dynamic ``Selection`` fields into
``self._fields`` by assigning them directly into the dict. Because they
never go through ``__set_name__``, the resulting ``Field`` instances
have ``model_name=None`` and ``name=None``.

Once a worker hits this code path, the next time any model is asked
about its onchange dependencies (``Model._has_onchange`` →
``Registry.get_dependent_fields`` → ``Field.resolve_depends``) Odoo
calls ``registry[self.model_name]``, which is ``registry[None]`` and
raises ``KeyError: None``. The exception propagates back to the web
client as a random RPC error on completely unrelated views
(``bank_rec_widget``, ``knowledge.article``, etc.).

To make matters worse, the cleanup ``for field in dynamic_fields:
self._fields.pop(field)`` is not wrapped in ``try/finally``, so any
exception inside ``super().onchange()`` leaves the orphan Fields behind
in the class-level ``_fields`` dict. The worker stays poisoned until
the process is restarted.

This patch:

* Calls ``Field.__set_name__`` on each dynamic Field so it is bound to
  the owner class (``model_name`` and ``name`` are populated). The
  registry lookup then resolves to ``mass.editing.wizard`` as expected.
* Wraps the ``super().onchange()`` call in ``try/finally`` so the
  dynamic fields are always removed from ``_fields``, even when the
  parent ``onchange`` raises.

A regression test asserts both invariants: the dynamic Field has the
correct ``model_name`` during the parent ``onchange`` call, and
``_fields`` is restored to its original state on both success and
failure paths.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mod:server_action_mass_edit Module server_action_mass_edit series:16.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants