Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Binding parameters issues #157

Merged
merged 5 commits into from
Oct 27, 2022
Merged

fix: Binding parameters issues #157

merged 5 commits into from
Oct 27, 2022

Conversation

abmusse
Copy link
Member

@abmusse abmusse commented Oct 6, 2022

Resolves #147 (comment)

#147 (comment)

We now set SQL_NTS as the indicator instead of the string length.

This helps in edge cases like empty string where indicator is set to 0. Also we now ensure the buffer size is at least the size of parameter or the size of the string.

@abmusse abmusse requested a review from kadler October 6, 2022 23:26
@abmusse abmusse changed the title fix: Binding parameters issue with empty strings fix: Binding parameters issues Oct 6, 2022
Resolves IBM#147 (comment)

IBM#147 (comment)

We now set `SQL_NTS` as the indicator instead of the string length.

This helps in edge cases like empty string where indicator is set to 0. Also we now ensure the buffer size is at least the size of parameter or the size of the string.
Copy link
Member

@kadler kadler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple small changes to be made.

param[i].ind = param[i].paramSize;
// CLI does not honor the buffer size parameter or the output size in the indicator
// Make the buffer size is at least the size of parameter or the size of the string
param[i].buf = (char *)calloc(std::max(static_cast<size_t>(param[i].paramSize) + 1, str_length), sizeof(char));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

str_length will be without the null terminator too

Suggested change
param[i].buf = (char *)calloc(std::max(static_cast<size_t>(param[i].paramSize) + 1, str_length), sizeof(char));
param[i].buf = (char *)calloc(std::max(static_cast<size_t>(param[i].paramSize), str_length) + 1, sizeof(char));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

// Make the buffer size is at least the size of parameter or the size of the string
param[i].buf = (char *)calloc(std::max(static_cast<size_t>(param[i].paramSize) + 1, str_length), sizeof(char));
// Set the indicator to be SQL_NTS
// this helps in edge cases like empty string where indicator is set 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// this helps in edge cases like empty string where indicator is set 0
// this helps in edge cases like empty string where indicator is set 0 (which CLI doesn't like for some reason)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Comment on lines 2781 to 2787
param[i].buf = (char *)calloc(std::max(static_cast<size_t>(param[i].paramSize) + 1, str_length), sizeof(char));
// set the indicator to be SQL_NTS
// this helps in edge cases like empty string where indicator is set 0
param[i].ind = SQL_NTS;
// SQL_PARAM_INPUT_OUTPUT is always set so
// copy the string to the buffer
strcpy((char*)param[i].buf, cString);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same changes as above need to be done here too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@abmusse abmusse requested a review from kadler October 10, 2022 16:10
Copy link
Contributor

@markdirish markdirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes make sense to me. It seems like a real headache that CLI doesn't respect these things it should..

Copy link
Member

@kadler kadler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I just had a couple grammar suggestions

src/db2ia/dbstmt.cc Outdated Show resolved Hide resolved
src/db2ia/dbstmt.cc Outdated Show resolved Hide resolved
abmusse and others added 2 commits October 27, 2022 12:04
Co-authored-by: Kevin Adler <kadler@us.ibm.com>
Co-authored-by: Kevin Adler <kadler@us.ibm.com>
@abmusse abmusse merged commit 99e127a into IBM:master Oct 27, 2022
@abmusse abmusse deleted the issue-147 branch October 27, 2022 17:07
@patrickhrastnik
Copy link

Thanks for fixing this bug @abmusse
Can you give us an estimate on when the fix will be rolled out in a new version of idb-connector?
According to Releases tab the last version was rolled out in April.
It'll help us scheduling our next steps to integrate the fix

@patrickhrastnik
Copy link

Furthermore, I'd need to know when idb-pconnector will be upgraded to use the new version of idb-connector. That's the package we're actually using.

@abmusse
Copy link
Member Author

abmusse commented Oct 31, 2022

@patrickhrastnik

I'm working on getting both packages released today. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error SQLSTATE=22504 SQLCODE=-191 Mixed data or UTF-8 data not properly formed.
4 participants