Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork74
Added AppendStoredProcedureCall#1185
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
cincuranet commentedAug 26, 2024
Sorry for late reply, I was busy with work. Can you maybe add some simple tests for calling insert/update/delete SPs, just to have basic validation that the SQL is correct. Something similar to what is inE2E tests. |
Camel-RD commentedAug 27, 2024
It looks like EXECUTE PROCEDURE SP_ABC p1, p2 will not work properly with EF concurrency checks. Will have to change it to SELECT * FROM SP_ABC(p1, p2, p3). |
Camel-RD commentedAug 28, 2024
AppendStoredProcedureCall fixed and tests added |
cincuranet left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Some minor pieces, otherwise LGTM in general.
src/FirebirdSql.EntityFrameworkCore.Firebird.Tests/EndToEndUsingSP/DeleteTestsUsingSP.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/FirebirdSql.EntityFrameworkCore.Firebird.Tests/EndToEndUsingSP/DeleteTestsUsingSP.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/FirebirdSql.EntityFrameworkCore.Firebird.Tests/EndToEndUsingSP/DeleteTestsUsingSP.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/FirebirdSql.EntityFrameworkCore.Firebird.Tests/EndToEndUsingSP/InsertTestsUsingSP.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/FirebirdSql.EntityFrameworkCore.Firebird.Tests/EndToEndUsingSP/InsertTestsUsingSP.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/FirebirdSql.EntityFrameworkCore.Firebird.Tests/EndToEndUsingSP/UpdateTestsUsingSP.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/FirebirdSql.EntityFrameworkCore.Firebird.Tests/EndToEndUsingSP/UpdateTestsUsingSP.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/FirebirdSql.EntityFrameworkCore.Firebird.Tests/EndToEndUsingSP/UpdateTestsUsingSP.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/FirebirdSql.EntityFrameworkCore.Firebird.Tests/EndToEndUsingSP/UpdateTestsUsingSP.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| } | ||
| commandStringBuilder.Append("SELECT * FROM "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would prefer here to be explicit about column names.
Camel-RD commentedAug 30, 2024
Explicit column names means that we cannot freely choose the results column names in SPs, but we can pick names for input parameters. Also, EFC uses result values by their positions and ignores any given result column names. |
cincuranet commentedSep 25, 2024
Not sure why you added commit53fccb6 , but that looks unrelated and needs to be removed. |
cincuranet commentedSep 25, 2024
I don't follow. Can you elaborate? |
This reverts commit53fccb6.
Camel-RD commentedSep 25, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Not to use explicit names is how it's done in SQL Server implementation. If explicit column names are used, there would also be an issue with naming the column for the rows affected value. The name could be ROWSCOUNT, but it should be properly documented so developers writing stored procedures can easily find this information. |
cincuranet commentedSep 25, 2024
SQL Server implementation uses
Yes. But same can be said about the whole SP structure. It needs to fit into what EF expects. |
Camel-RD commentedSep 25, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Updated the code to use explicit column names. And i saw some additional issues with this AppendStoredProcedureCall code. When stored procedure doesn't have any return values defined SELECT * FROM SP() will produce an SQL error. In this case EXECUTE PROCEDURE should be used. Also AppendStoredProcedureCall code should probably throw some exceptions when output parameters or return values are defined. Made changes to fix these issues and added a test for case when ID isn't auto generetad and insert procedure doesn't return any values.
Hm, SQLServer stored procedures have 3 ways to return values including as selectable procedure.
Even if EF Core documentation is lacking, its possible to gues what EF is expecting from exceptions it throws. |
Added AppendStoredProcedureCall override to support InsertUsingStoredProcedure, UpdateUsingStoredProcedure, DeleteUsingStoredProcedure in model builder.