Allow configuration overrides in the connection options
In the case that the user has extensions to the underlying HttpConnection instead of relying on supporting all use cases and features the consumer can simply provide a new subclass of IConnectionProvider (or subclass the existing HttpConnection) that will be used to generate new Thrift connections.
Summary of changes:
- Added an override property to the
ClientOptionspassed to theDBSQLClientto allow providing a new implementation ofIConnectionProvider. This allows consumers to subclassHttpConnectionand for the injected dependency to be used by the client to generate thrift connections. Specifically we have customers who have unique network configurations that we'd like to be able to customize thehttp.Agentimplementation for. ExtendingHttpConnectionto provide a new agent would be the best way for us to do that. - Forward all options through the
getConnectionOptionsmethod. This will set any that are not present but also allows for unique TLS options to be used by anyIConnectionProviderif they are needed. - Add
rejectUnauthorizedas a possible option on the connection when using https. - Make the
ConnectionOptionsimplementIConnectionOptions - Mocha recommends not using arrow functions as they capture
thisat the declaration site rather than using the runtimethis. Mocha relies on that context and recommends not using arrow functions: https://mochajs.org/#arrow-functions
Hi @sidfarkus! First of all, thank you for your contribution! We really appreciate all your effort on implementing this feature (I want to highlight how solid this PR is - description, tests, and everything else).
I was actually thinking about adding some way to customize connection options. For me it looks that users may want to customize headers (add some), and http(s) agent. Basically, there's almost nothing else in HttpConnection class.
On the other hands, I have few points against the ability to fully override HttpConnection:
- the library relies on
HttpConnectionimplementation in terms that it has to, for example, create instances ofThriftHttpConnection. Custom http connection may create some of Thrift's built-in classes, but it will break a lot of things - like error handling, OAuth flows, etc. And I anticipate that in such cases users will begin submitting issues here - this will become a public interface of the library we'll have to support (at least, til next major release). It means that we'll have much less freedom in changing library internals, like mentioned
HttpConnectionorThriftHttpConnectionclasses and how they interact with the rest of the library - similar to the above,
IConnectionOptionswas intended to be an internal interface, and even though it has some properties common withConnectionOptions- they're not the same. Merging them adds a bit of chaos to the public part of the library.
Considering the above, I'd like to suggest you a bit different solution. Let's extend ConnectionOptions with optional headers and agent fields; they will be passed to a HttpConnection, which is already able to use custom headers. And agent could be just used instead of the default one HttpConnection creates. Or, alternatively, instead of agent itself you may expose agent options, but, IMHO, agent instance looks more versatile (and also follows other libraries like axios or node-fetch).
Please let me know what do you think, and let's discuss both solutions here. Thank you!