sqlc icon indicating copy to clipboard operation
sqlc copied to clipboard

Improve sqlc.embed Go field naming

Open sapk opened this issue 2 years ago • 15 comments

An attempt to fix #2686

It make the argument of sqlc.embed used as the name of the field name in Go struct. This allow join of the same table with custom name and not forcing to be incremental naming.

~This change can break previous models using sqlc.embed~

sapk avatar Sep 03 '23 21:09 sapk

I added the change under a configuration flag so that it isn't a breaking change by default. If you don't like the config flag I can revert the last commit.

sapk avatar Sep 12 '23 22:09 sapk

I rebased the PR on latest main and updated the tests results for the new version.

sapk avatar Sep 13 '23 23:09 sapk

@sapk any help needed here?

andrei-dascalu avatar Oct 09 '23 07:10 andrei-dascalu

This PR is ready just let me know if it need any change. The latest version, keep backward compatibility via a flag using by default the old logic.

From discussion in #2686 maybe you want me to remove this flag? or reverse the logic to set it as by default active?

sapk avatar Oct 09 '23 15:10 sapk

@sapk I was asking just because here it says there are conflicts so it can't be merged. Not sure who can approve, maybe @kyleconroy ?

andrei-dascalu avatar Oct 11 '23 07:10 andrei-dascalu

Yes I added a commit after to make it configurable and keep a backward compat by default.

sapk avatar Oct 11 '23 07:10 sapk

Some change in the struct of the configuration seems to have broken this MR. I will rebase this work on the new configuration logic.

sapk avatar Nov 05 '23 14:11 sapk

Some change in the struct of the configuration seems to have broken this MR. I will rebase this work on the new configuration logic.

Yes, sorry about that. We had to change a few things to support publishing sqlc-gen-go. All Go codegen configuration now lives inside of the codegen/golang package. The change means you won't need to change anything in the codegen plugin proto, and can avoid all the changes in config and shim.

andrewmbenton avatar Nov 06 '23 21:11 andrewmbenton

No worries. It is now easier to add (or remove) option. The PR should be up-to-date now.

sapk avatar Nov 07 '23 01:11 sapk

To resume, this PR allow to have returning struct relying on alias in sqlc.embed().:

type ListUserLinkRow struct {
	Owner    User
	Consumer User
}

instead of

type ListUserLinkRow struct {
	User   User
	User_2 User
}

for query like

-- name: ListUserLink :many
SELECT
    sqlc.embed(owner),
    sqlc.embed(consumer)
FROM
    user_links
    INNER JOIN users AS owner ON owner.id = user_links.owner_id
    INNER JOIN users AS consumer ON consumer.id = user_links.consumer_id;

We still get enumerate in case of duplicate even if the option is enabled:

-- name: Duplicate :one
SELECT sqlc.embed(users), sqlc.embed(users) FROM users;
type DuplicateRow struct {
	User   User
	User_2 User
}

sapk avatar Nov 07 '23 01:11 sapk

Before getting into code comments, I think @kyleconroy should weigh in on whether this is a good idea and if so whether we should just make this naming behavior the default and remove the option flag, or invert the flag to allow opting back in to the old behavior.

andrewmbenton avatar Nov 09 '23 00:11 andrewmbenton

@andrewmbenton I agree, that's why when I rebased the PR, I split it into two commits. One that implement the new logic and one that put it under a flag so that if you don't want the flag it can easily be removed.

sapk avatar Nov 09 '23 08:11 sapk

I updated the PR from main latest and added missing documentation for the potential new configuration.

sapk avatar Dec 28 '23 16:12 sapk

Hey everyone,

Any update about this? This feature is seriously needed because with the current behavior there is just no way to know what each embedded field represents.

Regarding backward compatibility, why not simply accept a second (optional) parameter that signifies the name? For example:

SELECT sqlc.embed(table1, 'MyCustomName')

Generates the following struct:

type S struct {
  MyCustomName Table1
}

All code that doesn't pass this second parameter simply follows the current behavior, so we don't break anyone's code.

bloeys avatar Aug 16 '24 19:08 bloeys

I updated the PR.

@bloeys I first go as backward compatibility with configuration and try to have a similar logic as named parameters so that the configuration could be removed after transition. But your idea is good and could be an other solution. I could implement it if it is preferred by project owners.

sapk avatar Aug 19 '24 15:08 sapk