SOCI / soci

Official repository of the SOCI - The C++ Database Access Library
http://soci.sourceforge.net/
Boost Software License 1.0
1.41k stars 477 forks source link

Added two sqlite3 tests showing binding placeholders. (only one 'use' … #961

Closed ccosmin closed 2 years ago

ccosmin commented 2 years ago
  1. only one 'use' necessary for same name, even if multiple instances of placeholder
  2. another test demonstrating exception in case of multiple 'use' for same placeholder name, but different instance
vadz commented 2 years ago

Thanks, there is clearly an inconsistency here between SQLite and the other backends. The trouble is that I don't think we can easily implement support for using multiple placeholders with the same name with the same exchange variable for the other backends, can we?

OTOH I think we could support using the same placeholder with multiple variables for SQLite by just rewriting the query and using different names for the subsequent occurrences of it. Do you think it would be worth doing this?

ccosmin commented 2 years ago

I think I'd just modify the sqlite3 backend sqlite3_statement_backend::bind_and_execute, re-writing the query seems more difficult.

Last use() would be the one which is bound for a particular placeholder. In any case, I imagine, current code would use the same value for the different instances of the same placeholder so that shouldn't be an issue.

vadz commented 2 years ago

Sure, this would be more efficient if nothing else, so if it can be done like this, I'm all for it.

We probably shouldn't merge this PR until it is done, right? I.e. the upcoming change will change the behaviour and hence invalidate the tests being added here.

ccosmin commented 2 years ago

I checked the code. If the caller has used the use() overload with a string name for the parameter, then the sqlite3_bind_parameter_index is called and the correct position of the parameter is used.

However, if no actual name was associated in use() then there's no way to know the actual position of the placeholder as recognized by sqlite. Best I can do is to ignore additional use() that go past the sqlite3 recognized range. This solution obviously is not satisfactory for cases like:

insert into test(first, last, email) values(:first, :first, :email)
use("john"), use("john"), use("j@x.com")

Then the email parameter will just be ignored. However, if the user wrote code like the following:

insert into test(first, last, email) values(:first, :first, :email)
use("john", "first"), use("john", "first"), use("j@x.com", "email")

Then the correct thing is done.

So all in all two possible solutions:

  1. ignore parameters that overflow
  2. fix caller code to have proper named parameters in use().
vadz commented 2 years ago

I thought about maintaining a map string -> index and looking up the index in it before allocating the next one. Could this work?

ccosmin commented 2 years ago

Hmm, so you mean parse the sqlite query string? Because what would be needed to solve this is a mapping position-in-query -> position-recognized-by-sqlite. If you have the name in use() sqlite gives you the “true” bind position.

Sent from my iPhone

On 3 May 2022, at 16:38, VZ @.***> wrote:

 I thought about maintaining a map string -> index and looking up the index in it before allocating the next one. Could this work?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

vadz commented 2 years ago

Sorry, I've somehow forgot about this one. I'm not sure if you've found anything better to do since then. Do we still want to merge this one?