Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(pgdialect): Remove unsigned integer conversion #1086

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

Aoang
Copy link
Contributor

@Aoang Aoang commented Dec 5, 2024

PostgreSQL does not support unsigned integer types. Previously, a Go uint32 type was mapped to the database integer type. When values exceeded the range, they were converted.
Even specifying the type as numeric did not prevent this conversion. This update removes the conversion, allowing the database to handle errors appropriately.

Fixed #624

PostgreSQL does not support unsigned integer types.
Previously, a Go uint32 type was mapped to the database
integer type. When values exceeded the range, they were
converted.
Even specifying the type as numeric did not prevent
this conversion. This update removes the conversion,
allowing the database to handle errors appropriately.

Fixed uptrace#624
@Aoang Aoang force-pushed the fix/pg-numeric-conversion branch from a3f2537 to ab3c679 Compare December 5, 2024 11:07
@j2gg0s
Copy link
Collaborator

j2gg0s commented Dec 6, 2024

I’d like to elaborate on the specific impacts.

Assume we use uint32 in a struct in Go, Bun will create table with Integer in Postgres.

type User struct {
        ID     int `bun:",pk"`
        Name   string
        Uint32 uint32
        Int64  int64
}
bun=# \d users;
                    Table "public.users"
 Column |       Type        | Collation | Nullable | Default
--------+-------------------+-----------+----------+---------
 id     | bigint            |           | not null |
 name   | character varying |           |          |
 uint32 | integer           |           |          |
 int64  | bigint            |           |          |
Indexes:
    "users_pkey" PRIMARY KEY, btree (id)

If we insert 1<<31+1, we got -2147483647.
But we can got it by 1<<31+1.

	users := []*User{
		{ID: 1, Name: "user 1", Uint32: 1<<31 - 1},
		{ID: 2, Name: "user 2", Uint32: 1<<31 + 1},
	}
	if _, err := db.NewInsert().Model(&users).Exec(ctx); err != nil {
		panic(err)
	}

	{
		user := User{}
		if err := db.NewSelect().Model(&user).Where("uint32 = ?", uint32(1<<31+1)).Scan(ctx); err != nil {
			panic(err)
		}
		fmt.Println(user)
	}
[bun]  13:57:53.983   INSERT                  742µs  INSERT INTO "users" ("id", "name", "uint32", "int64") VALUES (1, 'user 1', 2147483647, 0), (2, 'user 2', -2147483647, 0)
[bun]  13:57:53.983   SELECT                  314µs  SELECT "user"."id", "user"."name", "user"."uint32", "user"."int64" FROM "users" AS "user" WHERE (uint32 = -2147483647)

With this PR, we will got error when insert:

[bun]  13:58:43.396   INSERT                1.541ms  INSERT INTO "users" ("id", "name", "uint32", "int64") VALUES (1, 'user 1', 2147483647, 0), (2, 'user 2', 2147483649, 0)    pgdriver.Error: ERROR: integer out of range (SQLSTATE=22003)

Although this may break some edge cases of runnable code, I still believe this is a good change.

@j2gg0s j2gg0s self-requested a review December 6, 2024 06:03
@j2gg0s j2gg0s self-assigned this Dec 6, 2024
@j2gg0s j2gg0s merged commit 014b142 into uptrace:master Dec 6, 2024
4 checks passed
@Aoang Aoang deleted the fix/pg-numeric-conversion branch December 6, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(pgdialect): uint32 field will overflow
2 participants