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: Bug that on batch fetching string columns cannot be retrieved #229

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Wakusei
Copy link

@Wakusei Wakusei commented Dec 26, 2019

What does this PR do?

Fix a bug that on batch fetching string columns cannot be retrieved.
With a multi-row cursor SQLSetPos must be called before calling SQLGetData.

This bug happens on the following situation.

auto results= nanodbc::execute(conn, L"SELECT * FROM blahblah", 3);
while(results.next)
{
    auto i = result.get<std::wstring>( 0 );
}

@Wakusei Wakusei changed the title Update nanodbc.cpp Fix a bug that on batch fetching string columns cannot be retrieved Dec 26, 2019
@mloskot
Copy link
Member

mloskot commented Dec 26, 2019

Thanks for the fix.

https://github.com/nanodbc/nanodbc/blob/master/README.md#tests says:

One of important objectives is to maintain nanodbc covered with tests. New contributions submitted via Pull Requests must include corresponding tests.

So, in order to review and accept this PR, it needs to be updated with test case that reproduces the bug, that's failing due to the bug, and that confirms the fix for the bug.

@lexicalunit lexicalunit requested review from lexicalunit and removed request for lexicalunit December 26, 2019 18:47
@lexicalunit lexicalunit self-assigned this Dec 26, 2019
Copy link
Contributor

@lexicalunit lexicalunit left a comment

Choose a reason for hiding this comment

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

Thank you for the fix @Wakusei!

Please add a unit test for this as @mloskot suggests.

Then rebase/merge after #230 is merged to master and then run ./utility/style.sh on your PR to ensure that the code has been formatted properly.

@Wakusei
Copy link
Author

Wakusei commented Dec 27, 2019

I see. I'll try to fix them. I try to run style.sh on my local environment by myself, but if I cannot correctly prepare the tools, please help me.

@mloskot
Copy link
Member

mloskot commented Dec 27, 2019

@Wakusei most important is to add test case for the bug/bugfix. The formatting, we (I) can take care of it, so don't worry if you hit an obstacle.

If you have a problem, just ask, I'll help.

@detule
Copy link
Collaborator

detule commented Feb 27, 2021

@Wakusei this is a neat feature:

I wonder if we shouldn't call SQLSetPos with operation == SQL_REFRESH, rather than SQL_POSITION. This to help with also fetching the data in the columns that have bound buffers.

If I am reading the docs on SQLBindCol correctly (my emphasis added):

TargetValuePtr
[Deferred Input/Output] Pointer to the data buffer to bind to the column. SQLFetch and SQLFetchScroll return data in this buffer. SQLBulkOperations returns data in this buffer when Operation is SQL_FETCH_BY_BOOKMARK; it retrieves data from this buffer when Operation is SQL_ADD or SQL_UPDATE_BY_BOOKMARK. SQLSetPos returns data in this buffer when Operation is SQL_REFRESH; it retrieves data from this buffer when Operation is SQL_UPDATE.

@mloskot
Copy link
Member

mloskot commented Apr 22, 2022

I've updated the PR's branch with the latest main resolving some conflicts.

@mloskot
Copy link
Member

mloskot commented Feb 4, 2023

I have just merged main to update to this PR to pick up recent changes to CI and clang-format runs.
I did not want to rebase to not to cause any trouble to your local repo.

This PR still needs test.

@mloskot mloskot added the status/help-wanted Looking for help or expertise on a subject label Feb 4, 2023
@mloskot mloskot changed the title Fix a bug that on batch fetching string columns cannot be retrieved fix: Bug that on batch fetching string columns cannot be retrieved Feb 4, 2023
@mloskot mloskot added the status/need-tests This PR needs to be completed with test cases label Feb 4, 2023
@mloskot
Copy link
Member

mloskot commented Aug 1, 2023

I've taken the liberty to update this PR and catch up with recent CI updates.

It still needs a test case though.

if (rowset_position_ < rowset_size_)
{
RETCODE rc;
NANODBC_CALL_RC(

Check warning

Code scanning / PREfast

Arithmetic overflow: Using operator '+' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '+' to avoid overflow (io.2). Warning

Arithmetic overflow: Using operator '+' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '+' to avoid overflow (io.2).
Erimu Harada and others added 2 commits August 2, 2023 21:34
Fix a bug that on batch fetching string columns cannot be retrieved.

With a multi-row cursor SQLSetPos must be called before calling SQLGetData.
@mloskot
Copy link
Member

mloskot commented Aug 2, 2023

The CI coverage is now much better and it currently is failing for this PR with this error

-------------------------------------------------------------------------------
test_block_cursor_with_nvarchar
-------------------------------------------------------------------------------
D:\a\nanodbc\nanodbc\test\mssql_test.cpp(480)
...............................................................................

D:\a\nanodbc\nanodbc\test\mssql_test.cpp(502): FAILED:
  REQUIRE( results.next() )
due to unexpected exception with message:
  D:\a\nanodbc\nanodbc\nanodbc\nanodbc.cpp:4019: HY109: [Microsoft][ODBC Driver
  17 for SQL Server]Invalid cursor position 

-------------------------------------------------------------------------------
test_block_cursor_with_nvarchar_and_first_row_null
-------------------------------------------------------------------------------
D:\a\nanodbc\nanodbc\test\mssql_test.cpp(512)
...............................................................................

D:\a\nanodbc\nanodbc\test\mssql_test.cpp(530): FAILED:
  REQUIRE( results.next() )
due to unexpected exception with message:
  D:\a\nanodbc\nanodbc\nanodbc\nanodbc.cpp:4019: HY109: [Microsoft][ODBC Driver
  17 for SQL Server]Invalid cursor position 

-------------------------------------------------------------------------------
test_block_cursor_with_nvarchar_and_second_row_null
-------------------------------------------------------------------------------
D:\a\nanodbc\nanodbc\test\mssql_test.cpp(541)
...............................................................................

D:\a\nanodbc\nanodbc\test\mssql_test.cpp(558): FAILED:
  REQUIRE( results.next() )
due to unexpected exception with message:
  D:\a\nanodbc\nanodbc\nanodbc\nanodbc.cpp:4019: HY109: [Microsoft][ODBC Driver
  17 for SQL Server]Invalid cursor position 

===============================================================================
test cases:    77 |    74 passed | 3 failed
assertions: 28417 | 28414 passed | 3 failed
Errors while running CTest

This needs to be investigated further.
Perhaps it is related to what @detule discussed in #229 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/help-wanted Looking for help or expertise on a subject status/need-tests This PR needs to be completed with test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants