Named params

[AdminCrystal]

Add the option to use named parameters instead of ? for placeholders. This pull request is not fully ready but I thought I would share with you if you are ok with the addition. If you are good with it I can update the docs and whatever else you need me to update to make it mergeable. I have a couple unit tests on my godot project which still work before and after this change as well as with the new code.

I should preface this pull request as this is essentially my first c++ pull request ever so if you can check to make sure I didn't shoot my foot somewhere that would be appreciated.

Example query_with_bindings

INSERT INTO game_user_collectables (
	game_id,
	codex_id,
	source_codex_id,
	user_id,
	save_id,
	collectable_count
) VALUES (
	?,
	?,
	?,
	?,
	?,
	?
);

Example query_with_named_bindings

INSERT INTO game_user_collectables (
	game_id,
	codex_id,
	source_codex_id,
	user_id,
	save_id,
	collectable_count
) VALUES (
	:game_id,
	:codex_id,
	:source_codex_id,
	:user_id,
	:save_id,
	:save_id
);

Or an example unit test I created

Example use of this new code

func test_collection_log_named_params() -> void:
	var sqlx_db := SqlxTest.new(["res://addons/collection_log/migrations/codex.sql"])
	var db = sqlx_db.db
	assert_bool(db.query("SELECT * FROM game_user_collectables")).is_true()

	assert_bool(db.query_result.is_empty()).is_true().override_failure_message("No rows should be present")

	var query := """
				INSERT INTO game_user_collectables (
					game_id,
					codex_id,
					source_codex_id,
					user_id,
					save_id,
					collectable_count
				) VALUES (
					:game_id,
					:codex_id,
					:source_codex_id,
					:user_id,
					:save_id,
					:save_id
				);
				"""
	var params := {
		"game_id": "godot_plugins",
		"codex_id": "NORMAL_JUMPS_TAKEN",
		"source_codex_id": "SELF",
		"user_id": "user_1",
		"save_id": 1,
	}
	assert_bool(db.query_with_named_bindings(query, params)).is_true()
	var expected := [
		{
			"game_id": "godot_plugins",
			"codex_id": "NORMAL_JUMPS_TAKEN",
			"source_codex_id": "SELF",
			"user_id": "user_1",
			"save_id": 1,
			"collectable_count": 1
		}
	]

	assert_bool(db.query("SELECT * FROM game_user_collectables")).is_true()
	assert_array(db.query_result).is_equal(expected)

[2shady4u]

Hello @AdminCrystal 😄

Would indeed be nice to have this feature.

Could you elaborate on the choice of splitting query_with_bindings()- and query_with_named_bindings()-methods?
Would there by any use case for mixed bindings?

Example query where save_id is bound as a nameless param while all other params are named:

INSERT INTO game_user_collectables (
	game_id,
	codex_id,
	source_codex_id,
	user_id,
	save_id,
	collectable_count
) VALUES (
	:game_id,
	:codex_id,
	:source_codex_id,
	:user_id,
	?,
	:save_id
);

Where the method has following signature:
query_with_bindings(query: String, named_params: Dictionary, nameless_params: Array)

Would there be any point to this use-case?
(Maybe there isn't!)

[2shady4u]

Eh, to be honest; I don't think this mixed use-case is useful at all.
I would definitely try to separate as much common functionality into private methods. (which you are already doing)

[AdminCrystal]

@2shady4u
I believe it is technically possible to use both named and nameless with sqlite, so I went and checked other implementations to see if they allow both, looks like Java Springboot only allows one or the other using JdbcTemplate or NamedParameterJdbcTemplate, Rust SQLx uses one or the other, and Go SQLx uses one or the other.

I have updated the readme, the doc comments and made the variable name same as query_with_bindings for consistency. Let me know if any changes are needed from my end on this pr. Thanks!

[2shady4u]

I did some initial review.

Main comment is definitely that a lot of code is duplicated -> Can be moved into further private methods; for example it would make sense to have a private "prepare_statement"-method as well:

Furthermore, for readability's sake I have following general comments:

1. Put the methods in a more logical sequence:

(imo it doesn't make sense to have execute_statement in-between query_with_bindings and query_with_named_bindings) 2. Try put the initialization and finalization of pointers in the same method; now it is very hard to follow lifecycle of the statement and the zErrMsg.

[AdminCrystal]

Ok I think it is now in a rereviewable state, c++ kicking my ass with pointers and what not. I believe I have addressed all of the comments, but as my c++ knowledge is 0 I wouldn't be surprised if after my refactor you have a couple more comments @2shady4u.

[AdminCrystal]

@2shady4u should be good again, just left a comment on the zErrMsg one as I dont think its freeable based on the return type of that specific function call.

[2shady4u]

Hello @AdminCrystal,

Sorry for the delay 😢
It is looking very good now! I just added some very minor comments, but after that I think it is ready to merge 😄

[AdminCrystal]

I have updated based on the comments, I also noticed StringName did not work just now but see you fixed already in main branch, I have pulled main branch and I added support for StringName for my named params section so that is new code introduced since last review in my pull request because by default the dictionary.has(*somePtr) fails if its string name so I have added that with this recent commit. Example test

func test_collection_log_named_params() -> void:
	var sqlx_db := SqlxTest.new(["res://addons/collection_log/migrations/codex.sql"])
	var db = sqlx_db.db
	assert_bool(db.query("SELECT * FROM game_user_collectables")).is_true()

	assert_bool(db.query_result.is_empty()).is_true().override_failure_message("No rows should be present")

	var query := """
				INSERT INTO game_user_collectables (
					game_id,
					codex_id,
					source_codex_id,
					user_id,
					save_id,
					collectable_count
				) VALUES (
					:game_id,
					:codex_id,
					:source_codex_id,
					:user_id,
					:save_id,
					:save_id
				);
				"""
	var params := {
		&"game_id": &"godot_plugins",
		"codex_id": &"NORMAL_JUMPS_TAKEN",
		&"source_codex_id": "SELF",
		&"user_id": "user_1",
		"save_id": 1,
	}
	TYPE_STRING_NAME
	assert_bool(db.query_with_named_bindings(query, params)).is_true()
	var expected := [
		{
			"game_id": "godot_plugins",
			"codex_id": "NORMAL_JUMPS_TAKEN",
			"source_codex_id": "SELF",
			"user_id": "user_1",
			"save_id": 1,
			"collectable_count": 1
		}
	]

	assert_bool(db.query("SELECT * FROM game_user_collectables")).is_true()
	assert_array(db.query_result).is_equal(expected)

[2shady4u]

Thank you for taking the time to work on this MR 😄
I just have to main comments and I have proposed solutions to them.
As always, feel free to challenge them if I missed something.

[AdminCrystal]

Have updated the pr with the fixes suggested

[2shady4u]

It has been merged!
Thank you for your contribution and the time you spent on this 😃

[AdminCrystal]

It has been merged! Thank you for your contribution and the time you spent on this 😃

Thanks for dealing with the little issues i kept adding. Now its in a lot better of a state.