feat: no ErrNoRows when scan nil struct ptr and nil map and nil builtin type ptr - #1379
feat: no ErrNoRows when scan nil struct ptr and nil map and nil builtin type ptr#1379CyJaySong wants to merge 4 commits into
Conversation
|
@Aoang Hi Bro,look here |
1df5a46 to
c7b489d
Compare
c7b489d to
9e80321
Compare
a02c4b2 to
385b0a6
Compare
|
@CyJaySong I'll take another pass at the PR tomorrow, just to make sure I understand the changes well. Thanks for your patience and the effort you're putting in contributing to Bun. |
|
@bevzzz Thanks for your recognition. I‘m looking forward to a smooth merger |
|
@bevzzz I am waiting in agony |
|
prolong life |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. If there is no update within the next 7 days, this pr will be closed. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions! |
|
prolong life |
Aoang
left a comment
There was a problem hiding this comment.
Thanks for your contribution, and sorry for the long review time!
So far, I've spotted two breaking changes, and it's still unclear if there are others.
Like if err := query.Scan(ctx, &id); errors.Is(err, sql.ErrNoRows) {}, upgrading will break application logic since sql.ErrNoRows won't be triggered anymore. This is a tough breaking change to introduce. At a minimum, this feature should be optional/opt-in, or exposed via a separate API like ScanSilent.
To push this forward, I suggest we discuss the design first in an issue to find a reasonable solution. Meanwhile, this PR remains very valuable, especially as it points out gaps in our regression test suite.
Refactor the reflect.Pointer branch in _newModel to use isSingleValueStruct so that time.Time, net.IPNet, netip.Addr, netip.Prefix and sql.Scanner implementers are routed to scanModel instead of structTableModel. Also apply the same check to the reflect.Slice branch to handle []*time.Time, []*sql.NullString and similar pointer slices correctly. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Thanks for the update. I retested the latest commit against PostgreSQL. The main backward-compatibility concern is still unresolved. Existing applications may already rely on func testPR1379ErrNoRows(t *testing.T, db *bun.DB) {
t.Run("nil pointer destination", func(t *testing.T) {
var got *int
err := db.NewSelect().
ColumnExpr("1").
Where("FALSE").
Scan(ctx, &got)
require.ErrorIs(t, err, sql.ErrNoRows)
require.Nil(t, got)
})
t.Run("allocated pointer destination", func(t *testing.T) {
got := new(int)
err := db.NewSelect().
ColumnExpr("1").
Where("FALSE").
Scan(ctx, &got)
require.ErrorIs(t, err, sql.ErrNoRows)
require.NotNil(t, got)
})
} |
@Aoang Oh, that was by design, not a bug. It fixes the inconsistent no raws handling behavior. I think this counts as a breaking change. |
|
@Aoang Hi, Bro, do you agree with my idea? |
Thanks for your contribution! There's no doubt that your implementation provides a much more elegant and reasonable way to handle things. Looking at the evolution of Go ORM ecosystems—from early days like xorm and gorm to tools like sqlx—none of them handled sql.ErrNoRows as cleanly as this approach does. My main concern, however, is that this is a breaking change that might force users to make extensive refactoring when upgrading to the new version. I did a quick survey of how users currently write code with bun: while most codebases won't be affected, there are still instances where incorrect usages slipped through due to a lack of code review (or oversight during review). These usages work fine in the current version, but this change would break them. Let's wait for feedback from other maintainers to see what they think. |
previous writing styles
new feature
Compatible with previous writing styles