Refactoring and cleanup in connection-related code (#149)

* Cleanup DBSQLClient code; remove redundant and no longer needed NoSaslAuthentication

Signed-off-by: Levko Kravets <levko.ne@gmail.com>

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

Signed-off-by: Levko Kravets <levko.ne@gmail.com>

* Remove some leftover code

Signed-off-by: Levko Kravets <levko.ne@gmail.com>

* Update tests

Signed-off-by: Levko Kravets <levko.ne@gmail.com>

* Pass User-Agent through HttpConnection instead of auth provider

Signed-off-by: Levko Kravets <levko.ne@gmail.com>

---------

Signed-off-by: Levko Kravets <levko.ne@gmail.com>

Author: kravets-levko

lib/DBSQLClient.ts

@@ -11,7 +11,6 @@ import IDBSQLSession from './contracts/IDBSQLSession';
 import IThriftConnection from './connection/contracts/IThriftConnection';
 import IConnectionProvider from './connection/contracts/IConnectionProvider';
 import IAuthentication from './connection/contracts/IAuthentication';
-import NoSaslAuthentication from './connection/auth/NoSaslAuthentication';
 import HttpConnection from './connection/connections/HttpConnection';
 import IConnectionOptions from './connection/contracts/IConnectionOptions';
 import Status from './dto/Status';
@@ -48,16 +47,13 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient {
 
   private connectionProvider: IConnectionProvider;
 
-  private authProvider: IAuthentication;
-
   private readonly logger: IDBSQLLogger;
 
   private readonly thrift = thrift;
 
   constructor(options?: ClientOptions) {
     super();
     this.connectionProvider = new HttpConnection();
-    this.authProvider = new NoSaslAuthentication();
     this.logger = options?.logger || new DBSQLLogger();
     this.client = null;
     this.connection = null;
@@ -73,6 +69,9 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient {
         path: prependSlash(path),
         https: true,
         ...otherOptions,
+        headers: {
+          'User-Agent': buildUserAgentString(options.clientId),
+        },
       },
     };
   }
@@ -87,17 +86,14 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient {
    * const session = client.connect({host, path, token});
    */
   public async connect(options: ConnectionOptions, authProvider?: IAuthentication): Promise<IDBSQLClient> {
-    this.authProvider =
+    authProvider =
       authProvider ||
       new PlainHttpAuthentication({
         username: 'token',
         password: options.token,
-        headers: {
-          'User-Agent': buildUserAgentString(options.clientId),
-        },
       });
 
-    this.connection = await this.connectionProvider.connect(this.getConnectionOptions(options), this.authProvider);
+    this.connection = await this.connectionProvider.connect(this.getConnectionOptions(options), authProvider);
 
     this.client = this.thrift.createClient(TCLIService, this.connection.getConnection());
 

lib/connection/auth/NoSaslAuthentication.ts

@@ -1,34 +1,30 @@
+import { HttpHeaders } from 'thrift';
 import IAuthentication from '../contracts/IAuthentication';
-import ITransport from '../contracts/ITransport';
-import { AuthOptions } from '../types/AuthOptions';
+import HttpTransport from '../transports/HttpTransport';
 
-type HttpAuthOptions = AuthOptions & {
-  headers?: object;
-};
+interface PlainHttpAuthenticationOptions {
+  username?: string;
+  password?: string;
+  headers?: HttpHeaders;
+}
 
 export default class PlainHttpAuthentication implements IAuthentication {
-  private username: string;
+  private readonly username: string;
 
-  private password: string;
+  private readonly password: string;
 
-  private headers: object;
+  private readonly headers: HttpHeaders;
 
-  constructor(options: HttpAuthOptions) {
+  constructor(options: PlainHttpAuthenticationOptions) {
     this.username = options?.username || 'anonymous';
-    this.password = options?.password !== undefined ? options?.password : 'anonymous';
+    this.password = options?.password ?? 'anonymous';
     this.headers = options?.headers || {};
   }
 
-  authenticate(transport: ITransport): Promise<ITransport> {
-    transport.setOptions('headers', {
+  public async authenticate(transport: HttpTransport): Promise<void> {
+    transport.updateHeaders({
       ...this.headers,
-      Authorization: this.getToken(this.username, this.password),
+      Authorization: `Bearer ${this.password}`,
     });
-
-    return Promise.resolve(transport);
-  }
-
-  private getToken(username: string, password: string): string {
-    return `Bearer ${password}`;
   }
 }

lib/connection/auth/PlainHttpAuthentication.ts

@@ -1,5 +1,5 @@
-import ITransport from './ITransport';
+import HttpTransport from '../transports/HttpTransport';
 
 export default interface IAuthentication {
-  authenticate(connection: ITransport): Promise<ITransport>;
+  authenticate(transport: HttpTransport): Promise<void>;
 }

lib/connection/auth/helpers/SaslPackageFactory.ts

@@ -1,3 +1,5 @@
+import { HttpHeaders } from 'thrift';
+
 export type Options = {
   socketTimeout?: number;
   username?: string;
@@ -9,7 +11,7 @@ export type Options = {
   retry_max_delay?: number;
   connect_timeout?: number;
   timeout?: number;
-  headers?: object;
+  headers?: HttpHeaders;
   path?: string;
   ca?: Buffer | string;
   cert?: Buffer | string;

lib/connection/contracts/IAuthentication.ts

@@ -1,24 +1,56 @@
-import ITransport from '../contracts/ITransport';
+import { ConnectOptions, HttpHeaders } from 'thrift';
 
-export default class HttpTransport implements ITransport {
-  private httpOptions: object;
+export default class HttpTransport {
+  private options: ConnectOptions;
 
-  constructor(httpOptions: object = {}) {
-    this.httpOptions = httpOptions;
+  constructor(options: ConnectOptions = {}) {
+    this.options = { ...options };
   }
 
-  getTransport(): any {
-    return this.httpOptions;
+  public getOptions(): ConnectOptions {
+    return this.options;
   }
 
-  setOptions(option: string, value: any) {
-    this.httpOptions = {
-      ...this.httpOptions,
+  public setOptions(options: ConnectOptions) {
+    this.options = { ...options };
+  }
+
+  public updateOptions(options: Partial<ConnectOptions>) {
+    this.options = {
+      ...this.options,
+      ...options,
+    };
+  }
+
+  public getOption<K extends keyof ConnectOptions>(option: K): ConnectOptions[K] {
+    return this.options[option];
+  }
+
+  public setOption<K extends keyof ConnectOptions>(option: K, value: ConnectOptions[K]) {
+    this.options = {
+      ...this.options,
       [option]: value,
     };
   }
 
-  getOptions(): object {
-    return this.httpOptions;
+  public getHeaders(): HttpHeaders {
+    return this.options.headers ?? {};
+  }
+
+  public setHeaders(headers: HttpHeaders) {
+    this.options = {
+      ...this.options,
+      headers: { ...headers },
+    };
+  }
+
+  public updateHeaders(headers: Partial<ConnectOptions['headers']>) {
+    this.options = {
+      ...this.options,
+      headers: {
+        ...this.options.headers,
+        ...headers,
+      },
+    };
   }
 }

lib/connection/contracts/IConnectionOptions.ts

@@ -4,14 +4,12 @@ import TCLIService_types from '../thrift/TCLIService_types';
 import DBSQLClient from './DBSQLClient';
 import DBSQLSession from './DBSQLSession';
 import DBSQLLogger from './DBSQLLogger';
-import NoSaslAuthentication from './connection/auth/NoSaslAuthentication';
 import PlainHttpAuthentication from './connection/auth/PlainHttpAuthentication';
 import HttpConnection from './connection/connections/HttpConnection';
 import { formatProgress } from './utils';
 import { LogLevel } from './contracts/IDBSQLLogger';
 
 export const auth = {
-  NoSaslAuthentication,
   PlainHttpAuthentication,
 };
 

lib/connection/contracts/ITransport.ts

@@ -29,16 +29,15 @@ describe('PlainHttpAuthentication', () => {
     expect(auth.password).to.be.eq('');
   });
 
-  it('auth token must be set to header', () => {
+  it('auth token must be set to header', async () => {
     const auth = new PlainHttpAuthentication();
     const transportMock = {
-      setOptions(name, value) {
-        expect(name).to.be.eq('headers');
-        expect(value.Authorization).to.be.eq('Bearer anonymous');
+      updateHeaders(headers) {
+        expect(headers).to.deep.equal({
+          Authorization: 'Bearer anonymous',
+        });
       },
     };
-    return auth.authenticate(transportMock).then((transport) => {
-      expect(transport).to.be.eq(transportMock);
-    });
+    await auth.authenticate(transportMock); // it just should not fail
   });
 });