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 #847

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

Conversation

ntbosscher
Copy link

Nested structs are now only instantiated when one of the database columns in that nested struct is not nil. This allows objects scanned in left/outer joins to keep their natural types (instead of setting everything to NullableX).

Example:

select house.id, owner.*,
from house
left join owner on owner.id = house.owner

Before

type House struct {
 ID      int
 Owner   *Person // if left join gives nulls, Owner is set
}

type Owner struct {
 ID sql.NullInt64 // make nullable columns even tho if table doesn't have those columns nullable
}

After

type House struct {
 ID      int
 Owner   *Person // if left join gives nulls, Owner will be nil
}

type Owner struct {
 ID int    // no need to set this to sql.NullInt
}

Nested structs are now only instantiated when one of the database columns in that nested struct is not nil. This allows objects scanned in left/outer joins to keep their natural types (instead of setting everything to NullableX).

Example:
```sql
select house.id, owner.*,
from house
left join owner on owner.id = house.owner
```

``` golang
type House struct {
 ID      int
 Owner   *Person // if left join gives nulls, Owner will be nil
}

type Owner struct {
 ID int    // no need to set this to sql.NullInt
}
```
@ntbosscher
Copy link
Author

A possible solution for #162

@ntbosscher
Copy link
Author

One trade off is that for nested structs, we end up doing most of the work in sql.convertAssignRows twice

This also further complicates reflectx.

However, I think the functionality is more natural. With our modeling we typically do things like the following.

type User struct {
    ID int
    Name string
}

type Plan struct {
    ID int
    CreditsPerMonth int64
}

type UserPlan struct {
   User User
   Plan *Plan
}

This maps directly to database tables. Doing things like setting Plan's properties to nullable fields feels weird and out of place.

@ntbosscher
Copy link
Author

Happy to make any adjustments if you see a better way to do this.

@Omar-V2
Copy link

Omar-V2 commented Mar 10, 2023

This makes a lot of sense, I ran into issues with the existing behaviour recently. I don't like that I have to make a bunch of fields Nullable on existing structs just so they can be properly scanned when performing a left join. Any updates from the team if this can get merged? @jmoiron? Thanks!

@maranqz
Copy link

maranqz commented May 5, 2023

@jmoiron, Could you check and merge it? Thanks. 🙏

@phelian
Copy link

phelian commented Jul 6, 2023

Another vote for this

Copy link

@geeeeeeeeek geeeeeeeeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and it works with multi outer joins. Vote to merge this PR.

Copy link

@geeeeeeeeek geeeeeeeeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait it doesn't work with pointer types: don't know how to parse type int64 -> **uint64

type AccountRow struct {
	Account     `db:"account"`
	OptionOrder `db:"option_order"`
}
type OptionOrder struct {
	ID        uuid.UUID `db:"id"`
	AccountID *uint64   `db:"account_id"`
}

@dlsniper
Copy link
Collaborator

dlsniper commented Feb 1, 2024

Hello, @ardan-bkennedy, and I recently stepped in to help maintain this project.
We are sorting the opened issues and pull requests and would like to know if you still NEED this merged.
I think that #900 might solve some issues with this PR and so I'm tempted to close this in favor of that PR.
Thank you for your contribution.

@dlsniper dlsniper added needs testing The PR needs more testing before being accepted requires attention The PR looks like it could potentially break things. Definitely requires more testing labels Feb 1, 2024
ardihikaru added a commit to developersismedika/sqlx that referenced this pull request Feb 5, 2024
Update with latest changes from origin/master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs testing The PR needs more testing before being accepted requires attention The PR looks like it could potentially break things. Definitely requires more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants