From 4e72cab8e226b6a386fed488218cd04c197adb5c Mon Sep 17 00:00:00 2001 From: Adil Ansari Date: Tue, 17 Jan 2023 14:22:05 -0800 Subject: [PATCH 1/2] refactor: Tigris operations to use enums for Status (#210) --- src/__tests__/test-service.ts | 2 - src/__tests__/tigris.rpc.spec.ts | 7 +- src/cache.ts | 5 +- src/collection.ts | 4 +- src/constants.ts | 8 +++ src/db.ts | 4 +- src/index.ts | 1 + src/session.ts | 4 +- src/tigris.ts | 2 +- src/types.ts | 108 ++++++++++++++----------------- 10 files changed, 70 insertions(+), 75 deletions(-) create mode 100644 src/constants.ts diff --git a/src/__tests__/test-service.ts b/src/__tests__/test-service.ts index 3def715..78a0cd1 100644 --- a/src/__tests__/test-service.ts +++ b/src/__tests__/test-service.ts @@ -259,7 +259,6 @@ export class TestTigrisService { assert(call.request.getBranch() === TestTigrisService.ExpectedBranch); const reply: CommitTransactionResponse = new CommitTransactionResponse(); - reply.setStatus("committed-test"); callback(undefined, reply); }, createProject( @@ -604,7 +603,6 @@ export class TestTigrisService { assert(call.request.getBranch() === TestTigrisService.ExpectedBranch); const reply: RollbackTransactionResponse = new RollbackTransactionResponse(); - reply.setStatus("rollback-test"); callback(undefined, reply); }, update( diff --git a/src/__tests__/tigris.rpc.spec.ts b/src/__tests__/tigris.rpc.spec.ts index e000bc8..f41be2d 100644 --- a/src/__tests__/tigris.rpc.spec.ts +++ b/src/__tests__/tigris.rpc.spec.ts @@ -26,6 +26,7 @@ import { SearchIterator } from "../consumables/search-iterator"; import { CacheService } from "../proto/server/v1/cache_grpc_pb"; import { DatabaseBranchError } from "../error"; import { Status } from "@grpc/grpc-js/build/src/constants"; +import { Status as TigrisStatus } from "../constants"; describe("rpc tests", () => { let server: Server; @@ -326,7 +327,7 @@ describe("rpc tests", () => { }, }); updatePromise.then((value) => { - expect(value.status).toBe('updated: {"id":1}, {"$set":{"title":"New Title"}}'); + expect(value.status).toBe(TigrisStatus.Updated); expect(value.modifiedCount).toBe(1); }); return updatePromise; @@ -569,7 +570,7 @@ describe("rpc tests", () => { beginTxPromise.then((session) => { const commitTxResponse = session.commit(); commitTxResponse.then((value) => { - expect(value.status).toBe("committed-test"); + expect(value.status).toBe(TigrisStatus.Success); }); return beginTxPromise; }); @@ -582,7 +583,7 @@ describe("rpc tests", () => { beginTxPromise.then((session) => { const rollbackTransactionResponsePromise = session.rollback(); rollbackTransactionResponsePromise.then((value) => { - expect(value.status).toBe("rollback-test"); + expect(value.status).toBe(TigrisStatus.Success); }); }); return beginTxPromise; diff --git a/src/cache.ts b/src/cache.ts index e75572f..3e6e34e 100644 --- a/src/cache.ts +++ b/src/cache.ts @@ -82,7 +82,7 @@ export class Cache { if (error) { reject(error); } else { - resolve(new CacheSetResponse(response.getStatus(), response.getMessage())); + resolve(new CacheSetResponse(response.getMessage())); } }); }); @@ -117,13 +117,12 @@ export class Cache { if (response.getOldValue() !== undefined && response.getOldValue_asU8().length > 0) { resolve( new CacheGetSetResponse( - response.getStatus(), response.getMessage(), Utility._base64DecodeToObject(response.getOldValue_asB64(), this._config) ) ); } else { - resolve(new CacheGetSetResponse(response.getStatus(), response.getMessage())); + resolve(new CacheGetSetResponse(response.getMessage())); } } }); diff --git a/src/collection.ts b/src/collection.ts index 435cd99..60fa9ca 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -226,7 +226,7 @@ export class Collection implements ICollection { response.getMetadata().getCreatedAt(), response.getMetadata().getUpdatedAt() ); - resolve(new UpdateResponse(response.getStatus(), response.getModifiedCount(), metadata)); + resolve(new UpdateResponse(response.getModifiedCount(), metadata)); } }); }); @@ -354,7 +354,7 @@ export class Collection implements ICollection { response.getMetadata().getCreatedAt(), response.getMetadata().getUpdatedAt() ); - resolve(new DeleteResponse(response.getStatus(), metadata)); + resolve(new DeleteResponse(metadata)); } }); }); diff --git a/src/constants.ts b/src/constants.ts new file mode 100644 index 0000000..50c12d4 --- /dev/null +++ b/src/constants.ts @@ -0,0 +1,8 @@ +export enum Status { + Created = "created", + Updated = "updated", + Deleted = "deleted", + Dropped = "dropped", + Success = "success", + Set = "set", +} diff --git a/src/db.ts b/src/db.ts index 29d4409..51aac8c 100644 --- a/src/db.ts +++ b/src/db.ts @@ -300,7 +300,7 @@ export class DB { if (error) { reject(error); } else { - resolve(new DropCollectionResponse(response.getStatus(), response.getMessage())); + resolve(new DropCollectionResponse(response.getMessage())); } } ); @@ -399,7 +399,7 @@ export class DB { // user code successful const commitResponse: CommitTransactionResponse = await session.commit(); if (commitResponse) { - resolve(new TransactionResponse("transaction successful")); + resolve(new TransactionResponse()); } } catch (error) { // failed to run user code diff --git a/src/index.ts b/src/index.ts index 15b515d..1dcbda1 100644 --- a/src/index.ts +++ b/src/index.ts @@ -4,6 +4,7 @@ export * from "./session"; export * from "./tigris"; export * from "./types"; export * from "./search/types"; +export * from "./constants"; export { Field } from "./decorators/tigris-field"; export { PrimaryKey } from "./decorators/tigris-primary-key"; export { TigrisCollection } from "./decorators/tigris-collection"; diff --git a/src/session.ts b/src/session.ts index 9451e04..d0941c4 100644 --- a/src/session.ts +++ b/src/session.ts @@ -52,7 +52,7 @@ export class Session { if (error) { reject(error); } else { - resolve(new CommitTransactionResponse(response.getStatus())); + resolve(new CommitTransactionResponse()); } }); }); @@ -70,7 +70,7 @@ export class Session { if (error) { reject(error); } else { - resolve(new RollbackTransactionResponse(response.getStatus())); + resolve(new RollbackTransactionResponse()); } } ); diff --git a/src/tigris.ts b/src/tigris.ts index 0e43a4d..10710c1 100644 --- a/src/tigris.ts +++ b/src/tigris.ts @@ -300,7 +300,7 @@ export class Tigris { if (error) { reject(error); } else { - resolve(new DeleteCacheResponse(response.getStatus(), response.getMessage())); + resolve(new DeleteCacheResponse(response.getMessage())); } } ); diff --git a/src/types.ts b/src/types.ts index a2551ea..df7ff32 100644 --- a/src/types.ts +++ b/src/types.ts @@ -3,6 +3,7 @@ import { CreateBranchResponse as ProtoCreateBranchResponse, DeleteBranchResponse as ProtoDeleteBranchResponse, } from "./proto/server/v1/api_pb"; +import { Status } from "./constants"; export class DatabaseInfo { private readonly _name: string; @@ -48,23 +49,16 @@ export class DatabaseOptions {} export class CollectionOptions {} -export class TigrisResponse { - private readonly _status: string; - - constructor(status: string) { - this._status = status; - } - - get status(): string { - return this._status; - } +export interface TigrisResponse { + status: Status; + message?: string; } -export class CreateBranchResponse extends TigrisResponse { +export class CreateBranchResponse implements TigrisResponse { + status: Status = Status.Created; private readonly _message: string; - constructor(status: string, message: string) { - super(status); + constructor(message: string) { this._message = message; } @@ -73,14 +67,14 @@ export class CreateBranchResponse extends TigrisResponse { } static from(response: ProtoCreateBranchResponse): CreateBranchResponse { - return new this(response.getStatus(), response.getMessage()); + return new this(response.getMessage()); } } -export class DeleteBranchResponse extends TigrisResponse { +export class DeleteBranchResponse implements TigrisResponse { + status: Status = Status.Deleted; private readonly _message: string; - constructor(status: string, message: string) { - super(status); + constructor(message: string) { this._message = message; } @@ -89,23 +83,18 @@ export class DeleteBranchResponse extends TigrisResponse { } static from(response: ProtoDeleteBranchResponse): DeleteBranchResponse { - return new this(response.getStatus(), response.getMessage()); + return new this(response.getMessage()); } } -export class DropCollectionResponse { - private readonly _status: string; +export class DropCollectionResponse implements TigrisResponse { + status: Status = Status.Dropped; private readonly _message: string; - constructor(status: string, message: string) { - this._status = status; + constructor(message: string) { this._message = message; } - get status(): string { - return this._status; - } - get message(): string { return this._message; } @@ -181,11 +170,15 @@ export class DMLMetadata { } } -export class DMLResponse extends TigrisResponse { +export interface DMLResponse { + metadata: DMLMetadata; +} + +export class DeleteResponse implements TigrisResponse, DMLResponse { + status: Status = Status.Deleted; private readonly _metadata: DMLMetadata; - constructor(status: string, metadata: DMLMetadata) { - super(status); + constructor(metadata: DMLMetadata) { this._metadata = metadata; } @@ -194,22 +187,23 @@ export class DMLResponse extends TigrisResponse { } } -export class DeleteResponse extends DMLResponse { - constructor(status: string, metadata: DMLMetadata) { - super(status, metadata); - } -} - -export class UpdateResponse extends DMLResponse { +export class UpdateResponse implements TigrisResponse, DMLResponse { + status: Status = Status.Updated; + private readonly _metadata: DMLMetadata; private readonly _modifiedCount: number; - constructor(status: string, modifiedCount: number, metadata: DMLMetadata) { - super(status, metadata); + + constructor(modifiedCount: number, metadata: DMLMetadata) { this._modifiedCount = modifiedCount; + this._metadata = metadata; } get modifiedCount(): number { return this._modifiedCount; } + + get metadata(): DMLMetadata { + return this._metadata; + } } export class WriteOptions {} @@ -320,22 +314,16 @@ export class FindQueryOptions { export class TransactionOptions {} -export class CommitTransactionResponse extends TigrisResponse { - constructor(status: string) { - super(status); - } +export class CommitTransactionResponse implements TigrisResponse { + status: Status = Status.Success; } -export class RollbackTransactionResponse extends TigrisResponse { - public constructor(status: string) { - super(status); - } +export class RollbackTransactionResponse implements TigrisResponse { + status: Status = Status.Success; } -export class TransactionResponse extends TigrisResponse { - constructor(status: string) { - super(status); - } +export class TransactionResponse implements TigrisResponse { + status: Status = Status.Success; } export class CacheMetadata { @@ -361,11 +349,11 @@ export class ListCachesResponse { } } -export class DeleteCacheResponse extends TigrisResponse { +export class DeleteCacheResponse implements TigrisResponse { + status: Status = Status.Deleted; private readonly _message: string; - constructor(status: string, message: string) { - super(status); + constructor(message: string) { this._message = message; } @@ -374,11 +362,11 @@ export class DeleteCacheResponse extends TigrisResponse { } } -export class CacheSetResponse extends TigrisResponse { +export class CacheSetResponse implements TigrisResponse { + status: Status = Status.Set; private readonly _message: string; - constructor(status: string, message: string) { - super(status); + constructor(message: string) { this._message = message; } @@ -390,8 +378,8 @@ export class CacheSetResponse extends TigrisResponse { export class CacheGetSetResponse extends CacheSetResponse { private readonly _old_value: object; - constructor(status: string, message: string, old_value?: object) { - super(status, message); + constructor(message: string, old_value?: object) { + super(message); if (old_value !== undefined) { this._old_value = old_value; } @@ -402,11 +390,11 @@ export class CacheGetSetResponse extends CacheSetResponse { } } -export class CacheDelResponse extends TigrisResponse { +export class CacheDelResponse implements TigrisResponse { + status: Status = Status.Deleted; private readonly _message: string; constructor(status: string, message: string) { - super(status); this._message = message; } From 610a14053733919e5921920c2730fcd6f75d6a0d Mon Sep 17 00:00:00 2001 From: Adil Ansari Date: Tue, 17 Jan 2023 18:29:58 -0800 Subject: [PATCH 2/2] fix: Bug where describe DB was using unavailable branch name (#212) --- src/db.ts | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/db.ts b/src/db.ts index 51aac8c..d2210bc 100644 --- a/src/db.ts +++ b/src/db.ts @@ -49,7 +49,7 @@ const BeginTransactionMethodName = "/tigrisdata.v1.Tigris/BeginTransaction"; * ``` * { * name: "my_database_branch", - * isTemplated: false + * dynamicCreation: false * } * ``` * @example A dynamically generated branch name "my_db_${GIT_BRANCH}" would translate to: @@ -57,7 +57,7 @@ const BeginTransactionMethodName = "/tigrisdata.v1.Tigris/BeginTransaction"; * export GIT_BRANCH=feature_1 * { * name: "my_db_feature_1", - * isTemplated: true + * dynamicCreation: true * } * ``` */ @@ -70,7 +70,7 @@ const NoBranch: TemplatedBranchName = { name: "", dynamicCreation: false }; */ export class DB { private readonly _db: string; - private _branchVar: TemplatedBranchName; + private _branch: string; private readonly grpcClient: TigrisClient; private readonly config: TigrisClientConfig; private readonly schemaProcessor: DecoratedSchemaProcessor; @@ -107,8 +107,7 @@ export class DB { this.config = config; this.schemaProcessor = DecoratedSchemaProcessor.Instance; this._metadataStorage = getDecoratorMetaStorage(); - // TODO: Should we just default to `main` or empty arg or throw an exception here? - this._branchVar = Utility.branchNameFromEnv(config.branch) ?? NoBranch; + this._branch = NoBranch.name; this._ready = this.initializeDB(); } @@ -124,27 +123,29 @@ export class DB { * @private */ private async initializeDB(): Promise { - if (this._branchVar.name === NoBranch.name) { + const branchVar = Utility.branchNameFromEnv(this.config.branch) ?? NoBranch; + if (branchVar.name === NoBranch.name) { return this; } const description = await this.describe(); - const branchExists = description.branches.includes(this.branch); + const branchExists = description.branches.includes(branchVar.name); if (!branchExists) { - if (this._branchVar.dynamicCreation) { + if (branchVar.dynamicCreation) { try { - await this.createBranch(this.branch); + await this.createBranch(branchVar.name); } catch (error) { if ((error as ServiceError).code !== Status.ALREADY_EXISTS) { throw error; } } - Log.event(`Created database branch: ${this.branch}`); + Log.event(`Created database branch: ${branchVar.name}`); } else { - throw new DatabaseBranchError(this.branch); + throw new DatabaseBranchError(branchVar.name); } } - Log.info(`Using database branch: '${this.branch}'`); + Log.info(`Using database branch: '${branchVar.name}'`); + this._branch = branchVar.name; return this; } @@ -482,7 +483,7 @@ export class DB { } get branch(): string { - return this._branchVar.name; + return this._branch; } get ready(): Promise {