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

omitempty bug in InsertRecords #24

Open
ScreamingHawk opened this issue Nov 4, 2024 · 6 comments
Open

omitempty bug in InsertRecords #24

ScreamingHawk opened this issue Nov 4, 2024 · 6 comments

Comments

@ScreamingHawk
Copy link

ScreamingHawk commented Nov 4, 2024

When you have a table with a field tagged omitempty, calling InsertRecords will fail if some records have empty values and some do not. The error is pgkit: ERROR: VALUES lists must all be the same length (SQLSTATE 42601).

This is because the reflection (correctly) ignores the fields tagged omitempty, which results in different fields being supplied for the insert statement.

https://github.com/goware/pgkit/blob/master/builder.go#L51
https://github.com/goware/pgkit/blob/master/mapper.go#L41

Example test case: #23

@david-littlefarmer
Copy link
Member

This doesn't make much sense to me:

type Account struct {
	...
	AltName   *string   `db:"alt_name,omitempty"` // can be NULL
	...
}

When you have pointer to a string and nullable field in DB, then with omitempty you wouldn't be ever able to set the field in DB from some value to NULL.

I think this is valid definition.

type Account struct {
	...
	AltName   *string   `db:"alt_name"` // can be NULL
	...
}

So never to have pointer and omitempty on one field.

Also i think that omitempty is used only for values, that are set by DB, as created at timestamp or/and id as primary key assigned by DB.

Maybe i am completely wrong 😅

But yes, this is still a bug i guess.

@VojtechVitek
Copy link
Member

@david-littlefarmer what if you want to store empty string?

@david-littlefarmer
Copy link
Member

You mean like this?

str := ""
o.AltName = &str

@LukasJenicek
Copy link
Contributor

If I wanted store empty string then I would never made the column nullable and it should be required at least empty string.

So in the code it would not be pointer as well.

@ScreamingHawk
Copy link
Author

I've updated the example to something more realistic. Setting created_at manually for one and expecting the db to populate with DEFAULT CURRENT_TIMESTAMP on the other.

@pkieltyka
Copy link
Member

can you guys check the https://github.com/goware/pgkit/blob/master/tests/pgkit_test.go to see if the case is covered..? maybe its just more confusing on how to use it, versus a bug..?

as well, if you do determine a bug, please add a failing test case in a PR, the test suite is pretty good and easy to work with, and this is the schema btw: https://github.com/goware/pgkit/blob/master/tests/schema_test.go

once we have a failing test case, we can look

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

No branches or pull requests

5 participants