Fixed propagation of NULL indicators in the soci::values::set() function

[Innokentiy-Alaytsev]

Originally, the indicator passed by the user was copied into a stored-for-later variable. This value could have been overwritten during conversion of the bound parameter to its base type. The fixed code preserves the value of an indicator.

[Innokentiy-Alaytsev]

New tests are failing because there is an issue with query use parameters processing downstream from statement_impl::do_add_query_parameters(). From what I've seen, the problem is that position-bound values from the soci::values use parameter are appended to the uses vector, resulting in excess items. This results in an exception when matching values with query parameters. The problem can be resolved by setting query context logging mode to log_context::never (log_context::on_error might work, but I'm not sure about the behaviour in the case of an error).

I'll look into it.

[Innokentiy-Alaytsev]

Unfortunately, some fixes required changes to SOCI API. Nothing was removed, one member function was added. Technically, this PR fixes 2 problems instead of the one in the title. I can split the PR into 2 separate PRs if it is the way.

[vadz]

Sorry for the delay with getting to this one. After looking at it, I agree with the main idea, i.e. we do need to preserve user-provided null indicators, but I'm not sure what exactly is_bindable() means, exactly, and why does it need to be false for values?

Even if the changes are probably correct, I'd like to understand them before applying and, ideally, have this explained in the comments in the sources.

Could you please try to explain this in more details? TIA!

[Innokentiy-Alaytsev]

The name is_bindable might not be the best. The flag solves 2 issues:

1. It determines whether the use variable should be directly processed when getting its name in statement_impl::do_add_query_parameters(). If soci::values variable is present, its contents are added to the soci::statement_impl::uses_ array, resulting in the size of the array being greater than the actual number of bound variables: 1 (for soci::values use variable) + number of values in soci::values. When the statement_impl::do_add_query_parameters() function iterates through use variables is uses_ and binds them to the named query parameters by position, a buffer overrun occurs if the number of query parameters is equal to the number of values in soci::values. Returning false from is_bindable() for soci::values allows to skip such use variables and only process their contents.

2. It allows properly binding values in multiple soci::values to the query parameters when binding by position. When soci::statement_impl::bind(soci::values&) bind the values in soci::values by position, it doesn't take into account the fact, that soci::values variables themselves are not bound to any query parameter and instead serve as a proxy for passing multiple parameters. When the position of the query parameter is determined, the current size of soci::statement_impl::uses_ is used ad newly bound parameter position. However, as soci::values variables are also stored in uses_, its size will always be greater then <the total number of positionally bound use variables> + <total number of values in all soci::values variables> by the number of soci::values variables. Counting the use variables, that have is_bindable() returning true allows to filter out soci::values binds and keep the binding position correct.

I hope it makes sense.

The context where I discovered the issue with positional binding might be outside of the expected use-case for soci:values. I'm developing an extension for using SOCI to access DB from GDScript in Godot game engine (in fact, I deem it ready for battle testing). Among other things, I decided to allow binding untyped arrays (as opposed to typed arrays, e.g. Array[ int ] for array of integers) as tuples with positional element binding through soci::values:

# From unit tests
const INSERT_QUERY := \
	"""
	INSERT INTO test_02_query_insert
	VALUES (:id, :string_val, :bool_val, :real_val);
	"""
			
const ARRAY_NOT_NULL := [ 1, "From Array, with NULL", true, null ]

result = (
		sql.new_query (INSERT_QUERY)
				.use (ARRAY_NOT_NULL)
				.execute ()
)

I reckon, it should work just fine for simple one-off queries, time will tell. Even if it's a questionable approach, the problem stands: positional binding of values through soci::values seems to be currently broken.

[vadz]

Thanks for the explanation!

To be honest, I'd consider just forbidding uses values and positional binding together because it doesn't seem to make much sense and we can be sure that nobody is using it, knowing that it doesn't work... But if you consider it useful enough to keep it, let's merge this PR.

I still dislike the name of the new function, but I have trouble coming up with something better. Maybe we could invert the function semantics and call it something like should_skip()?

[Innokentiy-Alaytsev]

To be honest, I'd consider just forbidding uses values and positional binding together because it doesn't seem to make much sense and we can be sure that nobody is using it, knowing that it doesn't work... But if you consider it useful enough to keep it, let's merge this PR.

Forbidding positional binding for soci::values is a reasonable solution. It will both eliminate the potential of sumbling on the same error and limit the amount of code that requires support (all positional get() and set() functions can be deleted in this case). Most of this PR will be deleted, except for the code fixing the indicator handling in the named binding case: no problemn ever emerged only because the convert_to_base() is called twice in the "binding pipeline".

It's up to you to decide, I'll accept any decision.

I still dislike the name of the new function, but I have trouble coming up with something better. Maybe we could invert the function semantics and call it something like should_skip()?

The name is bad, I agree. Maybe skip_binding()? I'm not sure about proper comment documentation, because it is a kind of a crutch.

[vadz]

It's up to you to decide, I'll accept any decision.

If you think it's worth having this for your bindings, I'm ready to accept it. If you don't care (enough) about it, I'd rather make things simpler by just forbidding this.

I still dislike the name of the new function, but I have trouble coming up with something better. Maybe we could invert the function semantics and call it something like should_skip()?

The name is bad, I agree. Maybe skip_binding()? I'm not sure about proper comment documentation, because it is a kind of a crutch.

Yes, skip_binding() would be better. The comment should basically just say that this is needed to avoid binding values themselves — or at least this is how I understand it.

[Innokentiy-Alaytsev]

If you think it's worth having this for your bindings, I'm ready to accept it. If you don't care (enough) about it, I'd rather make things simpler by just forbidding this.

It's not worth it. I have my binding, yeah, but solely because it's possible to have them. Besides, I'm not sure similar approach is great for C++: it's hard to imagine that someone will implement binding for something like std::tuple.

[vadz]

Thanks!

You wrote before

Most of this PR will be deleted, except for the code fixing the indicator handling in the named binding case

Would you mind doing this, i.e. deleting everything except this so that at least this much reduced version could be merged?

[Innokentiy-Alaytsev]

Of course, though a bit later.

[vadz]

Sure, this is not urgent at all. TIA!

[vadz]

Thanks! Will merge when the Appveyor CI build finishes.

[Innokentiy-Alaytsev]

Done. I kept the fix for indicator processing positional values binding for completeness. This code will be removed sometime soon, but until then it will be correct)

[vadz]

Oops, I've missed the force push and have merged the previous version of the PR. But AFAICS there are no changes between the original version and the latest one, so I'm closing this because everything was merged by the commit referenced above.

[Innokentiy-Alaytsev]

2 last force pushes are about me pushing the "add changes from master" button, which resulted in merge commit instead of a rebase, and me undoing the merge and manually rebasing (because I can)).