Skip to content

Support $ as a parameter prefix#1952

Merged
mgravell merged 1 commit into
DapperLib:mainfrom
Giorgi:main-1
Sep 3, 2023
Merged

Support $ as a parameter prefix#1952
mgravell merged 1 commit into
DapperLib:mainfrom
Giorgi:main-1

Conversation

@Giorgi

@Giorgi Giorgi commented Aug 23, 2023

Copy link
Copy Markdown
Contributor

This PR adds support for $ as a parameter prefix. I made the changes as described in this comment: #1687 (comment)

I need support for $ because I'm building ADO.NET Provider for DuckDB and DuckDB uses $: Add support for named parameters in prepared statements

@mgravell

mgravell commented Aug 23, 2023

Copy link
Copy Markdown
Member

Does the existing pseudo-positional parameter support not work? By that I mean: looking at DuckDB, it seems to use positional rather than named parameters - and the ? isn't a prefix - it is the entire thing. Dapper can't work well with that by itself, because it doesn't know how to order things - however, Dapper supports a syntax variant:

where Name = ?name?

which it rewrites as

where Name = ?

but uses the token name to figure out which parameter to add in that position. This works with multiple providers, and it looks like it should work here, without any other changes?

@mgravell

Copy link
Copy Markdown
Member

Secondary: is there any integration test possible that would should this proposed change actually working against DuckDB? I don't know how DuckDB deploys in terms of whether it can work on the CI server, but something that can at least run on a dev box is desirable.

@Giorgi

Giorgi commented Aug 23, 2023

Copy link
Copy Markdown
Contributor Author

By that I mean: looking at DuckDB, it seems to use positional rather than named parameters

This is not correct, the upcoming version of DuckDB (0.9.0, scheduled to be released in September) will support named parameters too. This PR added support for named parameters: Add support for named parameters in prepared statements

As for how to run tests against DuckDB, DuckDB is an embedded database and you don't need to deploy anything. The simplest option is to clone the https://github.com/Giorgi/DuckDB.NET repo, checkout Named-Parameters branch and run dotnet build /p:BuildType=Full BuildType=Full tells the build to fetch necessary DuckDB binaries so you don't need to do anything else.

Inside the ParameterCollectionTests.cs file, you will find this commented assertion:

//connection.Execute(queryStatement, new { foo = 1, bar = "test" }).Should().BeGreaterOrEqualTo(1, "Passing parameters as object should work");

The changes in this PR make that assertion successful.

@mgravell mgravell merged commit 8a68070 into DapperLib:main Sep 3, 2023
@Giorgi

Giorgi commented Oct 2, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for merging the PR. Do you want me to add test cases to Dapper that use DuckDB and test the new prefix?

@Giorgi

Giorgi commented Oct 2, 2023

Copy link
Copy Markdown
Contributor Author

I already have a test case for it in my repo: Giorgi/DuckDB.NET@c5c2e5d

@mgravell

mgravell commented Oct 2, 2023

Copy link
Copy Markdown
Member

Ideally "yes", but the real question is: how do we execute those tests? What is required to spin up a duckdb instance? Can it run in CI, or is it dev machine only?

@Giorgi Giorgi mentioned this pull request Oct 3, 2023
@mgravell

Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants