Skip to content

Comments

fix(contrib/drivers/pgsql): preserve bytea data integrity on read and write#4678

Open
lingcoder wants to merge 1 commit intogogf:masterfrom
lingcoder:fix/issue-4677-pgsql-bytea-read-corruption
Open

fix(contrib/drivers/pgsql): preserve bytea data integrity on read and write#4678
lingcoder wants to merge 1 commit intogogf:masterfrom
lingcoder:fix/issue-4677-pgsql-bytea-read-corruption

Conversation

@lingcoder
Copy link
Contributor

@lingcoder lingcoder commented Feb 6, 2026

Summary

Fix two bytea data corruption issues in the PostgreSQL driver:

  1. READ path (fixes record.Get().Bytes() corrupts bytea data on retrieval from PostgreSQL #4677): CheckLocalTypeForField and ConvertValueForLocal had no case for plain bytea type, causing it to fall through to the Core layer which incorrectly mapped it to LocalTypeString. Binary data was then converted to string via gconv.String(), corrupting the bytes on retrieval.

  2. WRITE path (fixes contrib/drivers/pgsql/pgsql_convert.go: If the byte type data contains 0x5D, it will be forcibly converted to 0x7D, causing problems with the stored data. #4231): ConvertValueForField applied PostgreSQL array syntax conversion ([{, ]}) to all slice types including []byte for bytea columns, corrupting bytes 0x5B ([) and 0x5D (]) on insertion.

Changes

  • contrib/drivers/pgsql/pgsql_convert.go:

    • CheckLocalTypeForField: Add case "bytea"LocalTypeBytes
    • ConvertValueForLocal: Add case "bytea" to preserve []byte as-is
    • ConvertValueForField: Skip []{} replacement for []byte with bytea field type
  • contrib/drivers/pgsql/pgsql_z_unit_convert_test.go:

    • Add unit tests for bytea type in CheckLocalTypeForField, ConvertValueForLocal, and ConvertValueForField
  • contrib/drivers/pgsql/pgsql_z_unit_issue_test.go:

    • Add Test_Issue4677: End-to-end round-trip test with various binary data (including 0x00, 0x5B, 0x5D, 0xFF)
    • Add Test_Issue4231: Targeted test for 0x5D byte corruption on write

Test plan

  • Test_CheckLocalTypeForField - bytea returns LocalTypeBytes
  • Test_ConvertValueForLocal - bytea preserves []byte as-is
  • Test_ConvertValueForField - bytea skips array syntax replacement
  • Test_Issue4677 - full DB round-trip with binary data
  • Test_Issue4231 - write path preserves 0x5B/0x5D bytes
  • Full pgsql test suite passes with no regressions

closes #4677
closes #4231

ref #4689

… write

Fix two bytea data corruption issues in PostgreSQL driver:

1. READ path (gogf#4677): `CheckLocalTypeForField` and `ConvertValueForLocal`
   had no case for plain "bytea" type, causing it to fall through to Core
   layer which incorrectly mapped it to `LocalTypeString`. Binary data was
   converted to string via `gconv.String()`, corrupting the bytes.

2. WRITE path (gogf#4231): `ConvertValueForField` applied PostgreSQL array
   syntax conversion (`[`->`{`, `]`->`}`) to all slice types including
   `[]byte` for bytea columns, corrupting bytes 0x5B(`[`) and 0x5D(`]`).

closes gogf#4677, closes gogf#4231
@lingcoder lingcoder force-pushed the fix/issue-4677-pgsql-bytea-read-corruption branch from ac1dba1 to 96f7299 Compare February 6, 2026 16:17
@gqcn gqcn requested a review from Copilot February 11, 2026 02:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes two bytea data corruption paths in the PostgreSQL driver by ensuring bytea is treated as binary ([]byte) end-to-end rather than falling back to string conversion, and by preventing array-syntax rewriting from mutating raw []byte payloads.

Changes:

  • Add explicit bytea -> LocalTypeBytes mapping and bytea handling in ConvertValueForLocal to preserve raw bytes on reads.
  • Skip PostgreSQL slice/array syntax conversion ([/]{/}) for []byte when the target field is bytea on writes.
  • Add unit + integration/issue tests covering byte patterns that previously triggered corruption (including 0x00, 0x5B, 0x5D, 0xFF).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
contrib/drivers/pgsql/pgsql_convert.go Ensures bytea maps to []byte on read and prevents write-time bracket replacement for []byte destined for bytea.
contrib/drivers/pgsql/pgsql_z_unit_convert_test.go Adds focused unit tests validating the new bytea mapping/convert behavior.
contrib/drivers/pgsql/pgsql_z_unit_issue_test.go Adds regression tests for #4677 and #4231 using real round-trip DB operations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -154,6 +163,13 @@ func (d *Driver) ConvertValueForLocal(ctx context.Context, fieldType string, fie
// Basic types are mostly handled by Core layer, only handle array types here
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says this function only handles array types, but the newly-added case "bytea" is a basic (non-array) type. Please update the comment to reflect the current behavior so future changes don’t accidentally regress bytea handling.

Suggested change
// Basic types are mostly handled by Core layer, only handle array types here
// Basic types are mostly handled by Core layer; handle array types and special-case bytea here.

Copilot uses AI. Check for mistakes.
if v, ok := fieldValue.([]byte); ok {
return v, nil
}
return fieldValue, nil
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ConvertValueForLocal, the new bytea branch returns fieldValue unchanged when it’s not already a []byte. For consistency with the bytea -> []byte mapping (and to keep all conversions centralized), it may be clearer to delegate to the Core conversion or explicitly convert non-[]byte inputs to []byte instead of returning them as-is.

Suggested change
return fieldValue, nil
return d.Core.ConvertValueForLocal(ctx, fieldType, fieldValue)

Copilot uses AI. Check for mistakes.
@lingcoder
Copy link
Contributor Author

lingcoder commented Feb 12, 2026

@gqcn 关于bytea处理这块的建议:

保留原始[]byte值不调用Core.ConvertDataForRecord是有意的设计决定,主要考虑:

  1. 数据完整性 - 直接返回原始字节,调用方能拿到真实的bytea数据
  2. 便于调试 - 如果交给Core处理,出了问题反而不好排查,现在这样一眼就能看出是bytea类型
  3. 语义清晰 - 让调用方明确知道这是字节数组,该怎么处理心里有数

这个设计让bytea的行为更透明可预测,也符合最小惊讶原则。感谢review!👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant