Skip to content
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

Allow configuration overrides in the connection options #201

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sidfarkus
Copy link

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 ClientOptions passed to the DBSQLClient to allow providing a new implementation of IConnectionProvider. This allows consumers to subclass HttpConnection and 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 the http.Agent implementation for. Extending HttpConnection to provide a new agent would be the best way for us to do that.
  • Forward all options through the getConnectionOptions method. This will set any that are not present but also allows for unique TLS options to be used by any IConnectionProvider if they are needed.
  • Add rejectUnauthorized as a possible option on the connection when using https.
  • Make the ConnectionOptions implement IConnectionOptions
  • Mocha recommends not using arrow functions as they capture this at the declaration site rather than using the runtime this. Mocha relies on that context and recommends not using arrow functions: https://mochajs.org/#arrow-functions

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.
@kravets-levko
Copy link
Contributor

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 HttpConnection implementation in terms that it has to, for example, create instances of ThriftHttpConnection. 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 HttpConnection or ThriftHttpConnection classes and how they interact with the rest of the library
  • similar to the above, IConnectionOptions was intended to be an internal interface, and even though it has some properties common with ConnectionOptions - 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!

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

Successfully merging this pull request may close these issues.

2 participants