[FIX] endpoint, endpoint_json2: serialize response payloads with json_default - #34
Conversation
Odoo's fields.Domain (e.g. from fields.Domain.AND) is not JSON serializable. A field whose compute returns such an object (a common pattern for view-only domain widgets, as added by partner_contact_address_default in this PR) crashed the JSON2 response when included in the payload. Mirrors the same fix submitted upstream at OCA/web-api#135: OCA/web-api#135
_make_json_response called json.dumps() with no default= hook, so any payload value json cannot represent natively raised TypeError and the request returned a 500. Every other JSON response in Odoo goes through json.dumps(data, default=json_default) (odoo/http.py). Pass the same hook here. It covers date, datetime, bytes, Domain, lazy and ReadonlyDict, and json.dumps applies it at every depth, so nested values are handled without walking the payload. Also let an endpoint supply its own hook through the result dict, for exec modes that need to override how a type is rendered; whatever it does not handle falls back to Odoo's default. Assisted-by: Claude Opus 5
_json2_serialize_values walked the whole payload to convert values json cannot represent, reimplementing odoo.tools.json.json_default without knowing it existed. It also converted containers without their contents, so 7edaf54 turned a Domain into its list form but left a date inside one of its conditions untouched. Drop the walker and pass an encoder hook to the controller instead. json.dumps applies it at every depth, so nested values are covered for free, and date, bytes and Domain need no handling of our own. Only datetime keeps a deviation: json_default renders naive UTC, which cannot express json2_tz, so datetimes stay ISO-8601 with an explicit offset. Note bytes now decode strictly -- Binary fields read back base64, so this is exact for real field values, where errors="replace" would have silently mangled anything else. Assisted-by: Claude Opus 5
yostashiro
changed the title
Nothing asserted that a hook supplied through the result dict reaches json.dumps, nor that _handle_exec__json2 hands its own hook over: the json2 tests called the hook directly, so a response could fall back to the default encoder with the suite still green. Add a controller-level test for both branches of the hook lookup, and one assertion on what _handle_exec__json2 returns. Drop the serialization tests that now only exercise odoo.tools.json.json_default, keeping the Domain case as a single assertion whose date inside a condition still covers conversion at depth. Assisted-by: Claude Opus 5
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 1d937d04ba
ℹ️ 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".
| default = kw.get("json_default") or json_default | ||
| data = json.dumps(payload, default=default) |
There was a problem hiding this comment.
When an endpoint's custom hook handles only an additional type—for example, Decimal—and the same payload contains an Odoo-supported value such as a date or Domain, json.dumps invokes only the custom hook. If that hook follows the normal protocol and raises TypeError for values it does not handle, the request returns a serialization error instead of falling back to Odoo's encoder as promised. Selecting one callable with or does not chain them; wrap the custom hook so unhandled values delegate to json_default, or require the hook itself to delegate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in a5193bf. The behaviour is intentional: json.dumps takes a single default= callable, and chaining would mean catching TypeError around the hook, which would also swallow genuine errors raised inside it. The contract is that a custom hook replaces the default for the whole payload and delegates to json_default for the types it does not render itself — which is what endpoint_json2._json2_json_default does. The comment (and the PR description) claimed automatic fallback and has been corrected.
The comment read as if the controller chained a custom hook with Odoo's encoder, but json.dumps takes a single default= callable: a hook that raises TypeError for a type it does not know would return a serialization error rather than fall back. Say instead that the hook replaces the default and is expected to delegate to json_default itself, which is what endpoint_json2 does. Assisted-by: Claude Opus 5
yostashiro
left a comment
yostashiro
left a comment
There was a problem hiding this comment.
@smorita7749 Thanks for the fix attempt!
A JSON2 response crashes whenever the payload holds a value
jsoncannot represent natively —fields.Domain(from a field computed withfields.Domain.AND, a common pattern for view-only domain widgets), but alsodate,datetimeandbytes.The root cause is in
endpoint, notendpoint_json2:_make_json_responsecallsjson.dumps()with nodefault=hook, while every other JSON response in Odoo goes throughjson.dumps(data, default=json_default)(odoo/http.py).endpoint_json2had grown its own payload walker to compensate, reimplementingodoo.tools.json.json_defaultwithout knowing it existed — and converting containers without their contents, so aDomainbecame a list while adateinside one of its conditions stayed adate.endpoint: passjson_defaulttojson.dumps.json.dumpsapplies it at every depth, so nested values are handled without walking the payload. An endpoint can also supply its own hook through the result dict; it replaces the default for the whole payload and is expected to delegate tojson_defaultfor the types it does not render itself.endpoint_json2: drop the walker and pass such a hook.date,bytesandDomainthen need no handling of our own.Only
datetimekeeps a deviation:json_defaultrenders naive UTC, which cannot expressjson2_tz, so datetimes stay ISO-8601 with an explicit offset.One behaviour change worth noting:
bytesnow decode strictly. Binary fields read back base64, so this is exact for real field values, where the previouserrors="replace"would have silently mangled anything else.Upstream: supersedes the
endpoint_json2-only fix in OCA/web-api#135. To be relayed as two PRs, one per module.Assisted-by: Claude Opus 5