From d2042a4239d6fb978cc0db4616b450e28da27ee5 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Wed, 18 Dec 2024 09:48:11 -0800 Subject: [PATCH] Merge Session & Options object into one This will make a nicer foundation for the upcoming withCodecs API, as I plan to start passing Options into the codecs machinery. Along the way, I refactored Options to ensure they are fully immutable and use proper collections types. One specific thing I wanted to fix is avoiding using `=== defaults()` checks as they are flaky. --- packages/driver/src/baseClient.ts | 18 +-- packages/driver/src/baseConn.ts | 20 +-- packages/driver/src/fetchConn.ts | 6 +- packages/driver/src/ifaces.ts | 9 ++ packages/driver/src/index.node.ts | 2 +- packages/driver/src/options.ts | 237 ++++++++++++++++------------ packages/driver/src/transaction.ts | 28 ++-- packages/driver/test/client.test.ts | 6 +- 8 files changed, 179 insertions(+), 147 deletions(-) diff --git a/packages/driver/src/baseClient.ts b/packages/driver/src/baseClient.ts index a06dfff43..a246b0e42 100644 --- a/packages/driver/src/baseClient.ts +++ b/packages/driver/src/baseClient.ts @@ -35,7 +35,6 @@ import { } from "./ifaces"; import type { RetryOptions, - Session, SimpleRetryOptions, SimpleTransactionOptions, TransactionOptions, @@ -190,7 +189,7 @@ export class ClientConnectionHolder { args, outputFormat, expectedCardinality, - this.options.session, + this.options, false /* privilegedMode */, language, ); @@ -581,33 +580,28 @@ export class Client implements Executor { return new Client(this.pool, this.options.withRetryOptions(opts)); } - withSession(session: Session): Client { - return new Client(this.pool, this.options.withSession(session)); - } - withModuleAliases(aliases: Record) { return new Client( this.pool, - this.options.withSession(this.options.session.withModuleAliases(aliases)), + this.options.withModuleAliases(aliases), ); } withConfig(config: SimpleConfig): Client { - const newConfig = this.options.session.withConfig(config); - return new Client(this.pool, this.options.withSession(newConfig)); + return new Client(this.pool, this.options.withConfig(config)); } withGlobals(globals: Record): Client { return new Client( this.pool, - this.options.withSession(this.options.session.withGlobals(globals)), + this.options.withGlobals(globals), ); } withQueryTag(tag: string | null): Client { return new Client( this.pool, - this.options.withSession(this.options.session.withQueryTag(tag)), + this.options.withQueryTag(tag), ); } @@ -775,7 +769,7 @@ export class Client implements Executor { query, OutputFormat.BINARY, Cardinality.MANY, - this.options.session, + this.options, ); const cardinality = util.parseCardinality(result[0]); diff --git a/packages/driver/src/baseConn.ts b/packages/driver/src/baseConn.ts index bd2581fc5..419b4600d 100644 --- a/packages/driver/src/baseConn.ts +++ b/packages/driver/src/baseConn.ts @@ -48,7 +48,7 @@ import * as chars from "./primitives/chars"; import Event from "./primitives/event"; import LRU from "./primitives/lru"; import type { SerializedSessionState } from "./options"; -import { Session } from "./options"; +import { Options } from "./options"; export const PROTO_VER: ProtocolVersion = [3, 0]; export const PROTO_VER_MIN: ProtocolVersion = [0, 9]; @@ -138,7 +138,7 @@ export class BaseRawConnection { isLegacyProtocol = false; protected stateCodec: ICodec = INVALID_CODEC; - protected stateCache = new WeakMap(); + protected stateCache = new WeakMap(); lastStateUpdate: SerializedSessionState | null = null; protected adminUIMode = false; @@ -894,7 +894,7 @@ export class BaseRawConnection { query: string, outputFormat: OutputFormat, expectedCardinality: Cardinality, - state: Session, + state: Options, capabilitiesFlags: number, options: QueryOptions | undefined, language: Language, @@ -936,7 +936,7 @@ export class BaseRawConnection { ); wb.writeString(query); - if (!this.adminUIMode && state === Session.defaults()) { + if (!this.adminUIMode && state.isDefaultSession()) { wb.writeBuffer(NULL_CODEC.tidBuffer); wb.writeInt32(0); } else { @@ -959,7 +959,7 @@ export class BaseRawConnection { query: string, outputFormat: OutputFormat, expectedCardinality: Cardinality, - state: Session, + state: Options, capabilitiesFlags: number = RESTRICTED_CAPABILITIES, options?: QueryOptions, ): Promise { @@ -1082,7 +1082,7 @@ export class BaseRawConnection { args: QueryArgs, outputFormat: OutputFormat, expectedCardinality: Cardinality, - state: Session, + state: Options, inCodec: ICodec, outCodec: ICodec, result: any[] | WriteBuffer, @@ -1256,7 +1256,7 @@ export class BaseRawConnection { args: QueryArgs = null, outputFormat: OutputFormat, expectedCardinality: Cardinality, - state: Session, + state: Options, privilegedMode = false, language: Language = Language.EDGEQL, ): Promise<{ result: any; warnings: errors.EdgeDBError[] }> { @@ -1307,7 +1307,7 @@ export class BaseRawConnection { if ( (!inCodec && args !== null) || - (this.stateCodec === INVALID_CODEC && state !== Session.defaults()) + (this.stateCodec === INVALID_CODEC && !state.isDefaultSession()) ) { [card, inCodec, outCodec, _, _, _, warnings] = await this._parse( language, @@ -1353,7 +1353,7 @@ export class BaseRawConnection { } } } else { - if (state !== Session.defaults()) { + if (!state.isDefaultSession()) { throw new errors.InterfaceError( `setting session state is not supported in this version of ` + `EdgeDB. Upgrade to EdgeDB 2.0 or newer.`, @@ -1491,7 +1491,7 @@ export class BaseRawConnection { undefined, OutputFormat.NONE, Cardinality.NO_RESULT, - Session.defaults(), + Options.defaults(), true, ); } catch { diff --git a/packages/driver/src/fetchConn.ts b/packages/driver/src/fetchConn.ts index 6212a3df1..9d91fe033 100644 --- a/packages/driver/src/fetchConn.ts +++ b/packages/driver/src/fetchConn.ts @@ -40,7 +40,7 @@ import { type QueryArgs, type QueryOptions, } from "./ifaces"; -import type { Session } from "./options"; +import type { Options } from "./options"; import { WriteBuffer } from "./primitives/buffer"; import * as chars from "./primitives/chars"; import Event from "./primitives/event"; @@ -209,7 +209,7 @@ export class AdminUIFetchConnection extends BaseFetchConnection { public async rawParse( language: Language, query: string, - state: Session, + state: Options, options?: QueryOptions, abortSignal?: AbortSignal | null, ): Promise< @@ -239,7 +239,7 @@ export class AdminUIFetchConnection extends BaseFetchConnection { public async rawExecute( language: Language, query: string, - state: Session, + state: Options, outCodec?: ICodec, options?: QueryOptions, inCodec?: ICodec, diff --git a/packages/driver/src/ifaces.ts b/packages/driver/src/ifaces.ts index 12ac6518a..6f0ca1b20 100644 --- a/packages/driver/src/ifaces.ts +++ b/packages/driver/src/ifaces.ts @@ -59,6 +59,15 @@ export interface Executor { queryRequiredSingleJSON(query: string, args?: QueryArgs): Promise; } +interface UserCodec { + encode(value: any): any; + decode(value: any): any; +} + +export type UserCodecs = { + [typeName: string]: UserCodec; +} + export interface KnownServerSettings { suggested_pool_concurrency?: number; system_config?: any; diff --git a/packages/driver/src/index.node.ts b/packages/driver/src/index.node.ts index cdbc30398..6948a699d 100644 --- a/packages/driver/src/index.node.ts +++ b/packages/driver/src/index.node.ts @@ -31,7 +31,7 @@ export { IsolationLevel, RetryCondition, RetryOptions, - Session, + Options, } from "./options"; export { defaultBackoff, logWarnings, throwWarnings } from "./options"; export type { BackoffFunction } from "./options"; diff --git a/packages/driver/src/options.ts b/packages/driver/src/options.ts index a2263c224..90c93a345 100644 --- a/packages/driver/src/options.ts +++ b/packages/driver/src/options.ts @@ -1,4 +1,4 @@ -import * as errors from "./errors"; +import * as errors from "./errors/index"; import { utf8Encoder } from "./primitives/buffer"; export type BackoffFunction = (n: number) => number; @@ -54,8 +54,10 @@ export function logWarnings(warnings: errors.EdgeDBError[]) { } export class RetryOptions { + // This type is immutable. + readonly default: RetryRule; - private overrides: Map; + private overrides: ReadonlyMap; constructor(attempts = 3, backoff: BackoffFunction = defaultBackoff) { this.default = new RetryRule(attempts, backoff); @@ -90,10 +92,12 @@ export class RetryOptions { } static defaults(): RetryOptions { - return new RetryOptions(); + return _retryOptionsDefault; } } +const _retryOptionsDefault = new RetryOptions(); + export interface SimpleTransactionOptions { isolation?: IsolationLevel; readonly?: boolean; @@ -101,6 +105,8 @@ export interface SimpleTransactionOptions { } export class TransactionOptions { + // This type is immutable. + readonly isolation: IsolationLevel; readonly readonly: boolean; readonly deferrable: boolean; @@ -115,55 +121,130 @@ export class TransactionOptions { } static defaults(): TransactionOptions { - return new TransactionOptions(); + return _defaultTransactionOptions; } } +const _defaultTransactionOptions = new TransactionOptions(); + const TAG_ANNOTATION_KEY = "tag"; -export interface SessionOptions { +export interface SerializedSessionState { + module?: string; + aliases?: [string, string][]; + config?: { [name: string]: unknown }; + globals?: { [name: string]: unknown }; +} + +export interface OptionsList { module?: string; moduleAliases?: Record; config?: Record; globals?: Record; + retryOptions?: RetryOptions; + transactionOptions?: TransactionOptions; + warningHandler?: WarningHandler; } -export interface SerializedSessionState { - module?: string; - aliases?: [string, string][]; - config?: { [name: string]: unknown }; - globals?: { [name: string]: unknown }; +type Mutable = { + -readonly [K in keyof T]: T[K] } -export class Session { +export class Options { + // This type is immutable. + readonly module: string; - readonly moduleAliases: Record; - readonly config: Record; - readonly globals: Record; + readonly moduleAliases: ReadonlyMap; + readonly config: ReadonlyMap; + readonly globals: ReadonlyMap; + readonly retryOptions: RetryOptions; + readonly transactionOptions: TransactionOptions; + readonly warningHandler: WarningHandler; /** @internal */ - annotations = new Map(); + readonly annotations: ReadonlyMap = new Map(); get tag(): string | null { return this.annotations.get(TAG_ANNOTATION_KEY) ?? null; } constructor({ + retryOptions = RetryOptions.defaults(), + transactionOptions = TransactionOptions.defaults(), + warningHandler = logWarnings, module = "default", moduleAliases = {}, config = {}, globals = {}, - }: SessionOptions = {}) { + }: OptionsList = {}) { + this.retryOptions = retryOptions; + this.transactionOptions = transactionOptions; + this.warningHandler = warningHandler; this.module = module; - this.moduleAliases = moduleAliases; - this.config = config; - this.globals = globals; + this.moduleAliases = new Map(Object.entries(moduleAliases)); + this.config = new Map(Object.entries(config)); + this.globals = new Map(Object.entries(globals)); } - private _clone(mergeOptions: SessionOptions) { - const session = new Session({ ...this, ...mergeOptions }); - session.annotations = this.annotations; - return session; + private _cloneWith(mergeOptions: OptionsList) { + const clone: Mutable = Object.create(Options.prototype); + + clone.annotations = this.annotations; + + clone.retryOptions = mergeOptions.retryOptions ?? this.retryOptions; + clone.transactionOptions = + mergeOptions.transactionOptions ?? this.transactionOptions; + clone.warningHandler = mergeOptions.warningHandler ?? this.warningHandler; + + if (mergeOptions.config != null) { + clone.config = new Map( + [...this.config, ...Object.entries(mergeOptions.config)] + ); + } else { + clone.config = this.config; + } + + if (mergeOptions.globals != null) { + clone.globals = new Map( + [...this.globals, ...Object.entries(mergeOptions.globals)] + ); + } else { + clone.globals = this.globals; + } + + if (mergeOptions.moduleAliases != null) { + clone.moduleAliases = new Map( + [...this.moduleAliases, ...Object.entries(mergeOptions.moduleAliases)] + ); + } else { + clone.moduleAliases = this.moduleAliases; + } + + clone.module = mergeOptions.module ?? this.module; + + return clone as Options; + } + + /** @internal */ + _serialise() { + const state: SerializedSessionState = {}; + if (this.module !== "default") { + state.module = this.module; + } + if (this.moduleAliases.size) { + state.aliases = Array.from(this.moduleAliases.entries()); + } + if (this.config.size) { + state.config = Object.fromEntries(this.config.entries()); + } + if (this.globals.size) { + const globs: Record = {}; + for (let [key, val] of this.globals.entries()) { + globs[key.includes("::") ? key : `${this.module}::${key}`] = val; + } + state.globals = globs; + } + return state; } withModuleAliases({ @@ -171,28 +252,25 @@ export class Session { ...aliases }: { [name: string]: string; - }): Session { - return this._clone({ + }): Options { + return this._cloneWith({ module: module ?? this.module, - moduleAliases: { ...this.moduleAliases, ...aliases }, + moduleAliases: aliases, }); } - withConfig(config: { [name: string]: any }): Session { - return this._clone({ - config: { ...this.config, ...config }, - }); + withConfig(config: Record): Options { + return this._cloneWith({config}); } - withGlobals(globals: { [name: string]: any }): Session { - return this._clone({ + withGlobals(globals: Record): Options { + return this._cloneWith({ globals: { ...this.globals, ...globals }, }); } - withQueryTag(tag: string | null): Session { - const session = new Session({ ...this }); - session.annotations = new Map(this.annotations); + withQueryTag(tag: string | null): Options { + const annos = new Map(this.annotations); if (tag != null) { if (tag.startsWith("edgedb/")) { throw new errors.InterfaceError("reserved tag: edgedb/*"); @@ -203,82 +281,28 @@ export class Session { if (utf8Encoder.encode(tag).length > 128) { throw new errors.InterfaceError("tag too long (> 128 bytes)"); } - session.annotations.set(TAG_ANNOTATION_KEY, tag); + annos.set(TAG_ANNOTATION_KEY, tag); } else { - session.annotations.delete(TAG_ANNOTATION_KEY); - } - return session; - } - - /** @internal */ - _serialise() { - const state: SerializedSessionState = {}; - if (this.module !== "default") { - state.module = this.module; - } - const _aliases = Object.entries(this.moduleAliases); - if (_aliases.length) { - state.aliases = _aliases; - } - if (Object.keys(this.config).length) { - state.config = this.config; - } - const _globals = Object.entries(this.globals); - if (_globals.length) { - state.globals = _globals.reduce( - (globals, [key, val]) => { - globals[key.includes("::") ? key : `${this.module}::${key}`] = val; - return globals; - }, - {} as { [key: string]: any }, - ); + annos.delete(TAG_ANNOTATION_KEY); } - return state; - } - - static defaults(): Session { - return defaultSession; - } -} - -const defaultSession = new Session(); -export class Options { - readonly retryOptions: RetryOptions; - readonly transactionOptions: TransactionOptions; - readonly session: Session; - readonly warningHandler: WarningHandler; + const clone: Mutable = this._cloneWith({}); + clone.annotations = annos; - constructor({ - retryOptions = RetryOptions.defaults(), - transactionOptions = TransactionOptions.defaults(), - session = Session.defaults(), - warningHandler = logWarnings, - }: { - retryOptions?: RetryOptions; - transactionOptions?: TransactionOptions; - session?: Session; - warningHandler?: WarningHandler; - } = {}) { - this.retryOptions = retryOptions; - this.transactionOptions = transactionOptions; - this.session = session; - this.warningHandler = warningHandler; + return clone as Options; } withTransactionOptions( opt: TransactionOptions | SimpleTransactionOptions, ): Options { - return new Options({ - ...this, + return this._cloneWith({ transactionOptions: opt instanceof TransactionOptions ? opt : new TransactionOptions(opt), }); } withRetryOptions(opt: RetryOptions | SimpleRetryOptions): Options { - return new Options({ - ...this, + return this._cloneWith({ retryOptions: opt instanceof RetryOptions ? opt @@ -286,18 +310,23 @@ export class Options { }); } - withSession(opt: Session): Options { - return new Options({ - ...this, - session: opt, - }); + withWarningHandler(handler: WarningHandler): Options { + return this._cloneWith({ warningHandler: handler }); } - withWarningHandler(handler: WarningHandler): Options { - return new Options({ ...this, warningHandler: handler }); + isDefaultSession() { + return ( + this.config.size === 0 && + this.globals.size === 0 && + this.moduleAliases.size === 0 && + this.module === 'default' + ) } static defaults(): Options { - return new Options(); + return _defaultOptions; } } + + +const _defaultOptions = new Options(); diff --git a/packages/driver/src/transaction.ts b/packages/driver/src/transaction.ts index 665ed7d83..2eec3c566 100644 --- a/packages/driver/src/transaction.ts +++ b/packages/driver/src/transaction.ts @@ -68,7 +68,7 @@ export class Transaction implements Executor { undefined, OutputFormat.NONE, Cardinality.NO_RESULT, - holder.options.session, + holder.options, true, ); @@ -144,7 +144,7 @@ export class Transaction implements Executor { undefined, OutputFormat.NONE, Cardinality.NO_RESULT, - this._holder.options.session, + this._holder.options, true, ); this._state = TransactionState.COMMITTED; @@ -163,7 +163,7 @@ export class Transaction implements Executor { undefined, OutputFormat.NONE, Cardinality.NO_RESULT, - this._holder.options.session, + this._holder.options, true, ); this._state = TransactionState.ROLLEDBACK; @@ -179,7 +179,7 @@ export class Transaction implements Executor { args, OutputFormat.NONE, Cardinality.NO_RESULT, - this._holder.options.session, + this._holder.options, ); } @@ -190,7 +190,7 @@ export class Transaction implements Executor { args, OutputFormat.NONE, Cardinality.NO_RESULT, - this._holder.options.session, + this._holder.options, false /* privilegedMode */, Language.SQL, ); @@ -203,7 +203,7 @@ export class Transaction implements Executor { args, OutputFormat.BINARY, Cardinality.MANY, - this._holder.options.session, + this._holder.options, ); } @@ -217,7 +217,7 @@ export class Transaction implements Executor { args, OutputFormat.BINARY, Cardinality.MANY, - this._holder.options.session, + this._holder.options, false /* privilegedMode */, Language.SQL, ); @@ -230,7 +230,7 @@ export class Transaction implements Executor { args, OutputFormat.JSON, Cardinality.MANY, - this._holder.options.session, + this._holder.options, ); } @@ -244,7 +244,7 @@ export class Transaction implements Executor { args, OutputFormat.BINARY, Cardinality.AT_MOST_ONE, - this._holder.options.session, + this._holder.options, ); } @@ -255,7 +255,7 @@ export class Transaction implements Executor { args, OutputFormat.JSON, Cardinality.AT_MOST_ONE, - this._holder.options.session, + this._holder.options, ); } @@ -269,7 +269,7 @@ export class Transaction implements Executor { args, OutputFormat.BINARY, Cardinality.AT_LEAST_ONE, - this._holder.options.session, + this._holder.options, ); } @@ -280,7 +280,7 @@ export class Transaction implements Executor { args, OutputFormat.JSON, Cardinality.AT_LEAST_ONE, - this._holder.options.session, + this._holder.options, ); } @@ -294,7 +294,7 @@ export class Transaction implements Executor { args, OutputFormat.BINARY, Cardinality.ONE, - this._holder.options.session, + this._holder.options, ); } @@ -308,7 +308,7 @@ export class Transaction implements Executor { args, OutputFormat.JSON, Cardinality.ONE, - this._holder.options.session, + this._holder.options, ); } } diff --git a/packages/driver/test/client.test.ts b/packages/driver/test/client.test.ts index 875aad4c1..8384d0fdc 100644 --- a/packages/driver/test/client.test.ts +++ b/packages/driver/test/client.test.ts @@ -34,7 +34,7 @@ import { QueryArgumentError, _CodecsRegistry, _ReadBuffer, - Session, + Options, AuthenticationError, InvalidReferenceError, throwWarnings, @@ -2314,7 +2314,7 @@ if (!isDeno && getAvailableFeatures().has("binary-over-http")) { ); const query = `SELECT Function { name }`; - const state = new Session({ module: "schema" }); + const state = new Options({ module: "schema" }); const options = { injectTypenames: true, implicitLimit: BigInt(5), @@ -2355,7 +2355,7 @@ if (!isDeno && getAvailableFeatures().has("binary-over-http")) { ); await expect( - fetchConn.rawParse(Language.EDGEQL, `select 1`, Session.defaults()), + fetchConn.rawParse(Language.EDGEQL, `select 1`, Options.defaults()), ).rejects.toThrow(AuthenticationError); }); }