Skip to content

Refactoring and cleanup in connection-related code #149

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

Merged
merged 5 commits into from
Jun 13, 2023

Conversation

kravets-levko
Copy link
Contributor

@kravets-levko kravets-levko commented Jun 13, 2023

This PR fixes some things that popped out while I worked on #147. To keep original PR clean, I decided to extract those changes here:

  • Remove redundant and not-really-needed NoSaslAuthentication
  • Improve types: make HttpTransport more type-safe (since we use Thrift only over HTTP, we don't need HttpTransport to be so generic), fix classes that used ITransport/HttpTransport
  • Make PlainHttpAuthentication update headers instead of completely replace
  • Pass User-Agent header through connection object - previously we did it through auth provider because PlainHttpAuthentication was replacing headers and headers provided by HttpConnection were just ignored

May be easier to review commit by commit

@databricks databricks deleted a comment from codecov-commenter Jun 13, 2023
Signed-off-by: Levko Kravets <[email protected]>
@databricks databricks deleted a comment from github-actions bot Jun 13, 2023
@kravets-levko kravets-levko marked this pull request as ready for review June 13, 2023 12:32
@kravets-levko kravets-levko changed the title Refactoring and cleanup in conection-related code Refactoring and cleanup in connection-related code Jun 13, 2023
Copy link
Contributor

@nithinkdb nithinkdb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@nithinkdb nithinkdb merged commit 0a2bdb4 into main Jun 13, 2023
@kravets-levko kravets-levko deleted the connection-cleanup branch June 13, 2023 17:29
nithinkdb pushed a commit to nithinkdb/databricks-sql-nodejs that referenced this pull request Aug 21, 2023
* Cleanup DBSQLClient code; remove redundant and no longer needed NoSaslAuthentication

Signed-off-by: Levko Kravets <[email protected]>

* Make HttpTransport and related types more type-safe, remove redundant code

Signed-off-by: Levko Kravets <[email protected]>

* Remove some leftover code

Signed-off-by: Levko Kravets <[email protected]>

* Update tests

Signed-off-by: Levko Kravets <[email protected]>

* Pass User-Agent through HttpConnection instead of auth provider

Signed-off-by: Levko Kravets <[email protected]>

---------

Signed-off-by: Levko Kravets <[email protected]>
Signed-off-by: nithinkdb <[email protected]>
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