Repository navigation
feat: Connection::is_autocommit - #960
Conversation
| 0 => false, | ||
| 1 => true, | ||
| other => panic!("Unexpected result value from SQLGetConnectAttr: {other}"), | ||
| other => panic!("Unexpected result value from SQLGetConnectAttr SQL_ATTR_AUTOCOMMIT: {other}"), |
There was a problem hiding this comment.
Not necessary to annotate the attribute. Yet, if we do it it should be correct. This one is getting CONNECTION_DEAD.
If we name it, maybe we can improve on the sentence, rather than just adding it. E.g. "Unexpected result value retrieving SQL_ATTR_AUTOCOMMIT via SQLGetConnectAttr.
| #[test_case(MARIADB; "Maria DB")] | ||
| #[test_case(SQLITE_3; "SQLite 3")] | ||
| #[test_case(POSTGRES; "PostgreSQL")] | ||
| fn is_autocommit(profile: &Profile) { |
There was a problem hiding this comment.
Please split this in two tests. One for true and and for false.
| fn transaction_capable(profile: &Profile) { | ||
| let conn = profile.connection().unwrap(); | ||
| assert_ne!( | ||
| TransactionCapable::None, |
There was a problem hiding this comment.
Please change assertion to what is returned as opposed to what is not
You can use a second argument to control different test expectations for the various databases.
| /// See: | ||
| /// <https://learn.microsoft.com/en-us/sql/odbc/reference/syntax/sqlgetinfo-function> | ||
| #[derive(Clone, Copy, Hash, Debug, Eq, PartialEq)] | ||
| pub enum TransactionCapable { |
There was a problem hiding this comment.
I would prefer this to be strict alias over u16 and defined in odbc-sys instead of odbc-api.
odbc-sys owns the definition of the ODBC interface. This is analogous to ConnectionAttribute which you already used for fetching autocommit information.
|
Thanks for the contribution. Sorry for the long feedback cycles, I am currently don't have enough spare time to be as responsive to OS contributions as I would like to be. It's often better to split separate features in different commits and PRs. This way unresolved issues in one do not block progress on the other. I'll leave it to up to you, whether you want to introduce an additional TransactionCapable enum after the strict alias in odbc-sys lands. |
alfhellmund
changed the title
|
Thanks for the review, and no worries about the timing. I followed your suggestion and split it up:
On the inline comments:
|
Adds two read accessors to `Connection`, as discussed in pacman82#959: - `is_autocommit()` reads `SQL_ATTR_AUTOCOMMIT` via `SQLGetConnectAttr`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4f48b62 to
d9ce608
Compare
Adds
Connection::is_autocommit, which readsSQL_ATTR_AUTOCOMMITviaSQLGetConnectAttr, as discussed in #959.Following your review, this PR now covers only the autocommit accessor. Reading
SQL_TXN_CAPABLEfollows in a separate PR onceTransactionCapableis available as a strict alias in odbc-sys (pacman82/odbc-sys#112).The integration tests cover all four drivers under test, split into one test for the default (
true) and one afterset_autocommit(false)(false).One observation that may save you a debugging session: MariaDB Connector/ODBC 3.2.x (e.g. 3.2.10 from Homebrew) reports a stale
SQL_ATTR_AUTOCOMMITright afterSQLSetConnectAttr— it still returns1until the next statement has been executed. The 3.2 driver reads the value from a cached server status (driver/ma_connection.cpp,getAutocommit()→ mariadb-connector-cppProtocol::getAutocommit, refreshed only ingetServerStatus()), while 3.1.x returns the value set by the application. The CI image ships 3.1.15, so the test passes there. I didn't find a matching MariaDB JIRA issue.Part of #959
🤖 Generated with Claude Code