[FIX] kw_api_custom_endpoint: apply the endpoint domain when change() resolves by id - #4
Open
brmk wants to merge 1 commit into
Open
Conversation
… resolves by id
The configured domain was applied on the list branch and ignored by the by-id
branches. change() resolved its target with a bare
m.search([(self.model_id_field, '=', obj_id)], limit=1)
and Odoo only skips the implicit active = True when a domain mentions active,
which that one does not. An archived record was therefore never found, write()
on the empty recordset was a silent no-op, and data_response serialised it as
{"content": [], "code": "200"}
so the caller was told a write succeeded that never happened. An endpoint
configured with [("active", "in", [True, False])] - which declares archived
records to be in scope - still could not write to them. The same gap let a POST
update a record the endpoint's domain was meant to exclude.
change() now resolves through api_get_obj_domain(), which narrows the id lookup
by the endpoint's own domain. No context is touched: a domain that mentions
active makes Odoo skip active_test by itself.
Behaviour change worth calling out: an id that resolves to nothing now answers
400: Wrong ID - the same shape response() already uses - instead of a 200 that
claims a write occurred.
Verified on Odoo 19.0 against a product.template endpoint:
domain [("active","in",[True,False])] active -> 200, record echoed, written
domain [("active","in",[True,False])] archived -> 200, record echoed, written
(was: 200, content: [], no write)
no domain active -> 200, record echoed, written
(unchanged)
no domain archived -> 400 Wrong ID
(was: 200, content: [], no write)
any unknown id -> 400 Wrong ID
Closes kitworks-systems#2
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
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.
Fixes #2.
The configured
domainis applied on the LIST branch and ignored by the by-id branches.change()resolved its target with a baresearch([(self.model_id_field, '=', obj_id)], limit=1), and Odoo only skips the implicitactive = Truewhen a domain mentionsactive. An archived record was therefore neverfound,
write()on the empty recordset was a silent no-op, anddata_responseserialisedit as
{"content": [], "code": "200"}— the caller told that a write succeeded when nothinghad been written.
So an endpoint configured with
[("active", "in", [True, False])], which declares archivedrecords to be in scope, still could not write to them. The same gap let a POST update a
record the endpoint's domain was meant to exclude.
change()now resolves through a smallapi_get_obj_domain()helper that narrows the idlookup by the endpoint's own domain — the same thing the list branch has always done.
No context is touched: a domain mentioning
activemakes Odoo skipactive_teston itsown.
Behaviour change
An id that resolves to nothing now answers
400: Wrong ID— the shaperesponse()alreadyuses — instead of a
200claiming a write happened. For an endpoint with no domain, anarchived id moves from a silent fake success to that honest 400.
Verified on Odoo 19.0
Against a
product.templateendpoint withnamechangeable:[("active","in",[True,False])][("active","in",[True,False])]content: [], not writtencontent: [], not writtencontent: []A note for reviewers
Our first attempt at this fixed it with
That put
active_test=Falseinto the whole request environment, andkw_api's translationhelper does
self.env['res.lang'].sudo().search([]).res.langis archivable, so it beganreturning inactive languages, and reading a translation for a language Odoo has not loaded
raised
KeyError: '<model>.<field>'from the ORM field cache — after a successful write,inside the response serialiser. Every write broke, not just archived ones. It reached our
production and was reverted 23 minutes later.
Applying the domain avoids that entire class of problem, which is why this PR takes that
route rather than the context one.
Left alone deliberately
response()(GET by id) anddelete()share the same bare search. I did not touch them:response()at least answers400honestly today, and widening whatdelete()can reachseems like a maintainer's call rather than a bug fix. Both are noted in #2.