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

UpdateQuery: marshal false as BOOLEAN if nullzero tag not set #937

Merged
merged 5 commits into from
Jan 10, 2024

Conversation

bevzzz
Copy link
Collaborator

@bevzzz bevzzz commented Nov 29, 2023

Related to: #922, see this comment for detailed analysis. This commit demonstrates the bug.

Setting NullZero = true for fields with default:x tag led to false values being appended as NULL in UpdateQuery.

bun/schema/table.go

Lines 378 to 381 in 8a43835

if s, ok := tag.Option("default"); ok {
field.SQLDefault = s
field.NullZero = true
}

Apart from causing the bug, this seemed like a misuse of NullZero property. Removing this behaviour required additional (small) changes to InsertQuery, so I also added new test cases and extended an existing one to make sure we don't introduce any regressions (hence the many changed files).

Finally, I moved the chunky f.IsPtr && f.HasNilValue(strct), f.HasZeroValue(strct) && (f.NullZero || f.SQLDefault != "") conditional to a separate method and added some documentation for it.

@vmihailenco
Copy link
Member

@bevzzz could you please rebase this?

@bevzzz bevzzz force-pushed the fix/922-default-true-insert-false branch from adcb2c6 to 3696bd4 Compare January 7, 2024 12:21
@bevzzz
Copy link
Collaborator Author

bevzzz commented Jan 10, 2024

There will be some more conflicts here, because of the test cases added in the other PR we've just merged.

I'll rebase later today 👌

@vmihailenco
Copy link
Member

@bevzzz thanks 👍 Sorry for the hassle.

@bevzzz bevzzz force-pushed the fix/922-default-true-insert-false branch from d45e285 to 176c86f Compare January 10, 2024 09:07
@bevzzz
Copy link
Collaborator Author

bevzzz commented Jan 10, 2024

No problem, glad I could contribute :)

@vmihailenco vmihailenco merged commit f6999e1 into uptrace:master Jan 10, 2024
3 checks passed
@bevzzz bevzzz deleted the fix/922-default-true-insert-false branch August 11, 2024 11:34
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.

2 participants