[17.0][FIX] server_action_mass_edit: stop poisoning the registry on onchange#1293
Open
DarioLodeiros wants to merge 3 commits into
Open
[17.0][FIX] server_action_mass_edit: stop poisoning the registry on onchange#1293DarioLodeiros wants to merge 3 commits into
DarioLodeiros wants to merge 3 commits into
Conversation
``MassEditingWizard.onchange`` injects dynamic ``Selection`` / ``Text`` 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. The cleanup ``for field in dynamic_fields: self._fields.pop(field)`` is also not wrapped in ``try/finally``, so any exception inside ``super().onchange()`` leaves the orphan Fields (and the temporary view record) behind. The worker's class-level ``_fields`` 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). * Wraps the ``super().onchange()`` call in ``try/finally`` so both the dynamic fields and the temporary view are always cleaned up, even when the parent ``onchange`` raises. A regression test asserts both invariants on the success and the failure paths.
OCA-git-bot
added
series:17.0
mod:server_action_mass_edit
The previous version of ``test_onchange_dynamic_fields_are_bound_and_cleaned`` monkey-patched ``odoo.models.Model.onchange`` to intercept the parent ``onchange`` call. In 17.0+ the real implementation lives on ``odoo.addons.web.models.Base.onchange`` (``_inherit='base'``) while ``Model.onchange`` is a ``NotImplementedError`` raiser. Tampering with ``Model.onchange`` confused the MRO of unrelated models in subsequent tests, leaking the raiser into ``test_onchanges``. Instead, patch ``fields.Field.__set_name__`` to record every binding call. This proves that each dynamic ``Field`` injected by ``MassEditingWizard.onchange`` is bound to ``mass.editing.wizard`` with the correct attribute name (without the production fix the binding is missed entirely and ``model_name`` stays at ``None``), while leaving the rest of the ORM untouched. The failure-path assertion is dropped from the test: the ``try/finally`` in the fix is straightforward, and forcing the parent ``onchange`` to raise without altering ``Model.onchange`` would require re-implementing the wizard's setup in the test.
After ``__set_name__`` is properly called on the dynamic ``Selection`` fields, the empty selection literal ``[()]`` makes ``web.Base.onchange`` trip on ``Selection.get_values`` (which tries to iterate ``[()]`` as ``(value, label)`` pairs). That validation path was silently skipped before the fix because the half-built ``Field`` had ``model_name=None``. Stub ``web.Base.onchange`` with a no-op for the test: we only care about whether ``MassEditingWizard.onchange`` binds the dynamic fields via ``__set_name__``, not about what the parent ``onchange`` does with the placeholder selection. The stub uses ``mock.patch.object`` so the original method is restored even if the test fails, without touching ``models.Model.onchange`` (which would confuse the MRO of unrelated models).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
Random
KeyError: Nonetraces from unrelated views (bank_rec_widget,knowledge.article, ...) surface to the web client asRPC_ERROR:The trace never points at this module on the surface, but the only thing planting
Fieldinstances withmodel_name = Noneinto the registry isMassEditingWizard.onchange.Root cause
MassEditingWizard.onchangeinjects dynamicSelection/Textfields by assigning them straight intoself._fields:These
Fieldobjects never go through__set_name__, somodel_nameandnamestay at the defaultNone. Any later access toregistry._field_triggers— for example from_has_onchangewhile building a view for any other model — callsfield.resolve_depends(self)→registry[None]→KeyError: None.The cleanup (
for field in dynamic_fields: self._fields.pop(field)plus the temporary view deletion) is not wrapped intry/finally. If anything insidesuper().onchange()raises, the orphanFieldinstances are left inmass.editing.wizard._fields(a class-level dict, shared by every request on that worker) and the temporary view stays orphaned in the database. The worker stays poisoned for the rest of its lifetime: every request that touches_field_triggersthen 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, which is unrelated.
Fix
Field.__set_name__on each dynamic field before inserting it into_fields, somodel_name/nameare populated and registry lookups resolve tomass.editing.wizardas expected.super().onchange()call (and the cached value rewrite) intry/finally, so both the dynamic fields and the temporary view are always cleaned up — even when the parentonchangeraises.Regression test
test_onchange_dynamic_fields_are_bound_and_cleanedasserts both invariants:super().onchange()is running, each dynamic Field hasmodel_name == 'mass.editing.wizard'(without the fix the test fails withNone != 'mass.editing.wizard': Dynamic field selection__bank_ids was injected with model_name=None)._fieldsis restored to its original state on both the success path and a failure path that forcessuper().onchange()to raise.Related PRs
Companion fixes for 16.0 / 18.0 are linked from this PR (16.0: #1292).