search

Faithlife / FaithlifeData

Helpers for querying ADO.NET-compatible databases.
https://faithlife.github.io/FaithlifeData/
MIT License
6 stars 4 forks source link

Add stored procedure support #16

Closed TyMick closed 3 years ago

TyMick commented 3 years ago

Resolves #5.

The parameter I added to each DbConnector.Command overload is bool isStoredProcedure. Should I stick with that, or would it be better for the parameter to be CommandType commandType = CommandType.Text just in case you want to support additional command types in the future? There's only one other type, and it's only supported by OLE DB, but you never know.

This also adds a new property to DbConnectorCommand, public bool IsStoredProcedure. Similar to above, this could instead be implemented as public CommandType CommandType if desired.

And finally, these are the lines added to DoCreate:

if (IsStoredProcedure)
    command.CommandType = CommandType.StoredProcedure;

I wrote some unit tests, just to test whether IsStoredProcedure is being set correctly, but I'm not sure how to test these functionally, since your other tests use SQLite, which doesn't support stored procedures. Should I brainstorm a way to use a different SQL system to test this part, or not bother?

ejball commented 3 years ago

Should I brainstorm a way to use a different SQL system to test this part, or not bother?

I don't think we need to add another type of SQL database just to support this. If the SQLite provider throws an exception when stored procedures are used, ensuring that the exception is thrown would be a reasonable test.

TyMick commented 3 years ago

Thanks for your help!

While I was at it, I changed the text param hint on each StoredProcedure method to <param name="text">The name of the stored procedure.</param> to match how the IDbCommand.CommandType documentation describes it:

When you set the CommandType property to StoredProcedure, you should set the CommandText property to the name of the stored procedure.

How are these changes looking?