Skip to content

JsonType.Convert cannot safely reject binary-charset []byte without breaking legitimate internal []byte usage #3631

Description

@vishnujayvel

Summary

Follow-up to dolthub/dolt#11216 (companion PR: #3632).

MySQL rejects converting a CHARACTER SET 'binary' string to JSON with ERROR 3144 (22032): Cannot create a JSON value from a string with CHARACTER SET 'binary'. go-mysql-server currently has no equivalent check: JsonType.Convert (sql/types/json.go) accepts []byte unconditionally and parses it via encoding/json, which silently replaces invalid UTF-8 bytes rather than erroring. This causes INSERT INTO t VALUES (CONCAT('"', CHAR(233), '"')) into a JSON column to silently corrupt data (byte 0xE9 becomes U+FFFD) instead of raising an error, reported as dolthub/dolt#11216.

Why this isn't a minimal fix in JsonType.Convert

The obvious fix -- reject []byte unconditionally in JsonType.Convert, since a []byte reaching that function should mean "originated from a CHARACTER SET 'binary' SQL string" (see StringType.Convert) -- breaks legitimate, already-tested internal usage. []byte is also the natural Go representation for pre-marshaled, trusted JSON bytes used internally:

  • sql/types/jsontests/json_test.go's TestJsonConvert explicitly asserts types.JSON.Convert(ctx, []byte({"a": true, "b": 3})) succeeds.
  • sql/expression/function/json/jsontests/json_function_tests.go's "JsonDocument"/"LazyJsonDocument" test-prep helpers call types.JSON.Convert directly on []byte produced by types.MarshallJson.

I verified empirically: rejecting all []byte unconditionally in JsonType.Convert fails both TestJsonConvert and TestJsonExtract/JsonDocument on current main.

There is no way to distinguish "a []byte that is a genuine SQL binary-charset string coercion" from "a []byte of trusted, pre-marshaled JSON bytes" using only the runtime value inside JsonType.Convert(ctx, v interface{}) -- both arrive as plain []byte with no other signal.

Where the fix likely belongs instead

The charset check needs to happen at a point where the source expression's sql.Type (specifically, whether it's a StringType with CharacterSet() == sql.CharacterSet_binary) is still in scope, before the value is handed to the generic JsonType.Convert:

  1. sql/expression/convert.go's ConvertToJSON (the CAST(x AS JSON) path) -- has direct access to the source child expression's Type(ctx).
  2. The row-value-to-column-type coercion path used by INSERT/UPDATE (e.g. sql/rowexec/insert.go around the col.Type.Convert(...) call) -- this is generic, column-type-agnostic code shared across all types, so it would need either a JSON-specific special case, or for the source expression's type to be threaded alongside the evaluated row value so it's still available when the destination column type is JSON.

Both of these are more invasive than a single-file fix: (1) requires touching multiple CAST/INSERT/UPDATE call sites rather than one shared conversion function, and (2) may require plumbing additional type information through the row-execution pipeline that isn't currently carried alongside evaluated row values. Neither should be done as a drive-by; they deserve their own scoped PR and maintainer input on the right shape (a wrapper type to mark "trusted internal JSON bytes" distinctly from "raw SQL value" is one option worth discussing, as an alternative to threading source-type info through row execution).

Related

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions