SQLDriverConnect implementation

SQLDisconnect implementation

Can you check if disconnect() does proper clean-up of the connection? It should closes and drop statements on the connection and end any transactions.

Yes, disconnect() doesn't close statements directly, but the destructor of all ODBCStatement objects from to the connection will be called after the m_statements.clear(); line. m_statements should contain all statements/transactions.

The implementation:

void ODBCConnection::disconnect() {
  if (m_isConnected) {
    // Ensure that all statements (and corresponding SPI statements) get cleaned
    // up before terminating the SPI connection in case they need to be de-allocated in
    // the reverse of the allocation order.
    m_statements.clear();
    m_spiConnection->Close();
    m_isConnected = false;
  }
}

This should prompt the dialog based on the driverCompletion variable. Maybe leave integrating the actual dialog to a separate PR but setup the infrastructure and leave TODOs here.

Good call, added TODOs

Fixed an issue where ExecuteWithDiagnostics attempts to get diagnostics from conn after conn is freed, and results in a seg fault error.

Catch as const DriverException& if possible

added const, since this is the only usage of DriverException in the file, I will leave the prefix driver::odbcabstraction as is

Micro-optimization. Add a move-version of config.Set() (eg Set(const std::string& key, std::string&& value)) and call it Emplace(). Then use config.Emplace(key, std::move(value)) to avoid copying.

Added Emplace and switched to use it

Can you check if disconnect() does proper clean-up of the connection? It should closes and drop statements on the connection and end any transactions.

This might need to be conditional based on driver manager. Also, this can be a string_view (and GetStringAttribute should probably be reworked to take in string_views?)

I can use string_view here but will just keep using 3.80 as it works

Updated code to make GetStringAttribute take string_views

added const, since this is the only usage of DriverException in the file, I will leave the prefix driver::odbcabstraction as is

Added Emplace and switched to use it

Yes, disconnect() doesn't close statements directly, but the destructor of all ODBCStatement objects from to the connection will be called after the m_statements.clear(); line. m_statements should contain all statements/transactions.

The implementation:

void ODBCConnection::disconnect() {
  if (m_isConnected) {
    // Ensure that all statements (and corresponding SPI statements) get cleaned
    // up before terminating the SPI connection in case they need to be de-allocated in
    // the reverse of the allocation order.
    m_statements.clear();
    m_spiConnection->Close();
    m_isConnected = false;
  }
}

Updated code to make GetStringAttribute take string_views

Shouldn't this be properties[copy]?

copy holds a copy of the value, we still need to construct a std::string copy for key

Use properties.emplace(std::move(copy), value))

I misunderstood usage of emplace before, I have fixed this now. In SQLConnect, uid and pwd specified in DSN will take precedence

Shouldn't we be using the execute with diagnostics wrapper here?

There was a bug that I ran into and had to remove the wrapper. The issue is that ExecuteWithDiagnostics calls GetDiagnostics().HasError() at the end of lambda function execution, and it will assume that conn is still a valid pointer. But GetDiagnostics() will fail because conn has been destructed. I didn't find any exceptions to be thrown inside releaseConnection call so it should be ok

Minor nit picking, but might be clearer to be passing in connection pointer instead of conn, even thought is pointing to same data.

Good catch, I have moved the connection pointer creation code to be inside the ExecuteWithDiagnostics call so conn can still be passed