Params: ignore unbound parameters (#223)

bpintea · bpintea · commit 22fa66896dde · 2020-03-18T01:11:51.000+01:00
Unbound parameters are allowed by the standard, although there's a mechanism that allows the application to signal the driver which parameters to ignore. There are appliacations however that set certain attributes of a first parameter (despite the query containing no markers), that however don't use this mechanism. This commit will have the driver ignore parameters with attributes set by the application, but not bound, as long as these unbound ones don't show up before the contiguous list of bound ones. (cherry picked from commit c39cb37)
diff --git a/driver/handles.c b/driver/handles.c
@@ -1138,8 +1138,8 @@ static esodbc_state_et check_buff(SQLSMALLINT field_id, SQLPOINTER buff,
 		/* pointer to a value other than string or binary string */
 		case SQL_DESC_DATA_PTR:
 			if ((buff_len != SQL_IS_POINTER) && (buff_len < 0)) {
-				/* spec says the length "should" be it's size => this check
-				 * might be too strict? */
+				/* according to the spec the length "should" be its size =>
+				 * this check might be too strict? */
 				ERR("buffer is for pointer, but its length indicator "
 					"doesn't match (%d).", buff_len);
 				return SQL_STATE_HY090;
@@ -1489,11 +1489,12 @@ esodbc_rec_st *get_record(esodbc_desc_st *desc, SQLSMALLINT rec_no, BOOL grow)
 	return &desc->recs[rec_no - 1];
 }
 
-static SQLSMALLINT recount_bound(esodbc_desc_st *desc)
+SQLSMALLINT count_bound(esodbc_desc_st *desc)
 {
 	SQLSMALLINT i;
-	for (i = desc->count; 0 < i && REC_IS_BOUND(&desc->recs[i-1]); i--)
+	for (i = desc->count; 0 < i && !REC_IS_BOUND(&desc->recs[i - 1]); i --) {
 		;
+	}
 	return i;
 }
 
@@ -2555,8 +2556,9 @@ SQLRETURN EsSQLSetDescFieldW(
 	 * buffer(s), so the above "binding" definition is incomplete.
 	 */
 	if (FieldIdentifier != SQL_DESC_DATA_PTR) {
-		DBGH(desc, "attribute to set is different than %d => unbinding data "
-			"buffer (was 0x%p).", rec->data_ptr);
+		DBGH(desc, "attribute to set is different than data ptr (%d) => "
+			"unbinding data buffer (was 0x%p).", SQL_DESC_DATA_PTR,
+			rec->data_ptr);
 		rec->data_ptr = NULL;
 	}
 
@@ -2654,10 +2656,10 @@ SQLRETURN EsSQLSetDescFieldW(
 					DBGH(desc, "rec 0x%p of desc type %d unbound.", rec,
 							desc->type);
 					if (RecNumber == desc->count) {
-						count = recount_bound(desc);
-						/* worst case: trying to unbound a not-yet-bound rec */
+						count = count_bound(desc);
+						/* worst case: trying to unbind a not-yet-bound rec */
 						if (count != desc->count) {
-							DBGH(desc, "adjusting rec count from %d to %d.",
+							DBGH(desc, "adjusting rec count from %hd to %hd.",
 									desc->count, count);
 							return update_rec_count(desc, count);
 						}
diff --git a/driver/handles.h b/driver/handles.h
@@ -417,6 +417,7 @@ typedef struct struct_stmt {
 #define STMT_GD_CALLING(_stmt)		(0 <= _stmt->gd_col)
 
 SQLRETURN update_rec_count(esodbc_desc_st *desc, SQLSMALLINT new_count);
+SQLSMALLINT count_bound(esodbc_desc_st *desc);
 esodbc_rec_st *get_record(esodbc_desc_st *desc, SQLSMALLINT rec_no, BOOL grow);
 void dump_record(esodbc_rec_st *rec);
 
diff --git a/driver/queries.c b/driver/queries.c
@@ -2857,6 +2857,31 @@ static SQLRETURN convert_param_val(esodbc_rec_st *arec, esodbc_rec_st *irec,
 	return SQL_SUCCESS;
 }
 
+static SQLRETURN get_param_recs(esodbc_stmt_st *stmt, SQLSMALLINT no,
+	esodbc_rec_st **arec, esodbc_rec_st **irec)
+{
+	esodbc_rec_st *a, *i;
+
+	a = get_record(stmt->apd, no, /*grow?*/FALSE);
+	if (a && REC_IS_BOUND(a)) {
+		*arec = a;
+	} else {
+		ERRH(stmt, "APD record #%hd @0x%p not bound", no, a);
+		RET_HDIAG(stmt, SQL_STATE_07002, "Invalid parameter configuration", 0);
+	}
+
+	i = get_record(stmt->ipd, no, /*grow?*/FALSE);
+	if (i && i->es_type) {
+		*irec = i;
+	} else {
+		ERRH(stmt, "IPD record #%hd @0x%p not configured", no, i);
+		/* possibly "manually" configured param (i.e. no SQLBindParam use) */
+		RET_HDIAG(stmt, SQL_STATE_07002, "Invalid parameter configuration", 0);
+	}
+
+	return SQL_SUCCESS;
+}
+
 /* Forms the JSON array with params:
  * [{"type": "<ES/SQL type name>", "value": <param value>}(,etc)*] */
 static SQLRETURN serialize_params_json(esodbc_stmt_st *stmt, char *dest,
@@ -2867,7 +2892,7 @@ static SQLRETURN serialize_params_json(esodbc_stmt_st *stmt, char *dest,
 	const static cstr_st j_val = CSTR_INIT("\", \"" REQ_KEY_PARAM_VAL "\": ");
 	esodbc_rec_st *arec, *irec;
 	SQLRETURN ret;
-	SQLSMALLINT i;
+	SQLSMALLINT i, count;
 	size_t l, pos;
 
 	pos = 0;
@@ -2876,10 +2901,17 @@ static SQLRETURN serialize_params_json(esodbc_stmt_st *stmt, char *dest,
 	}
 	pos ++;
 
-	for (i = 0; i < stmt->apd->count; i ++) {
-		arec = get_record(stmt->apd, i + 1, /*grow?*/FALSE);
-		irec = get_record(stmt->ipd, i + 1, /*grow?*/FALSE);
-		assert(arec && irec && irec->es_type);
+	/* some apps set/reset various parameter record attributes (maybe to
+	 * workaround for some driver issues? like Linked Servers), which increaes
+	 * the record count, but (1) do not bind the parameter and (2) do not use
+	 * the standard mechanism (through SQL_DESC_ARRAY_STATUS_PTR) to signal
+	 * params to ignore. */
+	count = count_bound(stmt->apd);
+	for (i = 0; i < count; i ++) {
+		ret = get_param_recs(stmt, i + 1, &arec, &irec);
+		if (! SQL_SUCCEEDED(ret)) {
+			return ret;
+		}
 
 		if (dest) {
 			if (i) {
@@ -2954,8 +2986,8 @@ static SQLRETURN statement_len_cbor(esodbc_stmt_st *stmt, size_t *enc_len,
 		bodylen += cbor_str_obj_len(sizeof(REQ_KEY_QUERY) - 1);
 		bodylen += cbor_str_obj_len(stmt->u8sql.cnt);
 
-		/* does the statement have any parameters? */
-		if (stmt->apd->count) {
+		/* does the statement have any bound parameters? */
+		if (count_bound(stmt->apd)) {
 			bodylen += cbor_str_obj_len(sizeof(REQ_KEY_PARAMS) - 1);
 
 			ret = statement_params_len_cbor(stmt, &len, conv_len);
@@ -3023,8 +3055,8 @@ static SQLRETURN statement_len_json(esodbc_stmt_st *stmt, size_t *outlen)
 		bodylen += json_escape(stmt->u8sql.str, stmt->u8sql.cnt, NULL, 0);
 		bodylen += 2; /* 2x `"` for query value */
 
-		/* does the statement have any parameters? */
-		if (stmt->apd->count) {
+		/* does the statement have any bound parameters? */
+		if (count_bound(stmt->apd)) {
 			bodylen += sizeof(JSON_KEY_PARAMS) - 1;
 			/* serialize_params_json will count/copy array delims (`[`, `]`) */
 			ret = serialize_params_json(stmt, /* no copy, just eval */NULL,
@@ -3193,7 +3225,7 @@ static SQLRETURN serialize_params_cbor(esodbc_stmt_st *stmt, CborEncoder *map,
 {
 	const static cstr_st p_type = CSTR_INIT(REQ_KEY_PARAM_TYPE);
 	const static cstr_st p_val = CSTR_INIT(REQ_KEY_PARAM_VAL);
-	SQLSMALLINT i;
+	SQLSMALLINT i, count;
 	CborError res;
 	SQLRETURN ret;
 	CborEncoder array; /* array for all params */
@@ -3204,10 +3236,13 @@ static SQLRETURN serialize_params_cbor(esodbc_stmt_st *stmt, CborEncoder *map,
 	res = cbor_encoder_create_array(map, &array, stmt->apd->count);
 	FAIL_ON_CBOR_ERR(stmt, res);
 
-	for (i = 0; i < stmt->apd->count; i ++) {
-		assert(stmt->ipd->count == stmt->apd->count);
-		arec = &stmt->apd->recs[i];
-		irec = &stmt->ipd->recs[i];
+	/* see note in serialize_params_json() */
+	count = count_bound(stmt->apd);
+	for (i = 0; i < count; i ++) {
+		ret = get_param_recs(stmt, i + 1, &arec, &irec);
+		if (! SQL_SUCCEEDED(ret)) {
+			return ret;
+		}
 
 		/* ~ { */
 		res = cbor_encoder_create_map(&array, &pmap, /* type + value = */2);
@@ -3289,8 +3324,8 @@ static SQLRETURN serialize_to_cbor(esodbc_stmt_st *stmt, cstr_st *dest,
 		err = cbor_encode_text_string(&map, stmt->u8sql.str, stmt->u8sql.cnt);
 		FAIL_ON_CBOR_ERR(stmt, err);
 
-		/* does the statement have any parameters? */
-		if (stmt->apd->count) {
+		/* does the statement have any bound parameters? */
+		if (count_bound(stmt->apd)) {
 			err = cbor_encode_text_string(&map, REQ_KEY_PARAMS,
 					sizeof(REQ_KEY_PARAMS) - 1);
 			FAIL_ON_CBOR_ERR(stmt, err);
@@ -3418,7 +3453,7 @@ static SQLRETURN serialize_to_json(esodbc_stmt_st *stmt, cstr_st *dest)
 		body[pos ++] = '"';
 
 		/* does the statement have any parameters? */
-		if (stmt->apd->count) {
+		if (count_bound(stmt->apd)) {
 			memcpy(body + pos, JSON_KEY_PARAMS, sizeof(JSON_KEY_PARAMS) - 1);
 			pos += sizeof(JSON_KEY_PARAMS) - 1;
 			/* serialize_params_json will count/copy array delims (`[`, `]`) */
diff --git a/test/connected_dbc.cc b/test/connected_dbc.cc
@@ -104,6 +104,17 @@ void ConnectedDBC::assertState(const SQLWCHAR *state)
 
 void ConnectedDBC::assertRequest(const char *params, const char *tz)
 {
+	const static char *answ_templ_no_params = "{"
+		JSON_KEY_QUERY "\"%s\""
+		/* params */ "%s"
+		JSON_KEY_MULTIVAL ESODBC_DEF_MFIELD_LENIENT
+		JSON_KEY_IDX_FROZEN ESODBC_DEF_IDX_INC_FROZEN
+		JSON_KEY_TIMEZONE "%s%s%s"
+		JSON_KEY_VERSION "\"%s\""
+		JSON_KEY_VAL_MODE
+		JSON_KEY_CLT_ID
+		JSON_KEY_BINARY_FMT "false"
+		"}";
 	const static char *answ_templ = "{"
 		JSON_KEY_QUERY "\"%s\""
 		JSON_KEY_PARAMS "%s"
@@ -115,18 +126,26 @@ void ConnectedDBC::assertRequest(const char *params, const char *tz)
 		JSON_KEY_CLT_ID
 		JSON_KEY_BINARY_FMT "false"
 		"}";
+	const char *templ;
 	char expect[1024];
 	int n;
 
 	cstr_st actual = {NULL, 0};
 	ret = serialize_statement((esodbc_stmt_st *)stmt, &actual);
 	ASSERT_TRUE(SQL_SUCCEEDED(ret));
 
+	if (params) {
+		templ = answ_templ;
+	} else {
+		templ = answ_templ_no_params;
+		params = "";
+	}
+
 	if (tz) {
-		n = snprintf(expect, sizeof(expect), answ_templ, test_name, params,
+		n = snprintf(expect, sizeof(expect), templ, test_name, params,
 				"\"", tz, "\"", version);
 	} else {
-		n = snprintf(expect, sizeof(expect), answ_templ, test_name, params,
+		n = snprintf(expect, sizeof(expect), templ, test_name, params,
 				"", JSON_VAL_TIMEZONE_Z, "", version);
 	}
 	ASSERT_LT(actual.cnt, sizeof(expect));
diff --git a/test/test_bindparam.cc b/test/test_bindparam.cc
@@ -0,0 +1,133 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+#include <gtest/gtest.h>
+#include "connected_dbc.h"
+
+#include <string.h>
+
+
+namespace test {
+
+class BindParam : public ::testing::Test, public ConnectedDBC {
+};
+
+
+TEST_F(BindParam, UnbindColapse) {
+	prepareStatement();
+
+	SQLSMALLINT val = 1;
+	for (int i = 1; i <= 3; i ++) {
+		ret = SQLBindParameter(stmt, /*param nr*/i, SQL_PARAM_INPUT, SQL_C_SSHORT,
+				ESODBC_SQL_BOOLEAN, /*size*/0, /*decdigits*/0, &val, sizeof(val),
+				/*IndLen*/NULL);
+		ASSERT_TRUE(SQL_SUCCEEDED(ret));
+	}
+
+	SQLHDESC apd;
+	ret = SQLGetStmtAttr(stmt, SQL_ATTR_APP_PARAM_DESC, &apd, SQL_IS_POINTER,
+			/*str-len-ptr*/NULL);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	for (int i  = 2; i <= 3; i ++) {
+		/* unbind 2nd and 3rd param (by setting smth else than the data ptr) */
+		ret = SQLSetDescField(apd, /*rec no*/i, SQL_DESC_DATA_PTR,
+				/*val*/NULL, SQL_IS_POINTER);
+		ASSERT_TRUE(SQL_SUCCEEDED(ret));
+	}
+
+	/* param count should have now be updated to 1 */
+	SQLSMALLINT count;
+	ret = SQLGetDescField(apd, /*rec no, ignored*/0, SQL_DESC_COUNT, &count,
+			SQL_IS_SMALLINT, /*str-len-ptr*/NULL);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	ASSERT_EQ(count, 1);
+}
+
+/* Linked Servers behavior */
+TEST_F(BindParam, NoBind) {
+	prepareStatement();
+
+	SQLHDESC apd;
+	ret = SQLGetStmtAttr(stmt, SQL_ATTR_APP_PARAM_DESC, &apd, SQL_IS_POINTER,
+			/*str-len-ptr*/NULL);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	ret = SQLSetStmtAttr(stmt, SQL_ATTR_PARAM_BIND_TYPE, (SQLPOINTER)0x1,
+			SQL_IS_INTEGER);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+	ret = SQLSetStmtAttr(stmt, SQL_ATTR_PARAM_BIND_TYPE, NULL, SQL_IS_INTEGER);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	ret = SQLSetStmtAttr(stmt, SQL_ATTR_PARAM_BIND_OFFSET_PTR, (SQLPOINTER)0x1,
+			SQL_IS_INTEGER);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+	ret = SQLSetStmtAttr(stmt, SQL_ATTR_PARAM_BIND_OFFSET_PTR, NULL,
+			SQL_IS_INTEGER);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	ret = SQLSetDescField(apd, /*rec no*/1, SQL_DESC_OCTET_LENGTH_PTR,
+			(SQLPOINTER)0x1, SQL_IS_POINTER);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+	ret = SQLSetDescField(apd, /*rec no*/1, SQL_DESC_OCTET_LENGTH_PTR, NULL,
+			SQL_IS_POINTER);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	ret = SQLSetStmtAttr(stmt, SQL_ATTR_PARAMSET_SIZE, (SQLPOINTER)1,
+			SQL_IS_INTEGER);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	assertRequest(/* no params */NULL);
+}
+
+TEST_F(BindParam, InvalidBind) {
+	prepareStatement();
+
+	SQLHDESC apd;
+	ret = SQLGetStmtAttr(stmt, SQL_ATTR_APP_PARAM_DESC, &apd, SQL_IS_POINTER,
+			/*str-len-ptr*/NULL);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	ret = SQLSetDescField(apd, /*rec no*/1, SQL_DESC_OCTET_LENGTH_PTR,
+			(SQLPOINTER)0x1, SQL_IS_POINTER);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+	ret = SQLSetDescField(apd, /*rec no*/1, SQL_DESC_OCTET_LENGTH_PTR, NULL,
+			SQL_IS_POINTER);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	SQLSMALLINT val = 1;
+	ret = SQLBindParameter(stmt, /*param nr*/2, SQL_PARAM_INPUT, SQL_C_SSHORT,
+			ESODBC_SQL_BOOLEAN, /*size*/0, /*decdigits*/0, &val, sizeof(val),
+			/*IndLen*/NULL);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	cstr_st actual = {NULL, 0};
+	ret = serialize_statement((esodbc_stmt_st *)stmt, &actual);
+	ASSERT_FALSE(SQL_SUCCEEDED(ret));
+}
+
+TEST_F(BindParam, BindTwo) {
+	prepareStatement();
+
+	SQLSMALLINT v1 = 1;
+	ret = SQLBindParameter(stmt, /*param nr*/1, SQL_PARAM_INPUT, SQL_C_SSHORT,
+			ESODBC_SQL_BOOLEAN, /*size*/0, /*decdigits*/0, &v1, sizeof(v1),
+			/*IndLen*/NULL);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	SQLSMALLINT v0 = 0;
+	ret = SQLBindParameter(stmt, /*param nr*/2, SQL_PARAM_INPUT, SQL_C_SSHORT,
+			ESODBC_SQL_BOOLEAN, /*size*/0, /*decdigits*/0, &v0, sizeof(v0),
+			/*IndLen*/NULL);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	assertRequest("[{\"type\": \"BOOLEAN\", \"value\": true}, "
+			"{\"type\": \"BOOLEAN\", \"value\": false}]");
+}
+
+} // test namespace
+
