fix(odoo-spo): read selection_add + merge across classes (codex #530 follow-up)#532
Conversation
Odoo addons extend an existing Selection field's domain via
`_inherit` + `fields.Selection(selection_add=[('reviewed','Reviewed')])`
WITHOUT redeclaring the base list. `_extract_selection_values` only read
the base `selection=` arg, so the emitted `selection_value` set was
incomplete and a downstream `ASSERT $value IN [...]` would reject records
in legal extension-added states.
Two fixes:
1. `_extract_selection_values` now reads BOTH the base list (positional
arg 0 or `selection=`) AND `selection_add=`, unioning base-first with
order-preserving dedup (factored into `_selection_tuple_keys`).
2. `_scan_file` binding MERGES selection keys per (model, field) instead
of last-write-wins — the base declaration and the `selection_add`
extension live in DIFFERENT classes, so the field's value set must
accumulate across scans, not overwrite.
Tests (53 -> 57):
- selection_add kwarg alone -> added keys
- base + selection_add -> union, base first
- selection_add dedups against base
- cross-class merge: base in _name class + selection_add in _inherit
class -> accumulated [draft, posted, reviewed]
python3 -m unittest tests.test_spo_enrich : 57/57 OK
|
Warning Review limit reached
More reviews will be available in 44 minutes and 44 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4b161aac8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| seen.add(key) | ||
| out.append(key) | ||
| _selection_tuple_keys(base, out, seen) | ||
| _selection_tuple_keys(add, out, seen) |
There was a problem hiding this comment.
This now treats selection_add as enough to return a non-empty value list even when the base selection was dynamic or a bare constant, which this helper intentionally cannot resolve. In a common inherited-field case like a base state = fields.Selection(STATES) plus state = fields.Selection(selection_add=[('reviewed', ...)]), the scan records only reviewed; enrich then emits a closed selection_value set with just the extension key, so downstream ASSERT generation rejects the original legal base states. Please keep selection_add as a pending extension and only emit a closed domain once a static base list is known.
Useful? React with 👍 / 👎.
| # live in DIFFERENT classes, so a given (model, field) | ||
| # accumulates across scans (codex #530). Order-preserving, | ||
| # de-duplicated union. | ||
| acc = selections.setdefault((mu, field_name), []) |
There was a problem hiding this comment.
This merge is applied to every fields.Selection declaration, not just selection_add extensions. When an _inherit class redeclares the whole field with selection=[...] to replace or reorder the domain, stale keys from the earlier definition remain in the accumulated set, so the generated selection_value facts allow values the final Odoo field no longer accepts. The scan needs to distinguish additive selection_add values from a full base selection redeclaration and overwrite/reset for the latter.
Useful? React with 👍 / 👎.
…ce/extend/poison + mro rustfmt Two codex P2 findings on merged #532 were real semantic bugs (live in main), both producing a WRONG `ASSERT $value IN [...]` — the silent-data-rejection class that matters for faithful DDL — plus a rustfmt miss on merged #533. # #532 finding 1 — selection_add must not close a dynamic domain A `Selection(selection_add=[('x','X')])` extension onto a base that is DYNAMIC (`Selection(STATE_CONST)` / `selection='_compute_x'`) was emitting `['x']` as if it were the complete closed set, so the projected `ASSERT $value IN ['x']` would reject every legal base value. # #532 finding 2 — full redeclarations must replace, not union The cross-class merge unioned EVERY `fields.Selection` declaration, so an `_inherit` class that REDECLARES the whole field with `selection=[...]` (replace/reorder) left stale keys from the earlier definition in the set. # The correct three-way semantics `_extract_selection(call) -> (base_dynamic, base_keys, add_keys)`: - static `selection=` → REPLACE (a redeclaration drops earlier keys) - `selection_add=` → EXTEND (unions across classes) - provided-but-dynamic → POISON (sentinel `None`; never closed; sticky) A closed `ASSERT IN` is emitted only when the whole domain is statically known. Poison is recorded as `None` in the `selections` map and skipped at emission (`selection_skip_open_domain` stat). # #533 — mro.rs rustfmt `mro.rs` landed un-`cargo fmt`'d (codex flagged the diff). Reformatted; 7/7 mro tests still pass. # Tests python3 -m unittest tests.test_spo_enrich : 61/61 (was 57) + test_dynamic_base_plus_add_stays_poisoned (finding 1) + test_full_redeclaration_replaces_not_unions (finding 2) + test_open_domain_none_emits_nothing + rewrote extraction tests for the (dynamic, base, add) contract cargo test -p lance-graph-ontology --lib mro : 7/7 rustfmt --check mro.rs : clean EPIPHANIES: E-ODOO-SELECTION-REPLACE-EXTEND-POISON.
2 participants
Summary
Follow-up to #530 — the
selection_addfix for the codex P2 review landed on the branch after #530's merge snapshot was taken, so it's cherry-picked here.The finding (codex, valid)
Odoo addons extend an existing Selection field's value domain via
_inherit+fields.Selection(selection_add=[('reviewed','Reviewed')])without redeclaring the base list._extract_selection_valuesonly read the baseselection=arg, so the emittedselection_valueset was incomplete — a downstreamASSERT $value IN [...]would reject records in legal extension-added states.Fix
_extract_selection_valuesnow reads both the base list (positional arg 0 orselection=) andselection_add=, unioning base-first with order-preserving dedup (factored into_selection_tuple_keys)._scan_filebinding now merges keys per(model, field)instead of last-write-wins — the base declaration and theselection_addextension live in different classes, so the field's value set must accumulate across scans.Tests (53 → 57)
selection_addkwarg alone → added keysselection_add→ union, base firstselection_adddedups against basestatein the_nameclass +selection_add=[('reviewed', …)]in an_inheritclass → accumulated[draft, posted, reviewed]python3 -m unittest tests.test_spo_enrich— 57/57 OK.Pure Extracted-leg breadth fix; the Core-first framing from #530 (typed Core is the authoritative home for
selection_value) is unchanged.