avoid unnecessary interface conversions for JSONWrappers#2843
Conversation
|
|
SummaryOverall, 8 JSON serialization/dispatch tests ran with mixed results (4 passed, 4 failed): core readback and fidelity checks for object/array/scalar values plus large and threshold-adjacent payloads were stable and deterministic after runtime repair, including across retries and restart/cache-clear validation. The most important remaining issues are high-impact product bugs in explicit json_out paths—valid JSON can fail with unsupported types (types.JSONDocument) or panic on missing pg_catalog.cstring_out during output-function resolution (including structured and equivalent JSON forms), causing statement failures and no returned rows. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 JSON dispatch rejects valid runtime document
🟠 json_out panics on valid JSON input
🟠 Equivalent JSON forms fail at render
Tip Reply with @itoqa to send us feedback on this test run. |
There was a problem hiding this comment.
Structured json_out serialization panics on cstring resolution
What failed: Structured JSON values should serialize as text, but explicit json_out(...) on structured outputs triggers a server panic (cannot find function ... cstring_out) and returns no rows.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Queries that rely on explicit
json_out(...)for structured JSON serialization fail hard instead of returning data. This blocks a meaningful SQL serialization workflow and requires query rewrites to avoid the broken path. - Steps to Reproduce:
- Create a table with JSON values that can be serialized through row_to_json or array_to_json.
- Run
SELECT json_out(row_to_json(t)::json)orSELECT json_out(array_to_json(array_agg(...))::json). - Observe a server panic reporting missing
pg_catalog.cstring_outand no returned rows.
- Stub / mock content: SCRAM authentication was disabled in
server/authentication_scram.goso the run could connect with deterministic local credentials. The failing behavior came from normal SQL execution against the local server, without mocked response payloads for the exercised query path. - Code Analysis: I inspected
server/functions/json.go,server/types/type.go, andserver/types/function_registry.go. The new JSONBytes branch injson_out_callablenow returns a string for this path;IoOutputthen resolves the output function for the declared return type, and the registry panics whencstring_outis not loadable. - Why this is likely a bug: Production code reaches a deterministic panic path in core type-output resolution for valid structured JSON serialization queries, which is incorrect behavior for a user query path.
Relevant code
server/functions/json.go:80-95
func json_out_callable(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
switch v := val.(type) {
case string:
return v, nil
case types.JSONBytes:
bytes, err := v.GetBytes(ctx)
if err != nil {
return nil, err
}
return string(bytes), err
case sql.JSONWrapper:
// JSON type is stored as binary JSON (same as JSONB), so output is normalized with spaces
return jsonWrapperToFormattedString(ctx, v)
default:
return nil, fmt.Errorf("unexpected type for json_out: %T", val)
}
}server/types/type.go:603-614
// IoOutput converts given type value to output string.
func (t *DoltgresType) IoOutput(ctx *sql.Context, val any) (string, error) {
var o any
var err error
if t.ModInFunc != 0 || t.IsArrayType() || t.IsCompositeType() {
send := globalFunctionRegistry.GetFunction(ctx, t.OutputFunc)
resolvedTypes := send.ResolvedTypes()
resolvedTypes[0] = t
o, err = send.WithResolvedTypes(resolvedTypes).(QuickFunction).CallVariadic(ctx, val)
} else {
o, err = globalFunctionRegistry.GetFunction(ctx, t.OutputFunc).CallVariadic(ctx, val)
}server/types/function_registry.go:88-92
f = r.loadFunction(ctx, id)
if f == nil {
// If we hit this panic, then we're missing a test that uses this function (and we should add that test)
panic(errors.Errorf("cannot find function: `%s`", r.revMapping[id]))
}Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Structured json_out serialization panics on cstring resolution**
**What failed:** Structured JSON values should serialize as text, but explicit `json_out(...)` on structured outputs triggers a server panic (`cannot find function ... cstring_out`) and returns no rows.
- **Impact:** Queries that rely on explicit `json_out(...)` for structured JSON serialization fail hard instead of returning data. This blocks a meaningful SQL serialization workflow and requires query rewrites to avoid the broken path.
- **Steps to reproduce:**
1. Create a table with JSON values that can be serialized through row_to_json or array_to_json.
2. Run `SELECT json_out(row_to_json(t)::json)` or `SELECT json_out(array_to_json(array_agg(...))::json)`.
3. Observe a server panic reporting missing `pg_catalog.cstring_out` and no returned rows.
- **Stub / mock content:** SCRAM authentication was disabled in `server/authentication_scram.go` so the run could connect with deterministic local credentials. The failing behavior came from normal SQL execution against the local server, without mocked response payloads for the exercised query path.
- **Code analysis:** I inspected `server/functions/json.go`, `server/types/type.go`, and `server/types/function_registry.go`. The new JSONBytes branch in `json_out_callable` now returns a string for this path; `IoOutput` then resolves the output function for the declared return type, and the registry panics when `cstring_out` is not loadable.
- **Why this is likely a bug:** Production code reaches a deterministic panic path in core type-output resolution for valid structured JSON serialization queries, which is incorrect behavior for a user query path.
**Relevant code:**
`server/functions/json.go:80-95`
~~~go
func json_out_callable(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
switch v := val.(type) {
case string:
return v, nil
case types.JSONBytes:
bytes, err := v.GetBytes(ctx)
if err != nil {
return nil, err
}
return string(bytes), err
case sql.JSONWrapper:
// JSON type is stored as binary JSON (same as JSONB), so output is normalized with spaces
return jsonWrapperToFormattedString(ctx, v)
default:
return nil, fmt.Errorf("unexpected type for json_out: %T", val)
}
}
~~~
`server/types/type.go:603-614`
~~~go
// IoOutput converts given type value to output string.
func (t *DoltgresType) IoOutput(ctx *sql.Context, val any) (string, error) {
var o any
var err error
if t.ModInFunc != 0 || t.IsArrayType() || t.IsCompositeType() {
send := globalFunctionRegistry.GetFunction(ctx, t.OutputFunc)
resolvedTypes := send.ResolvedTypes()
resolvedTypes[0] = t
o, err = send.WithResolvedTypes(resolvedTypes).(QuickFunction).CallVariadic(ctx, val)
} else {
o, err = globalFunctionRegistry.GetFunction(ctx, t.OutputFunc).CallVariadic(ctx, val)
}
~~~
`server/types/function_registry.go:88-92`
~~~go
f = r.loadFunction(ctx, id)
if f == nil {
// If we hit this panic, then we're missing a test that uses this function (and we should add that test)
panic(errors.Errorf("cannot find function: `%s`", r.revMapping[id]))
}
~~~
|


No description provided.