Skip to content

Shouldn't statement->execute() set used to true? #34

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

Closed
mqudsi opened this issue Mar 5, 2016 · 6 comments
Closed

Shouldn't statement->execute() set used to true? #34

mqudsi opened this issue Mar 5, 2016 · 6 comments

Comments

@mqudsi
Copy link
Contributor

mqudsi commented Mar 5, 2016

Hello,

Thanks for this library, I just migrated a project over from CppSqlite (which I'd been maintaining) over to this.

I have a question on prepared statements - is there a reason manually calling execute on a statement (and not statement++) does not set used to true? From the documentation and going by logic, I assumed execute() would execute the statement and set it to used. reset would unbind parameters and set used to false. But that's not the case since it remains "unused" even after explicitly executing and will run once more before destructing unless used(false) is manually called.

Thanks.

@Killili
Copy link
Collaborator

Killili commented Mar 5, 2016

Hi, and thanks.

As far as i remember, reset will reset the bindings and if you want to reuse them, as is the case with prepared selects you might not want to use op++.

@mqudsi
Copy link
Contributor Author

mqudsi commented Mar 5, 2016

Thanks for the quick reply. I guess my question wasn't too clear.

ErrorCode DBWrapper::InsertBlock(int blockID, uint32_t overlapOffset /*= -1*/)
{
    auto statement2 = *_masterDB << "INSERT INTO 'Overlaps'\
                    (BlockID, OverlapOffset) VALUES \
                    (?, ?);";

    return SafeSql(__FUNCTION__, [&] {
            statement2->reset();
            statement2 << blockID;
            statement2 << (int)overlapOffset;
            statement2->execute();
                    statement2->used(true);
    });
}

In the above code, if statement2->used(true); is not commented out, when SafeSql returns (which just executes the lambda in a try..catch) and statement2's destructor is called, it will execute once again (and fail because the bindings are invalid).

The destructor for database_binder checks if execution_started is false in order to determine if it should be run once more. But calling execute() does not set execution_started to true in the above case.

@aminroosta
Copy link
Collaborator

@mqudsi This looks like a bug to me.
@Killili Will #35 fix this?

@Killili
Copy link
Collaborator

Killili commented Mar 5, 2016

Ok after thinking a bit about it (has been a few weeks since i wrote that).
And i had to read a lot of old Delphi code today so have mercy.
I think the intentional separation between execute and reset is the thing that i was thinking about and this "used" issue is an oversight.

So go ahead pull the fix.

@Killili Killili closed this as completed Mar 5, 2016
@aminroosta
Copy link
Collaborator

@Killili Cool!
Please take another look at diff in #35, If it looks good to you please merge it :-)
I also set ps.used(false) on ps.reset(), that should be fine. right?

@mqudsi
Copy link
Contributor Author

mqudsi commented Mar 5, 2016

@Killili no worries, thank you kindly.

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

No branches or pull requests

3 participants