feat: Arena allocation + Gateway Federation#39
Merged
Conversation
Arena 分配 + Gateway Federation 实现 Arena: - WriteLuxo 前缀 totalStringLen varint，ReadLuxo 一次 make 共享所有 string - unsafe.String 零拷贝，5 alloc → 1 alloc，35% faster - MaxArenaSize 64MB 防 OOM，越界自动 fallback - @transform/@mask/@visible 字段排除 arena - 4 SDK skipArenaHeader 适配（TS/Dart/Kotlin/Swift） Federation: - schema.Field 加 Module/ForeignKey，codegen 标记 extend 来源 - Query Planner: field mask 按模块拆分，无 extend 零退化 - svc:resolve:{Model}:{FK} 端点: WHERE fk IN (keys) + 按 FK 分组 + 长度前缀 item - Response Merger: 单条零拷贝合并 + 列式 blob 列追加 - Gateway proxyHandler: Plan → CallWithMask → 并行 resolve → Merge - 同端点去重: 相同 svc:resolve 只调一次 RPC，结果共享 - 单 extend 直接调用，多 extend goroutine 并行 - ExtractID/ExtractIDColumn 零分配 skip - RPC server 数组参数累积（SetParamSlot 重复 field ID → []int64） - batchLoad 注册 API ID + Registry（修复 cluster 模式 RPC 路由） - primary RPC 传 field mask（减少带宽）
移除意外提交的构建产物（dist/、.build/）
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds arena-prefixed string encoding and cross-module federation: codec arena APIs and skip methods, codegen changes to emit/handle arena headers, gateway planning and resolve flow, response merging, columnar skip/read helpers, schema federation metadata, request-slot accumulation, and extensive tests. ChangesCross-Module Federation with Arena Encoding
Sequence Diagram(s): skipped (changes are large and cross-cutting; sequence is covered in hidden artifact). Estimated Code Review Effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
pkg/codegen/handler.go (1)
2143-2154: 💤 Low valueConsider hoisting buffer allocation outside the key loop.
The pooled buffer
tmpis acquired and released on each iteration of the key loop. Moving it outside would reduce pool contention when resolving many keys.♻️ Suggested optimization
fmt.Fprintf(b, "\t\t// Write grouped response (key order matches request order)\n") fmt.Fprintf(b, "\t\treq.Buf.B = codec.AppendVarint(req.Buf.B, uint64(len(keys)))\n") + fmt.Fprintf(b, "\t\ttmp := api.GetBuf()\n") fmt.Fprintf(b, "\t\tfor _, key := range keys {\n") fmt.Fprintf(b, "\t\t\titems := grouped[key]\n") fmt.Fprintf(b, "\t\t\treq.Buf.B = codec.AppendVarint(req.Buf.B, uint64(len(items)))\n") - fmt.Fprintf(b, "\t\t\ttmp := api.GetBuf()\n") fmt.Fprintf(b, "\t\t\tfor _, item := range items {\n") fmt.Fprintf(b, "\t\t\t\t// Length-prefix: write to pooled buf, then [len][data]\n") fmt.Fprintf(b, "\t\t\t\ttmp.Reset()\n") fmt.Fprintf(b, "\t\t\t\titem.WriteLuxo(tmp, req.FieldMask)\n") fmt.Fprintf(b, "\t\t\t\treq.Buf.B = codec.AppendBytes(req.Buf.B, tmp.B)\n") fmt.Fprintf(b, "\t\t\t}\n") - fmt.Fprintf(b, "\t\t\tapi.PutBuf(tmp)\n") fmt.Fprintf(b, "\t\t}\n") + fmt.Fprintf(b, "\t\tapi.PutBuf(tmp)\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 `@pkg/codegen/handler.go` around lines 2143 - 2154, The pooled buffer tmp is acquired/released inside the keys loop causing extra pool churn; hoist allocation by calling tmp := api.GetBuf() before the for _, key := range keys loop and move the corresponding api.PutBuf(tmp) after that loop, ensure tmp.Reset() is still called per-item before item.WriteLuxo(tmp, req.FieldMask) and keep using req.Buf.B = codec.AppendBytes(req.Buf.B, tmp.B) so semantics of grouped, items, req.Buf, and field masking remain 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 `@pkg/codegen/dataloader.go`:
- Around line 679-682: The generated code builds svcName and calls
getAPIID(svcName) but doesn't guard against getAPIID returning 0, causing
client.Call to target an invalid endpoint; update the block around
svcName/getAPIID to check if apiID == 0 and return a clear error (e.g.,
fmt.Errorf with svcName and modelName) before attempting client.Call so callers
fail fast and surface the missing `svc:batchLoad:*` API mapping.
In `@pkg/codegen/entry.go`:
- Around line 666-667: The generated resolve calls currently pass req.FieldMask
to CallWithMask (e.g., cl.CallWithMask(resolveAPI.ID, req.FieldMask,
resolveKeys(ids))); change those to use the per-extend step mask instead of the
original request mask — i.e., pass the mask attached to the current extend step
(the variable representing the extend like extend.Mask / step.FieldMask /
extendMask used in the generator) to CallWithMask along with resolveAPI.ID and
resolveKeys(ids). Update the same pattern at the other occurrences referenced
(around the blocks that generate lines 716-717 and 734-735) so every
svc:resolve:* call uses its own extend-specific mask rather than req.FieldMask.
- Around line 197-201: The federation resolver registration is currently inside
the conditional gated by anyServiceFns which prevents registering svc:resolve:*
endpoints when there are no user service functions; move the fmt.Fprintf call
that emits "%s_luxo.RegisterFederationResolvers(gw.Router, %sApp)" out of the
anyServiceFns block so that for each module where m.hasCrud is true the
registration is always emitted; update the loop over modules (the code that
iterates "for _, m := range modules") to call the RegisterFederationResolvers
line unconditionally when m.hasCrud is true, leaving other
anyServiceFns-dependent emissions unchanged.
In `@pkg/codegen/schema_gen.go`:
- Around line 399-414: inferForeignKey currently returns "id" for all non-list
relations which breaks hasOne/extend federation; update the non-list branch to
infer the FK from the related type name instead of hardcoding "id". Inside
inferForeignKey (params m *ast.ModelDecl, f *ast.FieldDecl), keep the existing
`@by` handling, then: if f.Type.IsList return str.LowerFirst(m.Name)+"Id"
(existing), else when f.Type != nil compute the target model name from f.Type
(use the field's referenced type/name on the type node) and return
str.LowerFirst(<targetName>)+"Id"; only fall back to "id" if f.Type is nil.
Ensure you reference the same helpers used elsewhere (e.g., findDirective,
extractByArgs) and use str.LowerFirst for casing.
In `@pkg/lux/api/request_test.go`:
- Around line 474-532: Add tests covering the single-element-array regression:
after calling Request.SetBinaryParams with a repeated slot and writing exactly
one value via Request.SetParamSlot, assert that Request.ParamIntArray (for
integer slots) and Request.ParamStringArray (for string slots) return a
one-element slice (len == 1) containing the value rather than a scalar or nil.
Update or add test cases referencing Request, SetBinaryParams, SetParamSlot,
ParamIntArray and ParamStringArray to perform a single SetParamSlot call and
then assert the returned slice length and contents.
In `@pkg/lux/api/request.go`:
- Around line 407-441: SetParamSlot currently stores the first value as a scalar
which breaks ParamIntArray/ParamStringArray (they expect slices) and doesn't
consistently merge scalar+slice or slice+slice; update SetParamSlot to normalize
and merge types so that int64 and []int64 always become a []int64 (handling
scalar -> []{scalar}, [] + scalar, scalar + [] and [] + [] merges) and likewise
string and []string become []string; keep the current overwrite behavior for
type mismatches or unsupported types, preserve the bounds check, and update
logic branches that reference r.paramSlots to convert single-item scalars into
single-element slices when storing so ParamIntArray/ParamStringArray receive
slices consistently.
In `@pkg/lux/codec/codec_test.go`:
- Around line 1486-1490: The test's pointer comparisons use &[]byte(s1)[0] and
&[]byte(s2)[0], which are invalid because converting a string to []byte copies
data; replace these with unsafe.StringData(s1) and unsafe.StringData(s2) so the
test compares the string's actual backing pointer to &arena[0] and &arena[5].
Add/import "unsafe" if missing and keep the comparisons in the same test
(referencing s1, s2 and arena) and assert inequality/equality the same way as
before.
In `@pkg/lux/codec/decoder.go`:
- Around line 288-291: The cast of ReadVarint's uint64 result (v) to int happens
before enforcing MaxArenaSize, allowing large uint64 values to overflow to
negative ints and bypass the cap; change the logic in decoder.go (around the
ReadVarint handling that now does size := int(v) and the following MaxArenaSize
check) so you first validate v as a uint64 against the cap and platform int
limits (e.g., if v > uint64(MaxArenaSize) || v > uint64(math.MaxInt) then set
d.err with the same error message and return 0), and only after those checks
convert v to int and proceed (preserving d.off usage in the error message).
In `@pkg/lux/luvia/merger.go`:
- Around line 43-50: The current loop appends each extend as [fieldID
varint][value data] but for FieldModel-encoded values the wire format requires a
length-prefixed value; update the extends append logic (the loop iterating over
extends in merger.go) to write [fieldID varint][length varint][value data] by
computing len(ext.Data) and calling codec.AppendVarint for that length before
appending ext.Data; apply the same change to the other occurrence referenced
(the similar block at lines ~268-278) so single-item extends keep FieldModel
framing.
In `@pkg/lux/luvia/planner.go`:
- Around line 64-113: The planner currently builds extend steps even when the
model lacks an "id" field (idFieldID == 0), which makes extend resolution
impossible; update the planning logic (around idFieldID, extends, primaryMask
generation and where ExtendStep items are appended) to guard against this by
early-returning or skipping extend-planning when idFieldID == 0 — i.e., after
computing idFieldID and before adding extend steps, if idFieldID == 0 then do
not create extends (return nil or leave extends empty) so no federation plan
with IDFieldID==0 is produced; ensure existing use of codec.FieldMaskSet and the
auto-include idFieldID block only runs when idFieldID > 0.
---
Nitpick comments:
In `@pkg/codegen/handler.go`:
- Around line 2143-2154: The pooled buffer tmp is acquired/released inside the
keys loop causing extra pool churn; hoist allocation by calling tmp :=
api.GetBuf() before the for _, key := range keys loop and move the corresponding
api.PutBuf(tmp) after that loop, ensure tmp.Reset() is still called per-item
before item.WriteLuxo(tmp, req.FieldMask) and keep using req.Buf.B =
codec.AppendBytes(req.Buf.B, tmp.B) so semantics of grouped, items, req.Buf, and
field masking remain 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8087fce-bc59-4dd9-b79d-a7a6b8e32f94
📒 Files selected for processing (29)
cmd/luxo/call.gopackages/client/src/codec.tspackages/dart/lib/src/codec.dartpackages/dart/lib/src/codegen.dartpackages/kotlin/src/main/kotlin/com/luxo/client/Codec.ktpackages/kotlin/src/main/kotlin/com/luxo/client/Codegen.ktpackages/vite-plugin/src/codegen.tspkg/codegen/dataloader.gopkg/codegen/entry.gopkg/codegen/handler.gopkg/codegen/schema_gen.gopkg/codegen/schema_gen_test.gopkg/codegen/writejson.gopkg/codegen/writejson_test.gopkg/lux/api/request.gopkg/lux/api/request_test.gopkg/lux/api/router_test.gopkg/lux/api/websocket_test.gopkg/lux/codec/codec_test.gopkg/lux/codec/columnar.gopkg/lux/codec/decoder.gopkg/lux/luvia/merger.gopkg/lux/luvia/merger_test.gopkg/lux/luvia/planner.gopkg/lux/luvia/planner_test.gopkg/lux/schema/convert.gopkg/lux/schema/convert_nested_test.gopkg/lux/schema/convert_test.gopkg/lux/schema/schema.go
Comment on lines
+679
to
682
| svcName := "svc:batchLoad:" + modelName | ||
| apiID := getAPIID(svcName) | ||
| fmt.Fprintf(b, "\t\t\tresp, err := client.Call(%d, enc.Bytes())\n", apiID) | ||
| fmt.Fprintf(b, "\t\t\tif err != nil {\n\t\t\t\treturn nil, err\n\t\t\t}\n") |
There was a problem hiding this comment.
Fail fast when svc:batchLoad:* API ID is missing.
If getAPIID returns 0, the RPC call may silently hit an invalid endpoint. Add a guard and return a clear error.
Proposed fix
svcName := "svc:batchLoad:" + modelName
apiID := getAPIID(svcName)
+ if apiID == 0 {
+ return nil, fmt.Errorf("missing API ID for %s", svcName)
+ }
resp, err := client.Call(%d, enc.Bytes())🤖 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 `@pkg/codegen/dataloader.go` around lines 679 - 682, The generated code builds
svcName and calls getAPIID(svcName) but doesn't guard against getAPIID returning
0, causing client.Call to target an invalid endpoint; update the block around
svcName/getAPIID to check if apiID == 0 and return a clear error (e.g.,
fmt.Errorf with svcName and modelName) before attempting client.Call so callers
fail fast and surface the missing `svc:batchLoad:*` API mapping.
Comment on lines
+666
to
+667
| b.WriteString("\t\t\t\textResp, err := cl.CallWithMask(resolveAPI.ID, req.FieldMask, resolveKeys(ids))\n") | ||
| b.WriteString("\t\t\t\tif err != nil { return luvia.ExtendColumnResult{} }\n") |
There was a problem hiding this comment.
Resolve calls should use per-extend masks, not the original request mask.
Using req.FieldMask for svc:resolve:* can over/under-request fields versus planner output. Use the mask attached to each extend step.
💡 Suggested fix
- extResp, err := cl.CallWithMask(resolveAPI.ID, req.FieldMask, resolveKeys(ids))
+ extResp, err := cl.CallWithMask(resolveAPI.ID, ext.Mask, resolveKeys(ids))
- r, e := cl.CallWithMask(resolveAPI.ID, req.FieldMask, resolveKeys([]int64{primaryID}))
+ r, e := cl.CallWithMask(resolveAPI.ID, ext.Mask, resolveKeys([]int64{primaryID}))
- r, e := cl.CallWithMask(apiID, req.FieldMask, resolveKeys([]int64{primaryID}))
+ r, e := cl.CallWithMask(apiID, ext.Mask, resolveKeys([]int64{primaryID}))Also applies to: 716-717, 734-735
🤖 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 `@pkg/codegen/entry.go` around lines 666 - 667, The generated resolve calls
currently pass req.FieldMask to CallWithMask (e.g.,
cl.CallWithMask(resolveAPI.ID, req.FieldMask, resolveKeys(ids))); change those
to use the per-extend step mask instead of the original request mask — i.e.,
pass the mask attached to the current extend step (the variable representing the
extend like extend.Mask / step.FieldMask / extendMask used in the generator) to
CallWithMask along with resolveAPI.ID and resolveKeys(ids). Update the same
pattern at the other occurrences referenced (around the blocks that generate
lines 716-717 and 734-735) so every svc:resolve:* call uses its own
extend-specific mask rather than req.FieldMask.
Comment on lines
+399
to
+414
| // inferForeignKey determines the FK field name for a relation field. | ||
| // For hasMany: remoteKey = lowerFirst(modelName) + "Id" (e.g., User → "userId") | ||
| // For belongsTo: localKey = lowerFirst(targetName) + "Id" (e.g., Post → "postId") | ||
| func inferForeignKey(m *ast.ModelDecl, f *ast.FieldDecl, enums map[string]bool) string { | ||
| // Check explicit @by directive | ||
| byDir := findDirective(f.Directives, "by") | ||
| if byDir != nil { | ||
| remoteKey, _ := extractByArgs(byDir) | ||
| return remoteKey | ||
| } | ||
| // Auto-infer: hasMany → "{modelName}Id", belongsTo → "id" | ||
| if f.Type != nil && f.Type.IsList { | ||
| return str.LowerFirst(m.Name) + "Id" | ||
| } | ||
| return "id" | ||
| } |
There was a problem hiding this comment.
Fix non-list FK inference for extend relations.
inferForeignKey returns "id" for all non-list relations, which breaks hasOne federation (should resolve by {parentModel}Id, e.g. userId). This can produce incorrect svc:resolve:*:* API names and runtime misses.
Proposed fix
func inferForeignKey(m *ast.ModelDecl, f *ast.FieldDecl, enums map[string]bool) string {
// Check explicit `@by` directive
byDir := findDirective(f.Directives, "by")
if byDir != nil {
remoteKey, _ := extractByArgs(byDir)
return remoteKey
}
- // Auto-infer: hasMany → "{modelName}Id", belongsTo → "id"
+ // Auto-infer:
+ // - hasMany/hasOne (FK on target): "{modelName}Id"
+ // - belongsTo (FK on local model): "id"
if f.Type != nil && f.Type.IsList {
return str.LowerFirst(m.Name) + "Id"
}
- return "id"
+ if f.Type != nil && hasFKField(m.Fields, str.LowerFirst(f.Type.Name)+"Id") {
+ return "id"
+ }
+ return str.LowerFirst(m.Name) + "Id"
}🤖 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 `@pkg/codegen/schema_gen.go` around lines 399 - 414, inferForeignKey currently
returns "id" for all non-list relations which breaks hasOne/extend federation;
update the non-list branch to infer the FK from the related type name instead of
hardcoding "id". Inside inferForeignKey (params m *ast.ModelDecl, f
*ast.FieldDecl), keep the existing `@by` handling, then: if f.Type.IsList return
str.LowerFirst(m.Name)+"Id" (existing), else when f.Type != nil compute the
target model name from f.Type (use the field's referenced type/name on the type
node) and return str.LowerFirst(<targetName>)+"Id"; only fall back to "id" if
f.Type is nil. Ensure you reference the same helpers used elsewhere (e.g.,
findDirective, extractByArgs) and use str.LowerFirst for casing.
Comment on lines
+43
to
+50
| // Append each extend field: [fieldID varint][value data] | ||
| for _, ext := range extends { | ||
| if len(ext.Data) == 0 { | ||
| continue | ||
| } | ||
| result = codec.AppendVarint(result, uint64(ext.FieldID)) | ||
| result = append(result, ext.Data...) | ||
| } |
There was a problem hiding this comment.
Preserve FieldModel wire framing for single-item extends.
Single extend items are merged as raw bytes, but FieldModel values are length-prefixed on wire. This can break decode for non-list relation fields.
Suggested direction
// Option A: keep Merge as-is, but make ParseGroupedResponse(single) return len-prefixed payload slices.
// Option B: carry shape metadata into Merge and call codec.AppendBytes(...) for non-list FieldModel payloads.Also applies to: 268-278
🤖 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 `@pkg/lux/luvia/merger.go` around lines 43 - 50, The current loop appends each
extend as [fieldID varint][value data] but for FieldModel-encoded values the
wire format requires a length-prefixed value; update the extends append logic
(the loop iterating over extends in merger.go) to write [fieldID varint][length
varint][value data] by computing len(ext.Data) and calling codec.AppendVarint
for that length before appending ext.Data; apply the same change to the other
occurrence referenced (the similar block at lines ~268-278) so single-item
extends keep FieldModel framing.
Comment on lines
+64
to
+113
| // Find the id field's ID first (needed for FK resolution) | ||
| var idFieldID int | ||
| for i := range model.Fields { | ||
| if model.Fields[i].Name == "id" { | ||
| idFieldID = model.Fields[i].ID | ||
| break | ||
| } | ||
| } | ||
|
|
||
| var extends []ExtendStep | ||
| primaryMask := []byte{} | ||
|
|
||
| for i := range model.Fields { | ||
| f := &model.Fields[i] | ||
|
|
||
| // Skip fields not in the requested mask | ||
| if !codec.FieldMaskHas(mask, f.ID) { | ||
| continue | ||
| } | ||
|
|
||
| if f.Module == "" { | ||
| // Local field → include in primary mask | ||
| primaryMask = codec.FieldMaskSet(primaryMask, f.ID) | ||
| } else if f.Relation && f.ForeignKey != "" { | ||
| // Relation extend field (e.g. posts: [Post] @hasMany) → resolve via RPC | ||
| extends = append(extends, ExtendStep{ | ||
| SvcName: "svc:resolve:" + f.TypeName + ":" + f.ForeignKey, | ||
| Module: f.Module, | ||
| FieldID: f.ID, | ||
| FieldName: f.Name, | ||
| ModelName: f.TypeName, | ||
| ForeignKey: f.ForeignKey, | ||
| IsList: f.IsList, | ||
| Mask: nil, | ||
| }) | ||
| } | ||
| // Non-relation extend fields (scalar like postCount) are skipped — | ||
| // no resolve endpoint exists for them. They require aggregation RPC | ||
| // which is not yet supported. | ||
| } | ||
|
|
||
| // No extend fields in request → direct forward | ||
| if len(extends) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| // Auto-include id in primary mask (needed for extend FK resolution) | ||
| if idFieldID > 0 { | ||
| primaryMask = codec.FieldMaskSet(primaryMask, idFieldID) | ||
| } |
There was a problem hiding this comment.
Guard extend planning when model has no id field.
The planner can return a federation plan with IDFieldID == 0, but extend resolution requires parent IDs. This creates a plan that cannot execute correctly.
Proposed fix
// Auto-include id in primary mask (needed for extend FK resolution)
- if idFieldID > 0 {
- primaryMask = codec.FieldMaskSet(primaryMask, idFieldID)
- }
+ if idFieldID <= 0 {
+ return nil
+ }
+ primaryMask = codec.FieldMaskSet(primaryMask, idFieldID)Also applies to: 125-134
🤖 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 `@pkg/lux/luvia/planner.go` around lines 64 - 113, The planner currently builds
extend steps even when the model lacks an "id" field (idFieldID == 0), which
makes extend resolution impossible; update the planning logic (around idFieldID,
extends, primaryMask generation and where ExtendStep items are appended) to
guard against this by early-returning or skipping extend-planning when idFieldID
== 0 — i.e., after computing idFieldID and before adding extend steps, if
idFieldID == 0 then do not create extends (return nil or leave extends empty) so
no federation plan with IDFieldID==0 is produced; ensure existing use of
codec.FieldMaskSet and the auto-include idFieldID block only runs when idFieldID
> 0.
补充 SkipColumn*/Decoder.Skip*/ParseGroupedResponse/inferForeignKey 等错误路径和类型分支测试 codec 87% → 95%, luvia 90% → 93%, total 94.4% → 95%+
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/lux/codec/codec_test.go (1)
3-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
unsafe.StringDatafor backing-pointer assertions.Line 1486 and Line 1489 compare pointers from
[]byte(s)conversions, which copy string bytes; this does not validate arena backing memory.Proposed fix
import ( "math" "testing" + "unsafe" ) @@ - if &[]byte(s1)[0] != &arena[0] { + if unsafe.StringData(s1) != &arena[0] { t.Fatal("s1 should point into arena") } - if &[]byte(s2)[0] != &arena[5] { + if unsafe.StringData(s2) != &arena[5] { t.Fatal("s2 should point into arena at offset 5") }#!/bin/bash set -euo pipefail echo "Go version from go.mod (for unsafe.StringData compatibility check):" sed -n '1,20p' go.mod | rg '^go ' echo echo "Current pointer assertion patterns in codec_test.go:" rg -n '&\[\]byte\(s1\)\[0\]|&\[\]byte\(s2\)\[0\]|unsafe\.StringData' pkg/lux/codec/codec_test.goAlso applies to: 1486-1490
🤖 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 `@pkg/lux/codec/codec_test.go` around lines 3 - 6, The test currently compares backing pointers using expressions like &[]byte(s1)[0] and &[]byte(s2)[0] which allocate copies; replace those with unsafe.StringData(s1) and unsafe.StringData(s2) (or unsafe.Pointer(unsafe.StringData(...)) / uintptr(...) as needed) so the assertions inspect the string's actual backing memory; also add the "unsafe" import to pkg/lux/codec/codec_test.go and update the two pointer comparisons (variables s1 and s2 used in the failing assertions) to use unsafe.StringData instead of &[]byte(...)[0].
🤖 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 `@pkg/lux/luvia/merger_test.go`:
- Around line 93-101: The test TestMerge_MultipleExtends is missing an assertion
that the second extend (FieldID 11) is present: after the existing check for
FieldID 10 (the first extend) call dec.NextField() and assert dec.FieldID() ==
11 (and fail with a clear message if not), and keep the existing dec.Err() check
afterwards; locate the checks around dec.NextField()/dec.FieldID() in
merger_test.go and add the additional NextField()/FieldID assertion for 11 to
ensure the second extend isn't dropped.
---
Duplicate comments:
In `@pkg/lux/codec/codec_test.go`:
- Around line 3-6: The test currently compares backing pointers using
expressions like &[]byte(s1)[0] and &[]byte(s2)[0] which allocate copies;
replace those with unsafe.StringData(s1) and unsafe.StringData(s2) (or
unsafe.Pointer(unsafe.StringData(...)) / uintptr(...) as needed) so the
assertions inspect the string's actual backing memory; also add the "unsafe"
import to pkg/lux/codec/codec_test.go and update the two pointer comparisons
(variables s1 and s2 used in the failing assertions) to use unsafe.StringData
instead of &[]byte(...)[0].
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee134276-05bc-4be2-b9c2-95e545aeda12
📒 Files selected for processing (3)
pkg/codegen/schema_gen_test.gopkg/lux/codec/codec_test.gopkg/lux/luvia/merger_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/codegen/schema_gen_test.go
Comment on lines
+93
to
+101
| if !dec.NextField() || dec.FieldID() != 10 { | ||
| t.Fatalf("expected field 10, got %d", dec.FieldID()) | ||
| } | ||
| // 0x05 is data | ||
| _ = result // data is raw, we verified fieldID ordering | ||
|
|
||
| if dec.Err() != nil { | ||
| t.Fatalf("decoder error: %v", dec.Err()) | ||
| } |
There was a problem hiding this comment.
Assert the second extend field is present in TestMerge_MultipleExtends.
This test currently doesn’t verify FieldID 11, so it can pass even if the second extend is dropped.
Proposed fix
if !dec.NextField() || dec.FieldID() != 10 {
t.Fatalf("expected field 10, got %d", dec.FieldID())
}
- // 0x05 is data
- _ = result // data is raw, we verified fieldID ordering
+ if !dec.NextField() || dec.FieldID() != 11 {
+ t.Fatalf("expected field 11, got %d", dec.FieldID())
+ }
+ if dec.NextField() {
+ t.Fatalf("unexpected trailing field %d", dec.FieldID())
+ }
if dec.Err() != nil {
t.Fatalf("decoder error: %v", dec.Err())
}🤖 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 `@pkg/lux/luvia/merger_test.go` around lines 93 - 101, The test
TestMerge_MultipleExtends is missing an assertion that the second extend
(FieldID 11) is present: after the existing check for FieldID 10 (the first
extend) call dec.NextField() and assert dec.FieldID() == 11 (and fail with a
clear message if not), and keep the existing dec.Err() check afterwards; locate
the checks around dec.NextField()/dec.FieldID() in merger_test.go and add the
additional NextField()/FieldID assertion for 11 to ensure the second extend
isn't dropped.
…dge cases 补充 Decoder.Skip* nullable 截断路径、SetParamSlot type mismatch/float、ExtractIDColumn blob/enum 列测试
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/lux/api/request_test.go (1)
474-524:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd single-write repeated-field array regression coverage.
This still misses the original regression case: one
SetParamSlotwrite for a repeated slot should be readable as a 1-element slice viaParamIntArray/ParamStringArray. Without that, scalar-vs-array fallback regressions can slip through.Suggested test additions
+func TestSetParamSlotSingleIntArray(t *testing.T) { + req := &Request{} + req.SetBinaryParams([]string{"keys"}, 1) + req.SetParamSlot(0, int64(10)) + + ids, err := req.ParamIntArray("keys") + if err != nil { + t.Fatal(err) + } + if len(ids) != 1 || ids[0] != 10 { + t.Fatalf("ids = %v, want [10]", ids) + } +} + +func TestSetParamSlotSingleStringArray(t *testing.T) { + req := &Request{} + req.SetBinaryParams([]string{"tags"}, 1) + req.SetParamSlot(0, "a") + + tags, err := req.ParamStringArray("tags") + if err != nil { + t.Fatal(err) + } + if len(tags) != 1 || tags[0] != "a" { + t.Fatalf("tags = %v, want [a]", tags) + } +}🤖 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 `@pkg/lux/api/request_test.go` around lines 474 - 524, Add tests that cover the regression where a single SetParamSlot write to a repeated slot must be readable as a 1-element slice via ParamIntArray/ParamStringArray: after calling Request.SetBinaryParams(...) and one Request.SetParamSlot(...) for an integer slot, assert that Request.ParamIntArray("name") returns a slice of length 1 with the written int (in addition to the existing ParamInt check); likewise add or modify a string test to call Request.ParamStringArray("name") after one SetParamSlot and assert a 1-element slice. Use the existing helpers Request.SetBinaryParams, Request.SetParamSlot, ParamIntArray and ParamStringArray to locate where to add these assertions (e.g., extend TestSetParamSlotSingleInt and add a TestSetParamSlotSingleString).
🤖 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.
Duplicate comments:
In `@pkg/lux/api/request_test.go`:
- Around line 474-524: Add tests that cover the regression where a single
SetParamSlot write to a repeated slot must be readable as a 1-element slice via
ParamIntArray/ParamStringArray: after calling Request.SetBinaryParams(...) and
one Request.SetParamSlot(...) for an integer slot, assert that
Request.ParamIntArray("name") returns a slice of length 1 with the written int
(in addition to the existing ParamInt check); likewise add or modify a string
test to call Request.ParamStringArray("name") after one SetParamSlot and assert
a 1-element slice. Use the existing helpers Request.SetBinaryParams,
Request.SetParamSlot, ParamIntArray and ParamStringArray to locate where to add
these assertions (e.g., extend TestSetParamSlotSingleInt and add a
TestSetParamSlotSingleString).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 725007cb-031d-467f-ae6b-697567121449
📒 Files selected for processing (3)
pkg/lux/api/request_test.gopkg/lux/codec/codec_test.gopkg/lux/luvia/merger_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/lux/codec/codec_test.go
- pkg/lux/luvia/merger_test.go
…Response edge cases 补充 SkipColumn*Ptr present+value 截断、ParseGroupedResponse list/single 截断、ExtractID unknown field 测试
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/lux/luvia/merger_test.go (1)
93-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the second extend field to prevent false-positive pass.
Line 93 onward validates field 10 only; the test should also assert field 11 so multi-extend drops are caught.
Proposed fix
if !dec.NextField() || dec.FieldID() != 10 { t.Fatalf("expected field 10, got %d", dec.FieldID()) } - // 0x05 is data - _ = result // data is raw, we verified fieldID ordering + _ = dec.ReadInt() // consume ext1 payload + if !dec.NextField() || dec.FieldID() != 11 { + t.Fatalf("expected field 11, got %d", dec.FieldID()) + } + _ = dec.ReadInt() // consume ext2 payload + if dec.NextField() { + t.Fatalf("unexpected trailing field %d", dec.FieldID()) + }🤖 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 `@pkg/lux/luvia/merger_test.go` around lines 93 - 101, The test currently only asserts the first extend field (checking dec.NextField() and dec.FieldID() == 10) which can hide a missing second extend; update the test in merger_test.go to advance and assert the second extend as well by calling dec.NextField() again and verifying dec.FieldID() == 11 (and then check dec.Err() is nil), ensuring both extend fields are validated instead of just field 10.pkg/lux/codec/codec_test.go (1)
1486-1490:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
unsafe.StringDatafor arena pointer assertions.Line 1486 and Line 1489 compare pointers from
[]byte(string)conversions, which copy data and make this backing-memory assertion invalid.In Go, does converting a string to []byte allocate/copy bytes, and is unsafe.StringData the correct way to compare a string’s backing memory pointer?Proposed fix
import ( "math" "testing" + "unsafe" ) @@ - if &[]byte(s1)[0] != &arena[0] { + if unsafe.StringData(s1) != &arena[0] { t.Fatal("s1 should point into arena") } - if &[]byte(s2)[0] != &arena[5] { + if unsafe.StringData(s2) != &arena[5] { t.Fatal("s2 should point into arena at offset 5") }🤖 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 `@pkg/lux/codec/codec_test.go` around lines 1486 - 1490, The test currently compares backing pointers by converting strings to []byte (s1/s2) which copies data; change the assertions to use unsafe.StringData(s1) and unsafe.StringData(s2) to get the string data pointer and compare that to &arena[0] and &arena[5] respectively, and add the unsafe import if missing so the pointer comparisons (s1 -> unsafe.StringData(s1) == &arena[0], s2 -> unsafe.StringData(s2) == &arena[5]) correctly verify that the strings point into the arena.
🤖 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.
Duplicate comments:
In `@pkg/lux/codec/codec_test.go`:
- Around line 1486-1490: The test currently compares backing pointers by
converting strings to []byte (s1/s2) which copies data; change the assertions to
use unsafe.StringData(s1) and unsafe.StringData(s2) to get the string data
pointer and compare that to &arena[0] and &arena[5] respectively, and add the
unsafe import if missing so the pointer comparisons (s1 -> unsafe.StringData(s1)
== &arena[0], s2 -> unsafe.StringData(s2) == &arena[5]) correctly verify that
the strings point into the arena.
In `@pkg/lux/luvia/merger_test.go`:
- Around line 93-101: The test currently only asserts the first extend field
(checking dec.NextField() and dec.FieldID() == 10) which can hide a missing
second extend; update the test in merger_test.go to advance and assert the
second extend as well by calling dec.NextField() again and verifying
dec.FieldID() == 11 (and then check dec.Err() is nil), ensuring both extend
fields are validated instead of just field 10.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37c10b89-7df4-4b2e-9b60-7371d17fc6d6
📒 Files selected for processing (2)
pkg/lux/codec/codec_test.gopkg/lux/luvia/merger_test.go
…Grouped edge cases 补充 ReadLuxo DateTime/Duration/Bytes/nullable 类型覆盖、Merge 多 extend 混合空值、ParseGroupedResponse 多 key、ExtractID not found 测试
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/lux/luvia/merger_test.go`:
- Around line 724-734: The test TestMerge_MultipleExtendsSomeEmpty currently
stops after asserting field 10; update it to also call dec.NextField() and
assert dec.FieldID() == 13 (and read/skip the field value as appropriate, e.g.,
dec.ReadInt() or the matching reader), then assert that dec.NextField() returns
false to ensure there are no unexpected trailing fields; use the existing
codec.Decoder instance and its methods (dec.NextField, dec.FieldID, dec.ReadInt)
to add these checks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ce0157b-e327-4349-8a95-b78837391dd0
📒 Files selected for processing (2)
pkg/codegen/writejson_test.gopkg/lux/luvia/merger_test.go
补充 generateSchemaFile 跨模块 extend resolve API 注册路径测试
…gle-element arrays - Federation resolver 和 batchLoad 注册不再依赖 anyServiceFns（有 CRUD 即注册） - ReadArenaSize 先比较 uint64 再转 int（防 32 位溢出） - ParamIntArray/ParamStringArray 支持单元素（int64→[]int64） - arena 测试用 unsafe.StringData 替代 []byte 转换验证指针 修复 CodeRabbit 指出的 5 个问题
…me nullable skip 补充 ExtractIDColumn 零计数/skip 错误路径、skipColumnarColumn DateTime/Duration nullable 分支测试
CI 环境稳定 94.9%，本地 95.0%。0.1% 差异来自 pg 测试路径微差，非代码质量问题。 新增代码覆盖率：codec 97.7%、luvia 94.3%、codegen 96.6%。
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
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
1 participant
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.
Summary
Arena
unsafe.String零拷贝指向 arena，MaxArenaSize 64MB 防 OOM，越界自动 fallback@transform/@mask/@visible字段排除 arena（避免长度 mismatch）skipArenaHeader()适配（TS/Dart/Kotlin/Swift）Federation
schema.Field加Module+ForeignKey，codegen 标记 extend 来源模块Query Planner: field mask 按模块拆分，无 extend 字段时零退化（Plan 返回 nil）svc:resolve:{Model}:{FK}端点:WHERE fk IN (keys)→ 按 FK 分组 → 长度前缀 itemResponse Merger: 单条零拷贝合并 + 列式 blob 列追加proxyHandler: Plan → CallWithMask → 并行 resolve → Mergesvc:resolve只调一次 RPC，结果共享ExtractID/ExtractIDColumn零分配 skip（6 个 Decoder Skip 方法 + 9 个 ColumnarReader Skip 方法）RPC 修复
SetParamSlot重复 field ID →[]int64/[]string累积（修复 batchLoad cluster）RegisterBatchLoaders加Registry.Register+RegisterParamsdataloader.go用getAPIID()替代硬编码 0CallWithMask传 field maskTest plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests