Re: More strict bind param count checking

Hi Ludek, thanks to your recent effort to improve the psqlodbc driver.
Please let me comment on your patch a little.

Ludek Finstrle wrote:
> Hello,
>
> I attach patch for more strict bind parameters count checking.
> It could avoid some bad behaviour (driver doesn't fail in some
> situations).
>
> Please review and comment
>
> Luf
>
>
> ------------------------------------------------------------------------
>
> diff -c psqlodbc.orig\convert.c psqlodbc\convert.c
> *** psqlodbc.orig\convert.c Mon Dec 05 20:20:53 2005
> --- psqlodbc\convert.c Wed Dec 14 00:58:24 2005
> ***************
> *** 1858,1866 ****
> QB_Destructor(qb);
> return SQL_ERROR;
> }
> ! if (marker_count > 0)
> {
> ! if (ipdopts && (ipdopts->allocated == marker_count))
> {
> CVT_APPEND_CHAR(qb, '(');
> for (i = 0; i < marker_count; i++)
> --- 1858,1866 ----
> QB_Destructor(qb);
> return SQL_ERROR;
> }
> ! if (ipdopts->allocated == marker_count)

Why are you using the operator '==' not '>=' ?
AFAIK there's no such limitation.

> {
> ! if (marker_count > 0)
> {
> CVT_APPEND_CHAR(qb, '(');
> for (i = 0; i < marker_count; i++)
> ***************
> *** 1871,1883 ****
> }
> CVT_APPEND_CHAR(qb, ')');
> }
> ! else
> ! {
> ! SC_set_error(stmt, STMT_COUNT_FIELD_INCORRECT, "The # of binded parameters < the # of parameter markers");
> ! SC_set_sqlstate(stmt, "07002");
> ! QB_Destructor(qb);
> ! return SQL_ERROR;
> ! }
> }
> CVT_APPEND_STR(qb, " as ");
> for (qp->opos = 0; qp->opos < qp->stmt_len; qp->opos++)
> --- 1871,1884 ----
> }
> CVT_APPEND_CHAR(qb, ')');
> }
> ! }
> ! else
> ! {
> ! CC_clear_error(stmt->hdbc);
> ! SC_set_error(stmt, STMT_COUNT_FIELD_INCORRECT, "The # of binded parameters < the # of parameter markers");
> ! SC_set_sqlstate(stmt, "07002");
> ! QB_Destructor(qb);
> ! return SQL_ERROR;
> }
> CVT_APPEND_STR(qb, " as ");
> for (qp->opos = 0; qp->opos < qp->stmt_len; qp->opos++)
> diff -c psqlodbc.orig\execute.c psqlodbc\execute.c
> *** psqlodbc.orig\execute.c Tue Dec 06 21:53:30 2005
> --- psqlodbc\execute.c Wed Dec 14 01:18:40 2005
> ***************
> *** 136,141 ****
> --- 136,143 ----
> StatementClass *stmt = (StatementClass *) hstmt;
> RETCODE result;
> CSTR func = "PGAPI_ExecDirect";
> + IPDFields *ipdopts;
> + SWORD marker_count;
>
> mylog("%s: entering...\n", func);
>
> ***************
> *** 156,161 ****
> --- 158,175 ----
>
> mylog("**** %s: hstmt=%u, statement='%s'\n", func, hstmt, stmt->statement);
>
> + /* Check bind parameters count */
> + if (SQL_SUCCESS != PGAPI_NumParams(stmt, &marker_count))
> + return SQL_ERROR;
> + ipdopts = SC_get_IPDF(stmt);
> + if (ipdopts->allocated != marker_count)

Why are you using '!=' not '<' ?

> + {
> + CC_clear_error(stmt->hdbc);
> + SC_set_error(stmt, STMT_COUNT_FIELD_INCORRECT, "The # of binded parameters < the # of parameter markers");
> + SC_set_sqlstate(stmt, "07002");
> + return SQL_ERROR;
> + }
> +
> /*
> * If an SQLPrepare was performed prior to this, but was left in the
> * premature state because an error occurred prior to SQLExecute then
> ***************
> *** 196,206 ****
> --- 210,234 ----
> APDFields *apdopts;
> IPDFields *ipdopts;
> BOOL prepare_before_exec = FALSE;
> + SWORD marker_count;
> +
>
> *exec_end = FALSE;
> conn = SC_get_conn(stmt);
> mylog("%s: copying statement params: trans_status=%d, len=%d, stmt='%s'\n", func, conn->transact_status, strlen(stmt->statement), stmt->statement);
>
> + /* Check bind parameters count */
> + if (SQL_SUCCESS != PGAPI_NumParams(stmt, &marker_count))
> + return SQL_ERROR;
> + ipdopts = SC_get_IPDF(stmt);
> + if (ipdopts->allocated != marker_count)

The same comment as the previous one.
In addtion, the check is not appropriate because Exec_with_parameter_resolved
could be called with insufficient parameters.

> + {
> + CC_clear_error(stmt->hdbc);
> + SC_set_error(stmt, STMT_COUNT_FIELD_INCORRECT, "The # of binded parameters < the # of parameter markers");
> + SC_set_sqlstate(stmt, "07002");
> + return SQL_ERROR;
> + }
> +
> /* save the cursor's info before the execution */
> cursor_type = stmt->options.cursor_type;
> scroll_concurrency = stmt->options.scroll_concurrency;