fix: skip field types before getattr and isolate detail prefetch#129
fix: skip field types before getattr and isolate detail prefetch#129
Conversation
_serialize_detail called getattr() on all fields before checking if the field type should be skipped (many2many, one2many, binary). This triggered computed Many2many fields like household_member_ids, causing AccessError when the compute traverses res.partner records. Also isolate get_detail() prefetch set via with_prefetch() to prevent sudo-contamination from _ensure_detail() causing record-rule checks against the wrong user (public user instead of authenticated user). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical access and permission issues encountered during Change Request creation, particularly via FastAPI endpoints. It refines the serialization of detail fields by ensuring that certain field types are skipped before their values are accessed, preventing unintended side effects from computed fields. Additionally, it isolates the prefetch context for detail records to avoid incorrect record-rule checks caused by Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two important fixes related to change request serialization and data fetching. The first fix correctly defers getattr calls on certain field types, preventing an AccessError on computed fields. The second fix properly isolates the prefetch cache to avoid user context contamination. The changes are logical and well-implemented. I have one suggestion to improve maintainability by removing a duplicated constant.
| # Field types that are skipped entirely — check BEFORE reading the | ||
| # value so that computed fields of these types are never triggered. | ||
| _SKIP_FIELD_TYPES = {"many2many", "one2many", "binary"} |
There was a problem hiding this comment.
To improve maintainability and avoid code duplication, it's better to reuse the SKIP_FIELD_TYPES constant that is already defined in odoo.addons.spp_api_v2.services.schema_builder and used elsewhere in this file (in _validate_detail_input).
I suggest you remove this local definition and instead:
- Add
from odoo.addons.spp_api_v2.services.schema_builder import SKIP_FIELD_TYPESto the file's top-level imports. - Use
SKIP_FIELD_TYPESon line 215. - Remove the now-redundant local import from
_validate_detail_input.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #129 +/- ##
========================================
Coverage 69.96% 69.97%
========================================
Files 832 835 +3
Lines 48813 49138 +325
========================================
+ Hits 34154 34385 +231
- Misses 14659 14753 +94
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
_serialize_detail called getattr() on all fields before checking if the field type should be skipped (many2many, one2many, binary). This triggered computed Many2many fields like household_member_ids, causing AccessError when the compute traverses res.partner records.
Also isolate get_detail() prefetch set via with_prefetch() to prevent sudo-contamination from _ensure_detail() causing record-rule checks against the wrong user (public user instead of authenticated user).
Why is this change needed?
When creating a Change Request via the FastAPI endpoint (e.g.
add_member_4ps),_serialize_detail()triggersAccessError: Public user (id=3) doesn't have 'read' access to res.partner. This happens because:_serialize_detail()callsgetattr(detail, field_name)on every field before checking if the field type (many2many,one2many,binary) should be skipped. For non-stored computed Many2many fields likehousehold_member_ids,getattrtriggers the compute method, which traversesres.partnerrecords and fails with AccessError.get_detail()returns a record that shares the prefetch set with_ensure_detail()'ssudo()context. This causes Odoo's prefetch mechanism to trigger record-rule checks against the public user (uid=3, the FastAPI endpoint's base user) instead of the authenticated user.How was the change implemented?
spp_api_v2_change_request/services/change_request_service.py: Moved the field type check (many2many,one2many,binary) to run beforegetattr(). Previously these types were checked aftergetattr()had already triggered side effects. The skip set is defined once as_SKIP_FIELD_TYPESand checked in the loop before any field access.spp_change_request_v2/models/change_request.py:get_detail()now calls.with_prefetch()on the browsed record to isolate it from_ensure_detail()'s sudo prefetch set.New unit tests
None — this is a bugfix to existing behavior, not a new feature. Existing tests should continue to pass.
Unit tests executed by the author
How to test manually
user_id = base.public_user(the default)Change Requests / UsergroupaddMemberssection (typeadd_member_4ps)AccessError— Public user (id=3) doesn't have 'read' access to res.partnerAlso verify CR creation via the Odoo web UI still works normally.
Related links
N/A