-
Notifications
You must be signed in to change notification settings - Fork 510
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
Generated query files return row counts for simple mutators #4578
base: master
Are you sure you want to change the base?
Conversation
b8054fc
to
898868a
Compare
This can also somewhat fix #3238 and make the behaviour consistent |
But for backward compatibility, it should be behind some flag which will default to false. |
@morki What backwards compatibility are you concerned about? This doesn't change the query behavior, only the API of generated code (but only the return value, so should be source-compatible). |
@MariusVolkhart as I understand this, it can compile fine, but it changes the behaviour. From your example:
This now normally executes the query. But with your change, the same compiles fine, but does not execute the query. This code will execute the query:
Do I understand it right? |
@morki Oh, gotcha. No, that's not how this works. In the example, updatePlayerName(4, "Hero").executeAsOne()
However, in the changes in this PR, |
898868a
to
cf5dea6
Compare
@MariusVolkhart oh, thank you for the explanation and sorry for bad understanding. I was probably tired when quickly reviewing. |
I think my main concern is how this can leak implementation details of individual drivers. I don't know that all the supported drivers actually return the same thing for each of the mutator queries. I don't know the right way to solve it. SQLDelight would need some knowledge of how the driver works and choose to return the result if its the thing we want to expose, or run a separate query to get the last_change_id or changes() (in SQLite, IDK what it is for other dialects) so that its guaranteed to behave the same with different drivers Or we just let them go through as is and its up to the consumer to know what the driver returns, but I don't think thats right |
Simple mutators like INSERT, UPDATE, and DELETE, without RETURNING clauses, typically return the number of rows changed. In fact, the SqlDriver#execute() function already does this. However, the value of execute() is not exposed to the generated query files. This commit makes it so that simple INSERT, UPDATE, and DELETE statements return the QueryResult<Long> that the SqlDriver already returns.
I lean towards the "we just let them go through as is and it's up to the consumer to know what the driver returns" approach. Firstly, not exposing the return value limits the capabilities of SQLDelight relative to the underlying driver. Secondly, exposing the mutated count means that people can develop custom drivers for implementations (SQLJS, Sqlite) that do query Thirdly, developers using a driver that doesn't support this will surely either know, or quickly find out when they try to use it, that their driver doesn't support this functionality. |
cf5dea6
to
995f65e
Compare
Simple mutators like INSERT, UPDATE, and DELETE, without RETURNING clauses, typically return the number of rows changed. In fact, the SqlDriver#execute() function already does this. However, the value of execute() is not exposed to the generated query files.
This commit makes it so that simple INSERT, UPDATE, and DELETE statements return the QueryResult that the SqlDriver already returns.
Review Guide
In short, this commit turns
into
The main logic changes are in
ExecuteQueryGenerator
andQueryGenerator
.