Skip to content

Commit f390385

Browse files
authored
Merge pull request #2881 from dolthub/fulghum/bugfix-jsonb
Bug fixes: JSONB serialization/deserialization
2 parents dccb410 + 0193709 commit f390385

5 files changed

Lines changed: 234 additions & 5 deletions

File tree

integration-tests/go-sql-server-driver/large_values_test.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,6 @@ func TestLargeOutOfBandValues(t *testing.T) {
280280

281281
t.Run("LargeJSON", func(t *testing.T) {
282282
t.Parallel()
283-
t.Skip("Doltgres panics (interface conversion: *val.ExtendedValueWrapper) when retrieving a large out-of-band JSON value")
284283
const jsonTargetSize = 12_000
285284
const numRows = 20
286285

@@ -992,7 +991,6 @@ func TestTypeDiversity(t *testing.T) {
992991

993992
t.Run("SpecialTypes", func(t *testing.T) {
994993
t.Parallel()
995-
t.Skip("Doltgres panics (interface conversion: *val.ExtendedValueWrapper) when retrieving a large out-of-band JSON value")
996994
server := setupTestServer(t, "special_types_test")
997995
db, err := server.DB(driver.Connection{})
998996
require.NoError(t, err)
@@ -1055,10 +1053,11 @@ func TestTypeDiversity(t *testing.T) {
10551053
require.NoError(t, json.Unmarshal([]byte(jsonDoc), &jsonObj))
10561054
require.Greater(t, len(jsonObj), 0, "large JSON value must be a non-empty object")
10571055

1058-
// Verify ENUM and SET values. FIND_IN_SET('blue', col_set) > 0 becomes a
1059-
// membership test against the comma-separated string.
1056+
// Verify ENUM and SET values. Use a comma-padded LIKE pattern to test
1057+
// exact element membership in the comma-separated SET string without
1058+
// relying on string_to_array, which is not yet implemented in Doltgres.
10601059
err = conn.QueryRowContext(ctx,
1061-
"SELECT COUNT(*) FROM special_types WHERE col_enum = 'gamma' AND ('blue' = ANY(string_to_array(col_set, ',')))").Scan(&count)
1060+
"SELECT COUNT(*) FROM special_types WHERE col_enum = 'gamma' AND (',' || col_set || ',') LIKE '%,blue,%'").Scan(&count)
10621061
require.NoError(t, err)
10631062
require.Equal(t, 1, count, "ENUM and SET values must round-trip correctly")
10641063

server/functions/json.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,15 @@ var json_out = framework.Function1{
7878
}
7979

8080
func json_out_callable(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
81+
// Large values stored with the old ExtendedAdaptiveEnc encoding are returned as a
82+
// *val.ExtendedValueWrapper (sql.AnyWrapper). Unwrap to get the underlying JSONDocument.
83+
if wrapper, ok := val.(sql.AnyWrapper); ok {
84+
unwrapped, err := wrapper.UnwrapAny(ctx)
85+
if err != nil {
86+
return nil, err
87+
}
88+
val = unwrapped
89+
}
8190
switch v := val.(type) {
8291
case string:
8392
return v, nil
@@ -89,8 +98,25 @@ func json_out_callable(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (a
8998
}
9099
}
91100

101+
// JsonOutCallable is exported so that tests outside this package can exercise the json output
102+
// function directly without going through the global function registry.
103+
var JsonOutCallable = json_out_callable
104+
105+
// JsonbOutCallable is exported so that tests outside this package can exercise the jsonb output
106+
// function directly without going through the global function registry.
107+
var JsonbOutCallable = jsonb_out_callable
108+
92109
// jsonb_out_callable formats a JSONB value for output. JSONB normalizes JSON with spaces after ':' and ','.
93110
func jsonb_out_callable(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
111+
// Large values stored with the old ExtendedAdaptiveEnc encoding are returned as a
112+
// *val.ExtendedValueWrapper (sql.AnyWrapper). Unwrap to get the underlying JSONDocument.
113+
if wrapper, ok := val.(sql.AnyWrapper); ok {
114+
unwrapped, err := wrapper.UnwrapAny(ctx)
115+
if err != nil {
116+
return nil, err
117+
}
118+
val = unwrapped
119+
}
94120
switch v := val.(type) {
95121
case string:
96122
// Parse and reformat the string as proper JSONB (normalized with spaces)

server/types/json_document.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,10 @@ func ConvertToJsonDocument(val interface{}) (JsonValue, error) {
319319
return nil, err
320320
}
321321
return JsonValueNumber(*d), nil
322+
case *apd.Decimal:
323+
return JsonValueNumber(*val), nil
324+
case apd.Decimal:
325+
return JsonValueNumber(val), nil
322326
case bool:
323327
return JsonValueBoolean(val), nil
324328
case nil:

testing/go/json_test.go

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
// Copyright 2026 Dolthub, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package _go
16+
17+
import (
18+
"context"
19+
"testing"
20+
21+
"github.com/dolthub/go-mysql-server/sql"
22+
gmstypes "github.com/dolthub/go-mysql-server/sql/types"
23+
"github.com/stretchr/testify/require"
24+
25+
"github.com/dolthub/doltgresql/server/functions"
26+
pgtypes "github.com/dolthub/doltgresql/server/types"
27+
)
28+
29+
// testAnyWrapper simulates *val.ExtendedValueWrapper — a sql.AnyWrapper that wraps a
30+
// sql.JSONWrapper rather than a plain string. This is what Dolt returns when reading a
31+
// large JSONB value that was stored out-of-band with the old ExtendedAdaptiveEnc encoding
32+
// (used before JsonAdaptiveEnc was enabled for json/jsonb columns). UnwrapAny() on such a
33+
// wrapper calls the child handler's DeserializeValue, which for JSONB returns a
34+
// gmstypes.JSONDocument, not a string.
35+
type testAnyWrapper struct {
36+
inner interface{}
37+
}
38+
39+
func (w *testAnyWrapper) UnwrapAny(_ context.Context) (interface{}, error) { return w.inner, nil }
40+
func (w *testAnyWrapper) IsExactLength() bool { return true }
41+
func (w *testAnyWrapper) MaxByteLength() int64 { return 1000 }
42+
func (w *testAnyWrapper) Compare(_ context.Context, _ interface{}) (int, bool, error) {
43+
return 0, false, nil
44+
}
45+
func (w *testAnyWrapper) Hash() interface{} { return nil }
46+
47+
// TestJsonbOutCallableWithAnyWrapper verifies that jsonb_out_callable handles a
48+
// sql.AnyWrapper value. Large JSONB values in databases created before JsonAdaptiveEnc
49+
// was enabled (using ExtendedAdaptiveEnc instead) are returned from storage as a
50+
// *val.ExtendedValueWrapper, which implements sql.AnyWrapper. Its UnwrapAny() yields a
51+
// gmstypes.JSONDocument — so jsonb_out_callable must unwrap and then handle the document,
52+
// not panic or return an "unexpected type" error.
53+
func TestJsonbOutCallableWithAnyWrapper(t *testing.T) {
54+
ctx := sql.NewEmptyContext()
55+
56+
tests := []struct {
57+
name string
58+
jsonStr string
59+
}{
60+
{"object", `{"key": "value", "num": 42}`},
61+
{"array", `[1, 2, 3]`},
62+
{"nested", `{"a": {"b": [true, null]}}`},
63+
{"string_scalar", `"hello"`},
64+
{"number_scalar", `99`},
65+
{"bool_scalar", `false`},
66+
{"null_scalar", `null`},
67+
// large_object exercises the out-of-band storage path: a document large
68+
// enough that the old ExtendedAdaptiveEnc encoding would store it off-page.
69+
{"large_object", makeLargeJSONObject(100)},
70+
}
71+
72+
for _, tt := range tests {
73+
t.Run(tt.name, func(t *testing.T) {
74+
jsonDoc := gmstypes.MustJSON(tt.jsonStr)
75+
76+
result, err := functions.JsonbOutCallable(ctx, [2]*pgtypes.DoltgresType{}, &testAnyWrapper{inner: jsonDoc})
77+
require.NoError(t, err, "jsonb_out_callable must handle sql.AnyWrapper wrapping a JSONDocument")
78+
require.NotEmpty(t, result)
79+
})
80+
}
81+
}
82+
83+
// TestJsonOutCallableWithAnyWrapper is the json_out equivalent of TestJsonbOutCallableWithAnyWrapper.
84+
// json_out_callable has the same AnyWrapper gap as jsonb_out_callable.
85+
func TestJsonOutCallableWithAnyWrapper(t *testing.T) {
86+
ctx := sql.NewEmptyContext()
87+
88+
tests := []struct {
89+
name string
90+
jsonStr string
91+
}{
92+
{"object", `{"key": "value", "num": 42}`},
93+
{"array", `[1, 2, 3]`},
94+
{"nested", `{"a": {"b": [true, null]}}`},
95+
{"string_scalar", `"hello"`},
96+
{"number_scalar", `99`},
97+
{"bool_scalar", `false`},
98+
{"null_scalar", `null`},
99+
{"large_object", makeLargeJSONObject(100)},
100+
}
101+
102+
for _, tt := range tests {
103+
t.Run(tt.name, func(t *testing.T) {
104+
jsonDoc := gmstypes.MustJSON(tt.jsonStr)
105+
106+
result, err := functions.JsonOutCallable(ctx, [2]*pgtypes.DoltgresType{}, &testAnyWrapper{inner: jsonDoc})
107+
require.NoError(t, err, "json_out_callable must handle sql.AnyWrapper wrapping a JSONDocument")
108+
require.NotEmpty(t, result)
109+
})
110+
}
111+
}
112+
113+
// TestJsonbSerializeWithAnyWrapper verifies that types.JsonB.SerializeValue handles a
114+
// sql.AnyWrapper that wraps a JSONDocument containing *apd.Decimal values. This
115+
// represents the combined Bug 1 + Bug 2 production failure path:
116+
//
117+
// 1. A large legacy JSONB value is read from Dolt → *val.ExtendedValueWrapper (AnyWrapper).
118+
// 2. The wrapper is passed to serializeTypeJsonB (e.g. during replication or export).
119+
// 3. sql.UnwrapAny extracts a gmstypes.JSONDocument.
120+
// 4. ToInterface() returns the .Val, which after the jsonValueToInterface bugfix
121+
// contains *apd.Decimal for numeric fields.
122+
// 5. ConvertToJsonDocument(*apd.Decimal) must handle that type — previously it did not.
123+
func TestJsonbSerializeWithAnyWrapper(t *testing.T) {
124+
ctx := sql.NewEmptyContext()
125+
126+
tests := []struct {
127+
name string
128+
inner gmstypes.JSONDocument
129+
}{
130+
{
131+
"scalar_decimal",
132+
gmstypes.JSONDocument{Val: mustDecimal("3.14")},
133+
},
134+
{
135+
"object_with_decimal",
136+
gmstypes.JSONDocument{Val: map[string]any{
137+
"n": mustDecimal("42"),
138+
"s": "hello",
139+
}},
140+
},
141+
{
142+
"array_with_decimals",
143+
gmstypes.JSONDocument{Val: []any{mustDecimal("1"), mustDecimal("2"), mustDecimal("3")}},
144+
},
145+
{
146+
"nested_decimals",
147+
gmstypes.JSONDocument{Val: map[string]any{
148+
"a": map[string]any{"b": mustDecimal("-9.9")},
149+
}},
150+
},
151+
}
152+
153+
for _, tt := range tests {
154+
t.Run(tt.name, func(t *testing.T) {
155+
wrapper := &testAnyWrapper{inner: tt.inner}
156+
_, err := pgtypes.JsonB.SerializeValue(ctx, wrapper)
157+
require.NoError(t, err,
158+
"SerializeValue must handle an AnyWrapper wrapping a JSONDocument with *apd.Decimal")
159+
})
160+
}
161+
}

testing/go/jsonb_test.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,45 @@ func requireJsonEqual(t *testing.T, expected, actual any) {
300300
}
301301
}
302302

303+
// TestLegacyJsonBReserialization verifies that a value deserialized from the legacy
304+
// ExtendedEnc format can be re-serialized without error. This exercises the path where
305+
// serializeTypeJsonB receives a JSONDocument whose Val contains *apd.Decimal (the type
306+
// that jsonValueToInterface now returns for JsonValueNumber after the bugfix), and
307+
// ConvertToJsonDocument must handle it.
308+
func TestLegacyJsonBReserialization(t *testing.T) {
309+
tests := []struct {
310+
name string
311+
input any
312+
}{
313+
{"number_integer", json.Number("42")},
314+
{"number_negative", json.Number("-7")},
315+
{"number_float", json.Number("3.14")},
316+
{"number_large", json.Number("99999999999999999999")},
317+
{"object_with_number", map[string]any{"n": json.Number("99")}},
318+
{"array_with_numbers", []any{json.Number("1"), json.Number("2"), json.Number("3")}},
319+
{"nested_number", map[string]any{"a": map[string]any{"b": json.Number("1.5")}}},
320+
}
321+
322+
for _, tt := range tests {
323+
t.Run(tt.name, func(t *testing.T) {
324+
doc := gmstypes.JSONDocument{Val: tt.input}
325+
326+
// Step 1: serialize (simulates old Doltgres write using the legacy ExtendedEnc path)
327+
serialized, err := types.JsonB.SerializeValue(context.Background(), doc)
328+
require.NoError(t, err)
329+
330+
// Step 2: deserialize (jsonValueToInterface now returns *apd.Decimal for numbers)
331+
deserialized, err := types.JsonB.DeserializeValue(context.Background(), serialized)
332+
require.NoError(t, err)
333+
require.NotNil(t, deserialized)
334+
335+
// Step 3: re-serialize the deserialized value.
336+
_, err = types.JsonB.SerializeValue(context.Background(), deserialized)
337+
require.NoError(t, err, "re-serializing a deserialized legacy JSONB value must not fail")
338+
})
339+
}
340+
}
341+
303342
// mustDecimal parses s into an *apd.Decimal, panicking on failure.
304343
func mustDecimal(s string) *apd.Decimal {
305344
d := new(apd.Decimal)

0 commit comments

Comments
 (0)