jmoiron / sqlx

general purpose extensions to golang's database/sql
http://jmoiron.github.io/sqlx/
MIT License
16.3k stars 1.09k forks source link

Update reflectx to allow for optional nested structs #950

Open kszafran opened 1 month ago

kszafran commented 1 month ago

Based on #847 and #900, but simplified and fully compatible with the standard library scanning behavior.

Compared to previous PRs:

All in all, the changes are now much more succinct, which I hope will help get this PR reviewed and merged.

kszafran commented 1 month ago

Note that this is a minor breaking change. If someone scans into a struct that has a struct pointer field and all the fields in the nested struct can accept NULL values, this would change the observed behavior. For example:

type Inner struct {
    A *string
    B *int64
}
type Outer struct {
    Inner *Inner
}

Previously, when NULL was scanned into the fields of the nested struct, the nested struct would still be present (outer.Inner would be &Inner{}). Now it'll be nil.

Moreover, if someone has custom Scanner implementations that handle nil in some non-standard way (e.g. by initializing the value somehow), this will affect them. Such scanners won't be called on fields inside nested structs (including non-optional nested structs!) until the first non-NULL value is encountered. So, for example:

type JSON json.RawMessage

func (j *JSON) Scan(src any) error {
    if src == nil {
        *j = "{}"
        return
    }
    ...
}

type Outer struct {
    Inner struct {
        Stuff JSON
        Thing string
    }
}

Scan on Stuff wouldn't be called with a NULL value. But if we reorder fields:

Thing string
Stuff JSON

then it would be called (assuming we're scanning non-NULL into Thing)...

While there are different ways we could go about it, I'm thinking the safest approach would be to make this feature opt-in, e.g. with an omitempty attribute:

type Outer struct {
    Inner *Inner `db:inner,omitempty`
}

If there's no omitempty attribute, we'd keep the old behavior. That way this change would be fully backwards compatible, plus adding an explicit omitempty would make the intended behavior clear. It might complicate the implementation a little bit (especially for deeply nested structs), but I think it would make this change overall safer.

Let me know what you think, and I could update this PR.

kszafran commented 1 month ago

BTW, note that this feature makes it possible to have optional nested structs without the use of pointers, too. For example:

type Inner struct {
    A string
    B int64
}
type Outer struct {
    Inner Inner
}

Normally, if I used a LEFT JOIN and a and b would be NULL, scanning would fail here, because NULL cannot be decoded into string or int64. However, with this feature (and the omitempty attribute), scanning would succeed, simply leaving the Inner struct empty. From that point of view, the support for optional nested structs is even more flexible, and this would be another argument to make the behavior configurable with omitempty. We'd just need to document well what omitempty actually does.