search

FirebirdSQL / NETProvider

Firebird ADO.NET Data Provider
https://www.firebirdsql.org/en/net-provider/
Other
159 stars 65 forks source link

Added AppendStoredProcedureCall #1185

Open Camel-RD opened 2 months ago

Camel-RD commented 2 months ago

Added AppendStoredProcedureCall override to support InsertUsingStoredProcedure, UpdateUsingStoredProcedure, DeleteUsingStoredProcedure in model builder.

cincuranet commented 1 month ago

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 in E2E tests.

Camel-RD commented 1 month ago

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 commented 1 month ago

AppendStoredProcedureCall fixed and tests added

Camel-RD commented 4 weeks ago

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 commented 3 days ago

Not sure why you added commit 53fccb6 , but that looks unrelated and needs to be removed.

cincuranet commented 3 days ago

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.

I don't follow. Can you elaborate?

Camel-RD commented 3 days ago

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.

I don't follow. Can you elaborate?

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 commented 3 days ago

Not to use explicit names is how it's done in SQL Server implementation.

SQL Server implementation uses EXEC and SQL Server does not have selectable stored procedures.

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.

Yes. But same can be said about the whole SP structure. It needs to fit into what EF expects.

Camel-RD commented 3 days ago

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.

SQL Server implementation uses EXEC and SQL Server does not have selectable stored procedures.

Hm, SQLServer stored procedures have 3 ways to return values including as selectable procedure.

Yes. But same can be said about the whole SP structure. It needs to fit into what EF expects.

Even if EF Core documentation is lacking, its possible to gues what EF is expecting from exceptions it throws.