Skip to content

feat: Add embedded structs based on result match #984

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

feat: Add embedded structs based on result match #984

wants to merge 8 commits into from

Conversation

kylecarbs
Copy link
Contributor

@kylecarbs kylecarbs commented Apr 26, 2021 •
edited
Loading

Identifies every column with a returned struct for queries.

This is a breaking change. I'm very open to comments, ideas, or reasons why this shouldn't even happen. To prevent breakage we could move this under a configuration option, and eventually migrate to default in a major version.

-- schema.sql
CREATE TABLE users (id text, email text);
CREATE TABLE usernames (user_id text, name text);

-- query.sql
-- name: GetUsers :many
SELECT * FROM users;

-- name: GetUsersWithUsername :many
SELECT users.*, usernames.name FROM users JOIN usernames ON users.id = usernames.user_id;

Before:

type GetUsersWithUsernameRow struct {
	ID    sql.NullString `db:"id"`
	Email sql.NullString `db:"email"`
	Name  sql.NullString `db:"name"`
}

After:

type GetUsersWithUsernameRow struct {
	User
	Name sql.NullString `db:"name"`
}

@kylecarbs kylecarbs marked this pull request as ready for review April 26, 2021 04:13
@cmoog
Copy link
Contributor

cmoog commented Apr 26, 2021

Run make regen at the root to regenerate the expected output for e2e tests.

@cmoog

This comment has been minimized.

Sign in to view
@cmoog
Copy link
Contributor

cmoog commented Apr 26, 2021 •
edited
Loading

Didn't realize the join handling hadn't reached master, but this will be a problem when #983 lands.

Handling fields that are nullable after joins, but not nullable in the data model will be difficult with this approach.

@kylecarbs
Copy link
Contributor Author

The embedded struct shouldn't be nullable though... right? It doesn't represent any data itself, simply a container for data.

@cmoog
Copy link
Contributor

cmoog commented Apr 26, 2021 •
edited
Loading

Correct. But the problem comes when Foo.A is not null in the data model, but can be null after a left join. When we marshal a possibly nullable field to Foo.A directly this nullability type information is lost. Instead we'd want to use sql.NullString for Foo.A in that particular query. More context here: #983

After those PRs land, adding proper handling for each join case to this PR will be straightforward. It's possible those cases will be automatically taken care of by #983 before this code path is even reached, not quite sure implementation wise.

@kylecarbs
Copy link
Contributor Author

Ahh understood.

@cmoog
Copy link
Contributor

cmoog commented Jun 17, 2021

bump

kyleconroy
kyleconroy requested changes Jun 28, 2021
View reviewed changes
Copy link
Collaborator

@kyleconroy kyleconroy left a comment

Choose a reason for hiding this comment

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

This PR is now out of date, there are merge conflicts.

Also, I'm not sure how you're planning on solving the LEFT JOIN issue. You can't use the existing structs because a non-nullable column can now be null. I think you'll need to generate new structs, which may defeat the entire purpose. Maybe we just don't allow embedding to work when using left joins?

Lastly, the syntax for this needs to be a special sqlc function. I'd suggest using sqlc.embed(). The reason is that overloading the table.* method isn't backwards compatible.

@kyleconroy
Copy link
Collaborator

@kylecarbs I haven't heard from you in a while, so I'm going to close this out. If you end up working on this, please open a new pull request or push to this one and mention me in a comment.

@kyleconroy kyleconroy closed this Sep 12, 2021
@ludusrusso
Copy link
Contributor

@kylecarbs I'm interesting in this feature. If you are not working on this anymore I can take it!

@kylecarbs
Copy link
Contributor Author

Feel free! I'd love to tackle it but don't quite have bandwidth at the moment.

@BrunoQuaresma BrunoQuaresma mentioned this pull request Apr 26, 2022
Merged
@nickjackson nickjackson mentioned this pull request May 11, 2022
Merged
@Emyrk Emyrk mentioned this pull request May 12, 2022
Closed
@kyleconroy kyleconroy mentioned this pull request May 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyleconroy kyleconroy kyleconroy requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kylecarbs @cmoog @kyleconroy @ludusrusso