Skip to content

Adding LastInsertId for Go #1484

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

Merged
merged 2 commits into from
Apr 3, 2022
Merged

Adding LastInsertId for Go #1484

merged 2 commits into from
Apr 3, 2022

Conversation

xeoncross
Copy link
Contributor

#1459 adds support for sql.Result.LastInsertId()

:execlastinsertid is calls .LastInsertId() rather than .RowsAffected() with :execrows.

This will avoid the sql.Result database package leaking across the boundary of the pkg.Querier interface generated by sqlc when using MySQL & MariaDB

@xeoncross xeoncross changed the title Adding LastInsertId for golang Adding LastInsertId for Go Mar 11, 2022
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 will avoid the sql.Result database package leaking across the boundary of the pkg.Querier interface generated by sqlc when using MySQL & MariaDB

I'd rather leak the sql.Result than add another command. The reason is that we already support :execresult. I think adding :execrows was a mistake, so I don't want to make the same mistake twice.

@xeoncross
Copy link
Contributor Author

Is there anything I can do to change your mind? The fact that the sql.Result leaks out of the models generated means an entire new translation layer must be added between the app and model to get res.LastInsertId() from the sql.Result when using MySQL. This pain does not exist in PostgreSQL, but a lot of companies still rely on MySQL and this means having to either write all the insert id fetching directly in the calling code - or add a model layer to the model layer to hide this extra step.

Can you share more about why :execrows was a mistake? Has that complicated the addition of new languages?

@kyleconroy
Copy link
Collaborator

Can you share more about why :execrows was a mistake?

In this case, you're right that it's not much more work to support. Also, there are only two methods on sql.Result, so we aren't going to have to add any more in the future.

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.

Since RowsAffected() maps to :execrows, let's map LastInsertId() to:execlastid.

Also, please add tests similar to exec_rows and exec_result.

@xeoncross
Copy link
Contributor Author

I tried to add the python generation example, but when I changed into the internal/endtoend/testdata/exec_lastid/python_postgresql folder and run it against my local sqlc it complained:

$ ../../../../../sqlc2 generate
> error parsing sqlc.json: unknown target langauge "python"

I was using the same code from the exec_rows example.

I also wasn't sure what the PGX (go_postgresql_pgx) should be since this isn't something for PostgreSQL - so I left it.

@kyleconroy
Copy link
Collaborator

It's okay that you weren't able to add the Python support, can do that in a subsequent PR.

You're also right that we don't need to add support for pgx, as it's Result type does not have this method.

@kyleconroy kyleconroy merged commit 2d8892b into sqlc-dev:main Apr 3, 2022
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.

3 participants