Allow callers to pass pre-connection attributes.

[detule]

Hi @mloskot, @lexicalunit :

This PR is one approach to exposing mechanisms to the user to set pre-connection attributes ( for example, #215 ).

The high level:

• It defines/exposes a connection::attribute. This is a 3-element tuple defining a connection attribute.
• It adds connection::connection(…., std::list< attribute > ) constructor, and a connection::connect(…., std::list< attribute > ) methods, allowing the caller to pass a list of pre-connection attributes as an argument. At the implementation level, other constructors (timeout, async) and other connect methods are routed through this one since this is intended to be the most generic one.

Some more detail:

1. Also considered a different workflow, where the user interaction would be:

c.allocate();
c.set_attribute(…);
c.connect(…)

but ultimately went in a different direction for a couple of reasons.

• We would be treating connection arguments like timeout and async, differently relative to other connection arguments: timeout as part of the connect() call, versus other attributes set using set_attribute. But, timeout (and async), at the end is just another connection attribute; it made some sense to have a unified API point.
• Separating the set_attribute and connect calls may be fine for some attributes but not all. In particular for some of the attributes that pass a buffer, that buffer is only read by the driver as part of the subsequent connect call, not as part of the set attribute call. So if, for example, the buffer went out of scope (or was somehow otherwise destroyed) between the set_attribute and connect calls, this could lead to subtle issues ( i.e. spending inordinate amount of time trying to understand why the azure token I was passing was incorrect, or chasing down segfaults with gdb ).

Not done here, but we could easily expose a connection.set_attribute method; if there is a desire to do this in addition to the mechanisms for setting pre-connection attributes above (rather than replace them).

2. The definition of the attribute: currently a tuple - so if for example a user wanted to add an azure authentication token they would need to do something along the lines of

attributes.push_back(
        { SQL_COPT_SS_ACCESS_TOKEN, SQL_IS_POINTER, buffer } );

Not in-love with this; feels a little bare-metal. Tried to infer the middle argument (StringLength to SQLSetConnectAttr) based on the type of buffer. But had trouble codifying the rules since it seemed to me they are not uniform - see for example ODBC documentation for the case of ValuePtr a binary buffer and how it is treated with the attribute is ODBC- versus driver-defined. It can probably be done, but also was mindful of the “nano” in nanodbc.

3. Added tests for integer ( timeout ), string ( trace file ), and more generic buffer ( async ) attribute payloads. ( The new async test is probably an overkill, since through the routing of the existing methods, the previous async test was also testing the attributes logic ). Also tested locally with an azure token where the payload is a pointer to a custom defined struct.

Feedback is appreciated.
Thank you!

[mloskot]

@detule Thank you for your work. I like the idea. Before I do detailed review, let me share high level comments first:

1. The interface needs to make it clear it is about wrapping SQLSetConnectAttr and SQLGetConnectAttr. So, I wouldn't mind replacing the tuple with dedicated structure connect_attribute which could also help handling the attributes in more sophisticated way where needed as well as handling driver-specific quirks like the Oracle timeout, that is, without polluting the connection classes with such knowledge.
2. There is need to accommodate for attribute metadata about Set before or after connection?, e.g. an enum. Again, connect_attribute struct could help with that too.
3. The connect_attribute could also help in addressing your doubt on "Not in-love with this; feels a little bare-metal."
4. Have you considered C++17 std::variant for attribute values? This should help deduce buffer type and make the interfaces friendlier and less bare-metal. We are looking towards making C++17 a default for nanodbc as there is lots of features/types that could be added (some have already been added). We could keep C++14 compatibility with by conditionally disabling the C++17 features at compile-time.

[detule]

Hi @mloskot

Thanks! Happy to go the route of a broader abstraction for connection::attribute.

Can you help me narrow down what the interface should look like? I had originally considered something like

template< typename T >
struct attribute {
  long attribute() = 0;
  long stringLength() = 0;
  std::shared_ptr<T> serialize() = 0;
}

Eventually moved away from it since I tried to use it with some simple attributes, and it was a bit of a chore / a lot more verbose than the tuple. I think you have something simpler / elegant in mind - let me know, happy to see it through.

[mloskot]

I don't think there is a need for any particular interfaces, but your tuple could be improved with std::variant

using connect_attribute = std::tuple<long, long, std::variant<int, nanodbc::string, ...>>;

or that tuple could be turned into plain structure with the same (public) members.
Simple as that.

[detule]

Thanks @mloskot
Let me know if this is more along the lines of what you had in mind. Some notes:

• Not a huge fan of all the #if __cpp_lib_variant I added. But the API does look a bit cleaner with the variant implementation.
• Pre C++17: retained a more basic, private implementation of connection::attribute that is intended for internal use. This saved from needing to add even more #ifs in nanodbc.cpp. Pre C++17, the connection attribute feature is not available for public use.
• Current pipeline does not test this feature. Tested this locally with on a linux container / gcc 11.3 / C++17 against MSSQL. I stopped short of adding C++17 tests to the appveyor matrix.
• Also fixed Cannot compile to C++17 standard without -fpermissive #341

Thanks again.

[detule]

Hi @mloskot :

Ping - whenever you have time / an opportunity to take a second look.
Happy to work with you in getting this merged - let me know.

Thanks.

[mloskot]

This looks good. I just added a few nitpicks about the camelCase naming.

[mloskot]

LGTM. Thanks!

[detule]

Thanks for the helpful feedback along the way - appreciate it.