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

Update reflectx to allow for optional nested structs #950

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kszafran
Copy link

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

Compared to previous PRs:

  • Instead of halfway copying convertAssign from the standard library (which was done in a way that introduced incompatibilities), I used //go:linkname to call the original function directly. Normally, this isn't recommended. However, Go authors recognize that some libraries do that, and marked selected functions to avoid breaking changes, including convertAssign. See https://go.dev/issue/67401.
  • I simplified the code by replacing the mutable ObjectContext with a basic func type.
  • I added tests for an optional struct inside an optional struct.

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

@kszafran
Copy link
Author

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
Copy link
Author

kszafran commented Oct 17, 2024

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.

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