feat(ctx): req_jsonをJsonValue契約に、req_json_object/req_json_tを新設#106
Conversation
req_jsonが生body文字列返却してたのを、宣言型どおり Result<Untrusted<JsonValue>,...>に修正。 object限定のreq_json_object、型検証つきreq_json_tを追加。 core/json.awkの3関数と対称なパターンで実装。 Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughctx の JSON 取得 API が拡張され、デコード結果を返す ChangesJSON取得APIと型付き展開
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: fe59c2143f
ℹ️ 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".
| for (key in out) { | ||
| if (!json::_validate_leaf(type, out[key], out_type[key])) | ||
| return awk::result_ng("JsonTypeError", "type mismatch: expected " type " at " key) | ||
| } |
There was a problem hiding this comment.
When the DSL lowers ctx.req.json<Array>() or ctx.req.json<JsonValue>() to this route, this loop validates every leaf with json::_validate_leaf, which returns false for container types. That makes valid array/object requests fail with JsonTypeError (for example [1] as Array), while empty arrays/objects skip the loop and incorrectly satisfy scalar types such as Int; this diverges from the existing json::decode_t root-kind and empty-container checks.
Useful? React with 👍 / 👎.
コンテナ型(Array/JsonObject等)指定時、全リーフをスカラー 検証していたため有効な配列/オブジェクトが誤ってJsonTypeError になっていた。json::decode_tと同じroot shape照合に統一し、 空コンテナとスカラー型の不一致も検出する。
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 234c41fb93
ℹ️ 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".
| if (ok == -1) return awk::result_ng("JsonTooDeepError", "max nesting depth exceeded") | ||
| if (ok == 0) { | ||
| msg = (awk::HAWK_JSON_ERROR != "") ? awk::HAWK_JSON_ERROR : "invalid JSON" | ||
| return awk::result_ng("JsonParseError", msg) |
There was a problem hiding this comment.
When ctx.req.json() is used through the DSL unwrap form (let body ?= ctx.req.json()), the generated fallback only maps ParseError to a 400 and otherwise returns 500 (dsl/desugar_let.awk currently special-cases only ParseError, AuthError, and NotFoundError). Returning JsonParseError here means malformed or empty JSON request bodies now fall through to a 500 instead of the client-error path, so either the unwrap mapping needs to understand the new JSON error types or this request helper needs to preserve the request parse error category expected by that flow.
Useful? React with 👍 / 👎.
| if (!json::_validate_leaf(type, out[key], out_type[key])) | ||
| return awk::result_ng("JsonTypeError", "type mismatch: expected " type " at " key) |
There was a problem hiding this comment.
For a typed request using a record name, such as the documented ctx.req.json<Todo>() shape with body {"title":"x"}, type is Todo; because only built-in container names are handled above, this falls into the leaf loop and _validate_leaf("Todo", ...) rejects every field with JsonTypeError. The new generic endpoint therefore rejects valid JSON for DSL record types unless record schemas are resolved here or record names are treated with the appropriate object/field validation.
Useful? React with 👍 / 👎.
| msg = (awk::HAWK_JSON_ERROR != "") ? awk::HAWK_JSON_ERROR : "invalid JSON" | ||
| return awk::result_ng("JsonParseError", msg) | ||
| } | ||
| return awk::result_ok_from_map(out, out_type) |
There was a problem hiding this comment.
This now returns the parsed body through result_ok_from_map(...), but the DSL success paths for when ctx.req.json() of ok body: and let body ?= ctx.req.json() still bind with result_val(...) in dsl/desugar_match.awk/dsl/desugar_let.awk. For any valid JSON object or array request, handlers receive the internal base64/record encoding rather than the parsed payload, so routes that forward or inspect body through normal DSL bindings will see framework serialization bytes unless the JSON result is materialized with result_val_into_map or a DSL-aware representation is returned.
Useful? React with 👍 / 👎.
| if (!has_leaf && type != "Any") | ||
| return awk::result_ng("JsonTypeError", "type mismatch: expected " type " but got empty container") | ||
| return awk::result_ok_from_map(out, out_type) |
There was a problem hiding this comment.
For ctx.req.json_t("Int"), a body like {"n":1,"bad":[]} sets has_leaf from n but the empty array emits no leaf to validate, so this condition is skipped and the function returns ok. Since the new typed request helper already rejects an empty array at the root for the same type, accepting the same invalid shape when it is nested lets malformed typed payloads through whenever another matching leaf is present.
Useful? React with 👍 / 👎.
ctx.req.json()/json.decode等はresult_ok_from_map形式でエンコードされるが、 ?=アンラップとwhenのok腕がresult_val（単一値デコード）で受けていたため、 成功時に内部base64/recordエンコーディングがそのまま変数へバインドされていた。 container型はmapとして展開、scalar leaf型は空キー抽出で対応する。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dsl/desugar_let.awk`:
- Around line 162-164: The _ds_is_json_container_type helper currently excludes
structured decode_t results, so json.decode_t and req.json_t values that return
List, Dict, or record types fall through the non-container path and only
populate map_var[""] instead of expanded fields. Update
_ds_is_json_container_type in desugar_let.awk to treat decode_t structural types
as containers, matching the out-map contract expected by result_val_into_map, so
those decoded values are handled through the container branch and their fields
are preserved.
In `@dsl/desugar_match.awk`:
- Line 409: The typed JSON branches in _ds_match_emit are inferring from the
original _DS_match_expr[d] surface form instead of the transformed expression,
which leaves _ds_infer_type unable to resolve Result-based json_t/decode_t
cases. Update the type inference path to run on the post-conversion expression
used for matching, so resolved is populated correctly and typed JSON
object/list/record values follow the structured matching path instead of falling
back to scalar handling. Use the existing _ds_match_emit flow and the
_DS_match_expr / _ds_infer_type symbols to locate the inference point.
In `@tests/unit/dsl/qeq_ctx_req_json_map/expected.awk`:
- Around line 3-9: The error-to-status mapping in the qeq_ctx_req_json_map
expectation only handles ParseError, AuthError, and NotFoundError, so
ctx::req_json* failures like JsonParseError and JsonTooDeepError fall through to
500. Update the dispatch logic in expected.awk around result_err_type(_ds_tc_1)
to recognize JsonParseError and JsonTooDeepError from
awk::result_err_type(_ds_tc_1) and map them to a 4xx response, matching the
existing 400 handling used for parse-related errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e61e3d9-2841-41f4-a5a7-6bdf62f350d5
📒 Files selected for processing (20)
dsl/desugar_let.awkdsl/desugar_match.awktests/unit/dsl/json_decode_t_nullable_unwrap/expected.awktests/unit/dsl/match_result_basic/expected.awktests/unit/dsl/match_result_default/expected.awktests/unit/dsl/option_unwrap_ok/expected.awktests/unit/dsl/qeq_ctx_req_json_map/expected.awktests/unit/dsl/qeq_ctx_req_json_map/input.awktests/unit/dsl/qeq_ctx_req_json_object_map/expected.awktests/unit/dsl/qeq_ctx_req_json_object_map/input.awktests/unit/dsl/qeq_non_json_result_scalar/expected.awktests/unit/dsl/qeq_non_json_result_scalar/input.awktests/unit/dsl/union_generic_top_level_split/expected.awktests/unit/dsl/untrusted_sig/expected.awktests/unit/dsl/unwrap_union_option_result_ok/expected.awktests/unit/dsl/when_ctx_req_json_map/expected.awktests/unit/dsl/when_ctx_req_json_map/input.awktests/unit/dsl/when_default_bind/expected.awktests/unit/dsl/when_typed_ng_multi/expected.awktests/unit/dsl/when_typed_ng_single/expected.awk
✅ Files skipped from review due to trivial changes (6)
- tests/unit/dsl/when_ctx_req_json_map/expected.awk
- tests/unit/dsl/qeq_ctx_req_json_object_map/input.awk
- tests/unit/dsl/when_ctx_req_json_map/input.awk
- tests/unit/dsl/unwrap_union_option_result_ok/expected.awk
- tests/unit/dsl/match_result_basic/expected.awk
- tests/unit/dsl/qeq_non_json_result_scalar/input.awk
| if (!result_ok(_ds_tc_1)) { | ||
| _ds_err_type__ds_tc_1 = awk::result_err_type(_ds_tc_1) | ||
| if (_ds_err_type__ds_tc_1 == "ParseError") return ctx::dispatch("res.status", 400) | ||
| if (_ds_err_type__ds_tc_1 == "AuthError") return ctx::dispatch("res.status", 401) | ||
| if (_ds_err_type__ds_tc_1 == "NotFoundError") return ctx::dispatch("res.status", 404) | ||
| return ctx::dispatch("res.status", 500) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the ng-branch (error dispatch table) is generated in desugar_let.awk
rg -n 'ParseError|AuthError|NotFoundError|JsonParseError|JsonTooDeepError' dsl/desugar_let.awkRepository: redpeacock78/H-awk
Length of output: 437
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests/unit/test_ctx.awk (relevant excerpts) ---'
grep -n -C 3 'req_json\|JsonParseError\|JsonTooDeepError\|result_err_type' tests/unit/test_ctx.awk || true
echo
echo '--- dsl/desugar_let.awk around the status table generator ---'
sed -n '150,195p' dsl/desugar_let.awk
echo
echo '--- any other JsonParseError/JsonTooDeepError references in repo ---'
rg -n 'JsonParseError|JsonTooDeepError' .Repository: redpeacock78/H-awk
Length of output: 13082
JsonParseError / JsonTooDeepError を 4xx にマップしてください
ctx::req_json* は JsonParseError / JsonTooDeepError を返すのに、この固定表では ParseError / AuthError / NotFoundError しか拾っていないため、不正な JSON ボディが res.status(500) に落ちます。JsonParseError と JsonTooDeepError も 400 系で扱う分岐を追加してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/dsl/qeq_ctx_req_json_map/expected.awk` around lines 3 - 9, The
error-to-status mapping in the qeq_ctx_req_json_map expectation only handles
ParseError, AuthError, and NotFoundError, so ctx::req_json* failures like
JsonParseError and JsonTooDeepError fall through to 500. Update the dispatch
logic in expected.awk around result_err_type(_ds_tc_1) to recognize
JsonParseError and JsonTooDeepError from awk::result_err_type(_ds_tc_1) and map
them to a 4xx response, matching the existing 400 handling used for
parse-related errors.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed6b355671
ℹ️ 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".
| _DS_body_buf[++_DS_body_count] = arr[1] arr[2] " = option_val(_ds_tc_" _DS_tc_count ")" | ||
| } else { | ||
| if (_ds_is_json_decode_rhs(surface_rhs)) { | ||
| resolved = _ds_strip_untrusted(_ds_inner_type(rhs_type)) |
There was a problem hiding this comment.
The fresh evidence is that ctx.req.json<Array>() now desugars to ctx::dispatch("req.json_t", "Array"), but dotted dispatch paths are not resolved when inferring rhs_type, so this assignment leaves resolved as T instead of Array/JsonObject. For let xs ?= ctx.req.json<Array>() (and the equivalent when path), the generated code therefore takes the scalar branch and assigns xs = _ds_tcm_1[""], replacing valid array/object request bodies with an empty scalar before handler code sees them.
Useful? React with 👍 / 👎.
| if (_ds_is_json_decode_rhs(surface_rhs)) { | ||
| resolved = _ds_strip_untrusted(_ds_inner_type(rhs_type)) | ||
| if (_ds_is_json_container_type(resolved)) { | ||
| _DS_let_locals[++_DS_let_count] = arr[2] "_types" |
There was a problem hiding this comment.
When a handler already has a local named after the ok binding plus _types, for example let body_types = "keep" followed by let body ?= ctx.req.json(), this synthetic local is emitted as another body_types parameter in the generated function. Gawk rejects duplicate function parameters before the app can run, so JSON unwrapping should use a reserved _ds_... temp or otherwise avoid colliding with user variables.
Useful? React with 👍 / 👎.
| _DS_body_buf[++_DS_body_count] = arr[1] arr[2] " = result_val(_ds_tc_" _DS_tc_count ")" | ||
| if (_ds_is_json_decode_rhs(surface_rhs)) { | ||
| if (_ds_is_json_container_type(resolved)) { | ||
| _DS_body_buf[++_DS_body_count] = arr[1] "result_val_into_map(_ds_tc_" _DS_tc_count ", " arr[2] ", " arr[2] "_types)" |
There was a problem hiding this comment.
For let body ?= ctx.req.json(); return ctx.res.json(body), this materializes values into body but leaves numeric/bool/null tags only in body_types, which normal response encoding never sees. Since json_encode_any treats unsuffixed map entries as strings, a valid request such as {"age":30,"ok":true} is echoed with string values rather than preserving the JSON leaf types.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function _ds_is_json_container_type(t) { | ||
| return (t == "JsonValue" || t == "JsonObject" || t == "Array" || t == "Map" || t == "JsonScalar") |
There was a problem hiding this comment.
Because JsonScalar is treated as a container here, let v ?= json.decode<JsonScalar>(s) emits result_val_into_map(..., v, v_types) instead of extracting the root "" value. When s is a valid scalar like 42, the handler receives v as an AWK array, so forwarding it through ctx.res.json(v) produces an object with an empty key rather than the scalar JSON value.
Useful? React with 👍 / 👎.
bot指摘5件対応: - JsonScalarをcontainer型判定から除外、scalar root検証のみ維持 - ctx::dispatch形式の型引数から静的型解決(dotted dispatch) - 合成ローカル変数名を_ds_tct_N/_ds_mct_Nにし衝突回避 - json_encode_any にtypesサイドカー追加、leaf型をエンコード時保持 - List</Dict</record型をcontainer判定に追加
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dsl/desugar_let.awk (1)
187-194: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winJSON デコードエラーを 400 系にマップしてください。
req.json*はJsonParseError/JsonTooDeepError/JsonTypeErrorを返すため、現在は JSON のクライアントエラーがすべて 500 に落ちます。修正案
- " if (" t " == \"ParseError\") return ctx::dispatch(\"res.status\", 400)\n" \ + " if (" t " == \"ParseError\" || " t " == \"JsonParseError\" || " t " == \"JsonTooDeepError\" || " t " == \"JsonTypeError\") return ctx::dispatch(\"res.status\", 400)\n" \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dsl/desugar_let.awk` around lines 187 - 194, _update _ds_result_ng_return so JSON-related result types from req.json* are mapped to 4xx instead of falling through to 500. Extend the existing error-type checks in _ds_result_ng_return to recognize JsonParseError, JsonTooDeepError, and JsonTypeError and dispatch res.status to an appropriate client error code, keeping the existing ParseError/AuthError/NotFoundError handling unchanged._
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/json.awk`:
- Around line 381-387: The `decode_t` container-type root validation only checks
`Array` and `JsonObject`/`Map`, so `List<...>`, `Dict<...>`, and record types
can skip the correct root-kind check and incorrectly succeed. Update the
`_is_container_type(type)` branch in `decode_t` to distinguish
`Array`/`List<...>` as array roots and `Dict<...>`/record types as object roots
before calling `result_ok_from_map`, using the existing `_jp_root_kind` checks
and the `JsonTypeError` failures already used in this block.
In `@dsl/desugar_let.awk`:
- Around line 17-20: Nested generic type arguments are being parsed incorrectly
in _ds_resolve_surface_generic because the current regex stops at the first >,
so cases like json.decode<Dict<Str,Int>>(...) and ctx.req.json<List<Int>>() fail
to resolve. Update _ds_resolve_surface_generic to extract the type argument
substring by tracking angle-bracket depth instead of relying on ([^>]+), then
pass the full nested generic text through to _ds_resolve_t_sig unchanged.
In `@dsl/desugar_match.awk`:
- Around line 453-454: The ok-binding type sidecar registered in
`_DS_JSON_TYPEVAR` for `match` handling is left behind after an arm finishes, so
later uses can resolve to the wrong `_ds_mct_*` value. Update the
`desugar_match.awk` logic around the `map_type_var` assignment in the `match`
arm flow to save the previous `_DS_JSON_TYPEVAR` entry and restore or clear it
when leaving the ok body scope, including the related cleanup path referenced by
`_DS_match_ok_var` and the later arm-processing block.
In `@tests/unit/dsl/qeq_json_scalar_decode_t/expected.awk`:
- Around line 10-12: The scalar JSON decode path is dropping the leaf type
sidecar, so the expected output for result_val_into_map should be updated to
pass the typed metadata through ctx::dispatch("res.json") instead of only
forwarding the raw value. Align this JsonScalar path with the typed flow used by
qeq_ctx_req_json_record_t and qeq_json_decode_t_list, ensuring the generated
code preserves string/number/bool/null distinctions during round-trip encoding.
Update the expected.awk fixture around result_val_into_map, the v assignment,
and the ctx::dispatch call to reflect the typed route.
---
Outside diff comments:
In `@dsl/desugar_let.awk`:
- Around line 187-194: _update _ds_result_ng_return so JSON-related result types
from req.json* are mapped to 4xx instead of falling through to 500. Extend the
existing error-type checks in _ds_result_ng_return to recognize JsonParseError,
JsonTooDeepError, and JsonTypeError and dispatch res.status to an appropriate
client error code, keeping the existing ParseError/AuthError/NotFoundError
handling unchanged._
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 182bfd98-67b4-4c7f-8ba1-ff9dfd0876b0
📒 Files selected for processing (33)
core/ctx.awkcore/json.awkcore/response.awkdsl/desugar.awkdsl/desugar_let.awkdsl/desugar_match.awktests/unit/dsl/match_result_basic/expected.awktests/unit/dsl/match_result_default/expected.awktests/unit/dsl/option_unwrap_ok/expected.awktests/unit/dsl/qeq_ctx_req_json_array_t/expected.awktests/unit/dsl/qeq_ctx_req_json_array_t/input.awktests/unit/dsl/qeq_ctx_req_json_collision/expected.awktests/unit/dsl/qeq_ctx_req_json_collision/input.awktests/unit/dsl/qeq_ctx_req_json_map/expected.awktests/unit/dsl/qeq_ctx_req_json_object_map/expected.awktests/unit/dsl/qeq_ctx_req_json_record_t/expected.awktests/unit/dsl/qeq_ctx_req_json_record_t/input.awktests/unit/dsl/qeq_json_decode_t_list/expected.awktests/unit/dsl/qeq_json_decode_t_list/input.awktests/unit/dsl/qeq_json_scalar_decode_t/expected.awktests/unit/dsl/qeq_json_scalar_decode_t/input.awktests/unit/dsl/union_generic_top_level_split/expected.awktests/unit/dsl/untrusted_sig/expected.awktests/unit/dsl/unwrap_union_option_result_ok/expected.awktests/unit/dsl/when_ctx_req_json_array_t/expected.awktests/unit/dsl/when_ctx_req_json_array_t/input.awktests/unit/dsl/when_ctx_req_json_map/expected.awktests/unit/dsl/when_default_bind/expected.awktests/unit/dsl/when_typed_ng_multi/expected.awktests/unit/dsl/when_typed_ng_single/expected.awktests/unit/run.awktests/unit/test_json.awktests/unit/test_response.awk
✅ Files skipped from review due to trivial changes (5)
- tests/unit/dsl/qeq_json_scalar_decode_t/input.awk
- tests/unit/dsl/qeq_ctx_req_json_array_t/expected.awk
- tests/unit/dsl/when_ctx_req_json_array_t/expected.awk
- tests/unit/dsl/when_typed_ng_multi/expected.awk
- tests/unit/run.awk
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/unit/dsl/untrusted_sig/expected.awk
- tests/unit/dsl/union_generic_top_level_split/expected.awk
- tests/unit/dsl/when_default_bind/expected.awk
- tests/unit/dsl/match_result_basic/expected.awk
- tests/unit/dsl/unwrap_union_option_result_ok/expected.awk
- tests/unit/dsl/when_ctx_req_json_map/expected.awk
- tests/unit/dsl/match_result_default/expected.awk
- tests/unit/dsl/when_typed_ng_single/expected.awk
- tests/unit/dsl/qeq_ctx_req_json_map/expected.awk
- tests/unit/dsl/option_unwrap_ok/expected.awk
- core/ctx.awk
There was a problem hiding this comment.
💡 Codex Review
Line 73 in a001aca
Keep typed request JSON untrusted
ctx.req.json_t is still parsing attacker-controlled request bodies, but this signature marks the ok value as trusted T instead of Untrusted<T>. For example, let raw: Str ?= ctx.req.json<Str>() then passes raw to safe.html.raw(raw) without the existing Untrusted rejection that ctx.req.body()/ctx.req.json() get, allowing typed JSON strings to bypass the sanitizer/type barrier.
ℹ️ 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".
| t ~ /^List</ || t ~ /^Dict</ || \ | ||
| (t !~ /^(JsonScalar|Int|Float|Str|Bool|Null|Any)$/ && t ~ /^[A-Z][A-Za-z0-9_]*$/) |
There was a problem hiding this comment.
Fresh evidence: this predicate now marks List<...>, Dict<...>, and capitalized record names as container types, but the decode_t/req_json_t container branch only checks root kind for the exact Array, JsonObject, and Map names before returning ok. As a result json.decode_t("List<Int>", "42"), json.decode_t("List<Int>", "[\"s\"]"), or ctx.req.json<Todo>() on a scalar/malformed object succeeds as the requested type instead of JsonTypeError, so typed JSON payloads can bypass validation.
Useful? React with 👍 / 👎.
| function json(res, data, types, code, body, validated, status_code) { | ||
| if (typeof(types) == "array") { | ||
| status_code = code | ||
| body = json_encode_any(data, types) |
There was a problem hiding this comment.
When this new overload is used by the DSL rewrites for let body ?= ctx.req.json(); return ctx.res.json(body), data is the flat map returned by result_val_into_map (a.b and x.0 keys) and types only stores leaf tags. Passing both directly to json_encode_any preserves scalar types but not structure, so a valid nested body like {"a":{"b":1},"x":[true]} is emitted as {"x.0":true,"a.b":1}, and untyped root arrays become objects instead of arrays.
Useful? React with 👍 / 👎.
| _DS_body_buf[++_DS_body_count] = arr[1] "result_val_into_map(_ds_tc_" _DS_tc_count ", " map_var ", " map_type_var ")" | ||
| _DS_body_buf[++_DS_body_count] = arr[1] arr[2] " = " map_var "[\"\"]" |
There was a problem hiding this comment.
In the scalar branch for JSON decode/request unwraps, this extracts only map_var[""] and discards map_type_var[""]. For let v: JsonScalar ?= json.decode<JsonScalar>(s) with s as true, null, or 42, v becomes an untagged AWK string; a later ctx.res.json(v) encodes it as "true"/"42" rather than the original JSON scalar.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function _ds_resolve_surface_generic(expr, m) { | ||
| if (!match(expr, /^((ctx\.)?[a-z][a-zA-Z0-9_]*(\.[a-z][a-zA-Z0-9_]*)*)<([^>]+)>[[:space:]]*\(/, m)) |
There was a problem hiding this comment.
For the documented generic form with parameterized types, such as ctx.req.json<List<Int>>() or json.decode<List<Int>>(s), this matcher stops at the first > and never resolves the Result<T,...> return type. The ?= path then reports cannot infer type for ?= RHS, so callers must avoid the new generic syntax and hand-write the _t dispatch/string form for any List<...> or Dict<...> JSON contract.
Useful? React with 👍 / 👎.
| } | ||
| if (match(dot_transformed, /^([[:space:]]*)return[[:space:]]+ctx::dispatch\("res\.json",[[:space:]]*([a-zA-Z_][a-zA-Z0-9_]*)\)[[:space:]]*$/, _json_m) && \ | ||
| ((_DS_func_name, _json_m[2]) in _DS_JSON_TYPEVAR)) { | ||
| dot_transformed = _json_m[1] "return json(res, " _json_m[2] ", " _DS_JSON_TYPEVAR[_DS_func_name, _json_m[2]] ")" |
There was a problem hiding this comment.
When a decoded JSON map is changed before returning it, this rewrite still passes the original type sidecar to the response encoder. For example, after let body ?= ctx.req.json() parses {"age":30}, a later body["age"] = "abc" is emitted through json(res, body, _ds_tct_1), so the stale int tag coerces the new string to 0 instead of serializing the value the handler assigned.
Useful? React with 👍 / 👎.
| function _ds_resolve_t_in_ret(transformed_expr, _dm) { | ||
| if (!match(transformed_expr, /^([a-zA-Z_][a-zA-Z0-9_]*)::dispatch\("([^"]+)"[[:space:]]*,[[:space:]]*"([^"]+)"/, _dm)) | ||
| return "" | ||
| return _ds_resolve_t_sig(_dm[1] "." _dm[2], _dm[3]) |
There was a problem hiding this comment.
When this resolver handles string-form typed JSON maps such as let d: Dict<Str,Int> ?= json.decode_t("Dict<Str,Int>", s), it substitutes Dict<Str,Int> into the Result<T,...> signature. The unwrap/type-check path then asks for the Result inner type, which stops at the first comma and compares the annotation with Dict<Str, so valid Dict<...> JSON contracts fail desugaring before runtime.
Useful? React with 👍 / 👎.
coderabbitai指摘4件対応: - decode_t/req_json_t のcontainer root検証をList<>/Dict<>/record型に分離対応 - サーフェスジェネリック型引数解決をネスト<>対応(depthカウント方式)に書換 - desugar_match の _DS_JSON_TYPEVAR を arm処理後に復元/削除しスコープ汚染回避 - scalar decode(?=)でもJSON type sidecar登録し leaf型round-tripを維持
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d491db7f6
ℹ️ 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".
| dot_r = _ds_dot_transform(pipe_r) | ||
| if (match(dot_r, /^([[:space:]]*)return[[:space:]]+ctx::dispatch\("res\.json",[[:space:]]*([a-zA-Z_][a-zA-Z0-9_]*)\)[[:space:]]*$/, _json_m) && \ | ||
| ((_DS_func_name, _json_m[2]) in _DS_JSON_TYPEVAR)) { | ||
| dot_r = _json_m[1] "return json(res, " _json_m[2] ", " _DS_JSON_TYPEVAR[_DS_func_name, _json_m[2]] ")" |
There was a problem hiding this comment.
When a handler declares -> Response, a when ctx.req.json() of ok body: arm that returns ctx.res.json(body) is rewritten here to return json(res, body, _ds_mct_1) before _ds_check_return runs immediately below. _ds_infer_type has no signature for the bare json(...) helper, so desugaring reports unknown function json for otherwise valid annotated handlers that echo parsed JSON from a when arm.
Useful? React with 👍 / 👎.
| if (!_ds_is_json_container_type(resolved)) { | ||
| map_var = "_ds_mcm_" _DS_mc_count | ||
| map_type_var = "_ds_mcmt_" _DS_mc_count | ||
| } else { |
There was a problem hiding this comment.
For scalar JSON results such as when json.decode<Int>(s) of ok v: or JsonScalar, this branch creates map_type_var but never associates it with the ok binding in _DS_JSON_TYPEVAR. A subsequent return ctx.res.json(v) in the same arm is left as the normal one-argument response path, so decoded 42, true, or null are emitted as strings instead of their original JSON scalar types.
Useful? React with 👍 / 👎.
| return _json_encode_object(val, types, prefix) | ||
| } | ||
| hint = (typeof(types) == "array" && (prefix in types)) ? types[prefix] : "" | ||
| if (hint != "") return _json_value(val, hint) |
There was a problem hiding this comment.
Decoded numbers with a decimal or exponent are tagged as float, and the new sidecar response path feeds those tags through this hint branch. _json_value only treats int/number as numeric, so let body ?= ctx.req.json(); return ctx.res.json(body) changes a body like {"price":1.5} into {"price":"1.5"}; scalar json.decode<Float> responses have the same issue.
Useful? React with 👍 / 👎.
| if (t == "array") { | ||
| if (val["__json_type"] == "array") return _json_encode_array(val) | ||
| return _json_encode_object(val) | ||
| if (val["__json_type"] == "array") return _json_encode_array(val, types, prefix) |
There was a problem hiding this comment.
Now that parsed request JSON can be materialized and echoed through this encoder, user object keys are interpreted as the internal array sentinel. For let body ?= ctx.req.json(); return ctx.res.json(body), a valid body like {"__json_type":"array","0":"x"} reaches this check and is emitted as ["x"] instead of preserving the object, so a client-controlled field can change the response shape.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function _ds_is_json_container_type(t) { | ||
| return (t == "JsonValue" || t == "JsonObject" || t == "Array" || t == "Map" || \ |
There was a problem hiding this comment.
JsonValue can be a scalar, but classifying it as a container makes untyped ctx.req.json()/json.decode() unwraps always bind via result_val_into_map. When the payload is a scalar such as 42, let body ?= ctx.req.json(); return ctx.res.json(body) treats body as an AWK map and emits an object with the empty key instead of the scalar value, so this branch needs to account for the decoded root kind rather than the static JsonValue name.
Useful? React with 👍 / 👎.
1 participant
Summary
Test plan
Summary by CodeRabbit
req.json_objectとreq.json_tを追加し、取得時の型検証に対応。