Giorgi / DuckDB.NET

Bindings and ADO.NET Provider for DuckDB
https://duckdb.net
MIT License
338 stars 61 forks source link

Ignore unused query parameters #159

Closed roee88 closed 7 months ago

roee88 commented 7 months ago

Ignore unused parameters instead of throwing an InvalidOperationException.

Closes #158

Unchanged behavior:

  1. If there are less parameters than expected, then InvalidOperationException: Invalid number of parameters is thrown. Covered by existing MissingParametersThrowsException test.
  2. If there are unbound parameters, then DuckDB.NET.Data.DuckDBException : Invalid Input Error: Values were not provided for the following prepared statement parameters: is thrown by duckdb.
coveralls commented 7 months ago

Pull Request Test Coverage Report for Build 6841346439


Files with Coverage Reduction New Missed Lines %
DuckDB.NET.Data/DuckDBCommand.cs 6 79.1%
<!-- Total: 6 -->
Totals Coverage Status
Change from base Build 6836291971: -0.3%
Covered Lines: 1140
Relevant Lines: 1293

💛 - Coveralls
Giorgi commented 7 months ago

This looks good but does the following test pass? It passes with SQLite:

using var connection = new DuckDBConnection("DataSource=:memory:");
connection.Open();

var command = connection.CreateCommand();
command.CommandText = "SELECT :used";
command.Parameters.Add(new DuckDBParameter("unused", 24));
command.Parameters.Add(new DuckDBParameter("used", 42));
var scalar = command.ExecuteScalar();
scalar.Should().Be(42);
roee88 commented 7 months ago

Did you mean SELECT $used? (if so then yes the test pass; with :used I get syntax error unrelated to this PR)

Do you want to replace BindUnreferencedNamedParameterTest to that test or to add another one?

Giorgi commented 7 months ago

Yes, I meant $used and please add that test case.

Giorgi commented 7 months ago

Also, since you are adding new test cases it would be great if you inherit ParameterCollectionTest from DuckDBTestBase and use the connection and command provided by the base class.

roee88 commented 7 months ago

It's a class fixture, are you sure you want the shared command between all tests? Seems more error prone (e.g., will need to clear Command.Parameters in all the tests). I'm not familiar with xunit so if that's not the case then sure :)

Giorgi commented 7 months ago

It's a class fixture but every test class is a separate instance so the state will be shared by only tests inside the same class.

You can either override the Dispose method to clear parameters or add it in the DuckDBTestBase Dispose method.

Giorgi commented 7 months ago

Thanks @roee88