From f7137138afbc37ac12c18d4c330ee73899aef051 Mon Sep 17 00:00:00 2001 From: Michael Landis Date: Fri, 10 Jan 2025 15:13:48 -0800 Subject: [PATCH] fix: copy constructors should use spread operator In order to prevent bugs in our config copy constructors, we should use the spread operator over `this`. Previously in the copy constuctors we built a copy of the corresponding properties by hand. Because some properties are optional, and because we add new properties over time, we introduced drift between the new properties and various copy constructors. That is, when we added new properties, some copy constructors were out of date and hence falling back to defaults instead of copying those new properties over. We can fix this across the board by: 1. aligning the field names in each config class with the corresponding property name, and 2. building the props copy using a spread over `this` followed by the new value. --- .../src/config/configuration.ts | 66 ++++--------------- .../src/config/leaderboard-configuration.ts | 26 +++----- .../src/config/storage-configuration.ts | 13 ++-- .../src/config/topic-configuration.ts | 10 ++- .../transport/cache/transport-strategy.ts | 57 ++++++++-------- .../transport/storage/transport-strategy.ts | 9 ++- .../transport/topics/transport-strategy.ts | 17 +++-- .../src/config/configuration.ts | 33 +++------- .../src/config/leaderboard-configuration.ts | 13 ++-- .../src/config/topic-configuration.ts | 4 +- .../config/transport/transport-strategy.ts | 21 ++++-- 11 files changed, 100 insertions(+), 169 deletions(-) diff --git a/packages/client-sdk-nodejs/src/config/configuration.ts b/packages/client-sdk-nodejs/src/config/configuration.ts index 3e645c3c4..cd82ae6b1 100644 --- a/packages/client-sdk-nodejs/src/config/configuration.ts +++ b/packages/client-sdk-nodejs/src/config/configuration.ts @@ -183,13 +183,8 @@ export class CacheConfiguration implements Configuration { withRetryStrategy(retryStrategy: RetryStrategy): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, - retryStrategy: retryStrategy, - transportStrategy: this.transportStrategy, - middlewares: this.middlewares, - throwOnErrors: this.throwOnErrors, - readConcern: this.readConcern, - compression: this.compression, + ...this, + retryStrategy, }); } @@ -199,13 +194,8 @@ export class CacheConfiguration implements Configuration { withTransportStrategy(transportStrategy: TransportStrategy): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, - retryStrategy: this.retryStrategy, - transportStrategy: transportStrategy, - middlewares: this.middlewares, - throwOnErrors: this.throwOnErrors, - readConcern: this.readConcern, - compression: this.compression, + ...this, + transportStrategy, }); } @@ -225,38 +215,23 @@ export class CacheConfiguration implements Configuration { withMiddlewares(middlewares: Middleware[]): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, - retryStrategy: this.retryStrategy, - transportStrategy: this.transportStrategy, - middlewares: middlewares, - throwOnErrors: this.throwOnErrors, - readConcern: this.readConcern, - compression: this.compression, + ...this, + middlewares, }); } addMiddleware(middleware: Middleware): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, - retryStrategy: this.retryStrategy, - transportStrategy: this.transportStrategy, + ...this, middlewares: [middleware, ...this.middlewares], - throwOnErrors: this.throwOnErrors, - readConcern: this.readConcern, - compression: this.compression, }); } withClientTimeoutMillis(clientTimeout: number): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, - retryStrategy: this.retryStrategy, + ...this, transportStrategy: this.transportStrategy.withClientTimeoutMillis(clientTimeout), - middlewares: this.middlewares, - throwOnErrors: this.throwOnErrors, - readConcern: this.readConcern, - compression: this.compression, }); } @@ -266,13 +241,8 @@ export class CacheConfiguration implements Configuration { withThrowOnErrors(throwOnErrors: boolean): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, - retryStrategy: this.retryStrategy, - transportStrategy: this.transportStrategy, - middlewares: this.middlewares, - throwOnErrors: throwOnErrors, - readConcern: this.readConcern, - compression: this.compression, + ...this, + throwOnErrors, }); } @@ -282,13 +252,8 @@ export class CacheConfiguration implements Configuration { withReadConcern(readConcern: ReadConcern): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, - retryStrategy: this.retryStrategy, - transportStrategy: this.transportStrategy, - middlewares: this.middlewares, - throwOnErrors: this.throwOnErrors, - readConcern: readConcern, - compression: this.compression, + ...this, + readConcern, }); } @@ -300,12 +265,7 @@ export class CacheConfiguration implements Configuration { compressionStrategy: CompressionStrategy ): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, - retryStrategy: this.retryStrategy, - transportStrategy: this.transportStrategy, - middlewares: this.middlewares, - throwOnErrors: this.throwOnErrors, - readConcern: this.readConcern, + ...this, compression: compressionStrategy, }); } diff --git a/packages/client-sdk-nodejs/src/config/leaderboard-configuration.ts b/packages/client-sdk-nodejs/src/config/leaderboard-configuration.ts index 2b011c483..5899f239f 100644 --- a/packages/client-sdk-nodejs/src/config/leaderboard-configuration.ts +++ b/packages/client-sdk-nodejs/src/config/leaderboard-configuration.ts @@ -120,11 +120,9 @@ export class LeaderboardClientConfiguration clientTimeoutMillis: number ): LeaderboardConfiguration { return new LeaderboardClientConfiguration({ - loggerFactory: this.loggerFactory, + ...this, transportStrategy: this.transportStrategy.withClientTimeoutMillis(clientTimeoutMillis), - throwOnErrors: this.throwOnErrors, - middlewares: this.middlewares, }); } @@ -132,10 +130,8 @@ export class LeaderboardClientConfiguration transportStrategy: TransportStrategy ): LeaderboardConfiguration { return new LeaderboardClientConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: transportStrategy, - throwOnErrors: this.throwOnErrors, - middlewares: this.middlewares, + ...this, + transportStrategy, }); } @@ -145,10 +141,8 @@ export class LeaderboardClientConfiguration withThrowOnErrors(throwOnErrors: boolean): LeaderboardConfiguration { return new LeaderboardClientConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: this.transportStrategy, - throwOnErrors: throwOnErrors, - middlewares: this.middlewares, + ...this, + throwOnErrors, }); } @@ -158,19 +152,15 @@ export class LeaderboardClientConfiguration withMiddlewares(middlewares: Middleware[]): LeaderboardConfiguration { return new LeaderboardClientConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: this.transportStrategy, - middlewares: middlewares, - throwOnErrors: this.throwOnErrors, + ...this, + middlewares, }); } addMiddleware(middleware: Middleware): LeaderboardConfiguration { return new LeaderboardClientConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: this.transportStrategy, + ...this, middlewares: [middleware, ...this.middlewares], - throwOnErrors: this.throwOnErrors, }); } } diff --git a/packages/client-sdk-nodejs/src/config/storage-configuration.ts b/packages/client-sdk-nodejs/src/config/storage-configuration.ts index 8af6ad960..bdd2246e9 100644 --- a/packages/client-sdk-nodejs/src/config/storage-configuration.ts +++ b/packages/client-sdk-nodejs/src/config/storage-configuration.ts @@ -87,10 +87,9 @@ export class StorageClientConfiguration implements StorageConfiguration { withClientTimeoutMillis(clientTimeoutMillis: number): StorageConfiguration { return new StorageClientConfiguration({ - loggerFactory: this.loggerFactory, + ...this, transportStrategy: this.transportStrategy.withClientTimeoutMillis(clientTimeoutMillis), - retryStrategy: this.retryStrategy, }); } @@ -98,17 +97,15 @@ export class StorageClientConfiguration implements StorageConfiguration { transportStrategy: StorageTransportStrategy ): StorageConfiguration { return new StorageClientConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: transportStrategy, - retryStrategy: this.retryStrategy, + ...this, + transportStrategy, }); } withRetryStrategy(retryStrategy: RetryStrategy): StorageConfiguration { return new StorageClientConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: this.transportStrategy, - retryStrategy: retryStrategy, + ...this, + retryStrategy, }); } } diff --git a/packages/client-sdk-nodejs/src/config/topic-configuration.ts b/packages/client-sdk-nodejs/src/config/topic-configuration.ts index 539ad4e5e..69b09bb68 100644 --- a/packages/client-sdk-nodejs/src/config/topic-configuration.ts +++ b/packages/client-sdk-nodejs/src/config/topic-configuration.ts @@ -86,9 +86,8 @@ export class TopicClientConfiguration implements TopicConfiguration { transportStrategy: TopicTransportStrategy ): TopicConfiguration { return new TopicClientConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: transportStrategy, - throwOnErrors: this.throwOnErrors, + ...this, + transportStrategy, }); } @@ -108,9 +107,8 @@ export class TopicClientConfiguration implements TopicConfiguration { withThrowOnErrors(throwOnErrors: boolean): TopicConfiguration { return new TopicClientConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: this.transportStrategy, - throwOnErrors: throwOnErrors, + ...this, + throwOnErrors, }); } } diff --git a/packages/client-sdk-nodejs/src/config/transport/cache/transport-strategy.ts b/packages/client-sdk-nodejs/src/config/transport/cache/transport-strategy.ts index db4bbf29a..8954ebefb 100644 --- a/packages/client-sdk-nodejs/src/config/transport/cache/transport-strategy.ts +++ b/packages/client-sdk-nodejs/src/config/transport/cache/transport-strategy.ts @@ -137,17 +137,15 @@ export class StaticGrpcConfiguration implements GrpcConfiguration { withDeadlineMillis(deadlineMillis: number): StaticGrpcConfiguration { return new StaticGrpcConfiguration({ - deadlineMillis: deadlineMillis, - maxSessionMemoryMb: this.maxSessionMemoryMb, - numClients: this.numClients, + ...this, + deadlineMillis, }); } withMaxSessionMemoryMb(maxSessionMemoryMb: number): StaticGrpcConfiguration { return new StaticGrpcConfiguration({ - deadlineMillis: this.deadlineMillis, - maxSessionMemoryMb: maxSessionMemoryMb, - numClients: this.numClients, + ...this, + maxSessionMemoryMb, }); } @@ -165,9 +163,8 @@ export class StaticGrpcConfiguration implements GrpcConfiguration { withNumClients(numClients: number): GrpcConfiguration { return new StaticGrpcConfiguration({ - deadlineMillis: this.deadlineMillis, - maxSessionMemoryMb: this.maxSessionMemoryMb, - numClients: numClients, + ...this, + numClients, }); } @@ -177,38 +174,37 @@ export class StaticGrpcConfiguration implements GrpcConfiguration { withMaxConcurrentRequests(maxConcurrentRequests: number): GrpcConfiguration { return new StaticGrpcConfiguration({ - deadlineMillis: this.deadlineMillis, - maxSessionMemoryMb: this.maxSessionMemoryMb, - numClients: this.numClients, - maxConcurrentRequests: maxConcurrentRequests, + ...this, + maxConcurrentRequests, }); } } export class StaticTransportStrategy implements TransportStrategy { - private readonly grpcConfig: GrpcConfiguration; + private readonly grpcConfiguration: GrpcConfiguration; private readonly maxIdleMillis: number; private readonly maxClientAgeMillis?: number; constructor(props: TransportStrategyProps) { - this.grpcConfig = props.grpcConfiguration; + this.grpcConfiguration = props.grpcConfiguration; this.maxIdleMillis = props.maxIdleMillis; this.maxClientAgeMillis = props.maxClientAgeMillis; } getGrpcConfig(): GrpcConfiguration { - return this.grpcConfig; + return this.grpcConfiguration; } getMaxClientAgeMillis(): number | undefined { return this.maxClientAgeMillis; } - withGrpcConfig(grpcConfig: GrpcConfiguration): StaticTransportStrategy { + withGrpcConfig( + grpcConfiguration: GrpcConfiguration + ): StaticTransportStrategy { return new StaticTransportStrategy({ - grpcConfiguration: grpcConfig, - maxIdleMillis: this.maxIdleMillis, - maxClientAgeMillis: this.maxClientAgeMillis, + ...this, + grpcConfiguration, }); } @@ -218,39 +214,38 @@ export class StaticTransportStrategy implements TransportStrategy { withMaxIdleMillis(maxIdleMillis: number): TransportStrategy { return new StaticTransportStrategy({ - grpcConfiguration: this.grpcConfig, - maxIdleMillis: maxIdleMillis, - maxClientAgeMillis: this.maxClientAgeMillis, + ...this, + maxIdleMillis, }); } withMaxClientAgeMillis(maxClientAgeMillis: number): TransportStrategy { return new StaticTransportStrategy({ - grpcConfiguration: this.grpcConfig, - maxIdleMillis: this.maxIdleMillis, - maxClientAgeMillis: maxClientAgeMillis, + ...this, + maxClientAgeMillis, }); } withClientTimeoutMillis(clientTimeout: number): StaticTransportStrategy { return new StaticTransportStrategy({ - grpcConfiguration: this.grpcConfig.withDeadlineMillis(clientTimeout), - maxIdleMillis: this.maxIdleMillis, + ...this, + grpcConfiguration: + this.grpcConfiguration.withDeadlineMillis(clientTimeout), }); } getMaxConcurrentRequests(): number | undefined { - return this.grpcConfig.getMaxConcurrentRequests(); + return this.grpcConfiguration.getMaxConcurrentRequests(); } withMaxConcurrentRequests( maxConcurrentRequests: number ): StaticTransportStrategy { return new StaticTransportStrategy({ - grpcConfiguration: this.grpcConfig.withMaxConcurrentRequests( + ...this, + grpcConfiguration: this.grpcConfiguration.withMaxConcurrentRequests( maxConcurrentRequests ), - maxIdleMillis: this.maxIdleMillis, }); } } diff --git a/packages/client-sdk-nodejs/src/config/transport/storage/transport-strategy.ts b/packages/client-sdk-nodejs/src/config/transport/storage/transport-strategy.ts index e3c45f01a..c1e14fe87 100644 --- a/packages/client-sdk-nodejs/src/config/transport/storage/transport-strategy.ts +++ b/packages/client-sdk-nodejs/src/config/transport/storage/transport-strategy.ts @@ -52,7 +52,8 @@ export class StaticStorageGrpcConfiguration withDeadlineMillis(deadlineMillis: number): StorageGrpcConfiguration { return new StaticStorageGrpcConfiguration({ - deadlineMillis: deadlineMillis, + ...this, + deadlineMillis, }); } } @@ -71,10 +72,11 @@ export class StaticStorageTransportStrategy } withGrpcConfig( - grpcConfig: StorageGrpcConfiguration + grpcConfiguration: StorageGrpcConfiguration ): StorageTransportStrategy { return new StaticStorageTransportStrategy({ - grpcConfiguration: grpcConfig, + ...this, + grpcConfiguration, }); } @@ -82,6 +84,7 @@ export class StaticStorageTransportStrategy clientTimeoutMillis: number ): StorageTransportStrategy { return new StaticStorageTransportStrategy({ + ...this, grpcConfiguration: this.grpcConfig.withDeadlineMillis(clientTimeoutMillis), }); diff --git a/packages/client-sdk-nodejs/src/config/transport/topics/transport-strategy.ts b/packages/client-sdk-nodejs/src/config/transport/topics/transport-strategy.ts index 00d9ab3a8..8d43c88c4 100644 --- a/packages/client-sdk-nodejs/src/config/transport/topics/transport-strategy.ts +++ b/packages/client-sdk-nodejs/src/config/transport/topics/transport-strategy.ts @@ -50,10 +50,8 @@ export class StaticTopicGrpcConfiguration implements TopicGrpcConfiguration { withNumClients(numClients: number): TopicGrpcConfiguration { return new StaticTopicGrpcConfiguration({ - numClients: numClients, - keepAlivePermitWithoutCalls: this.keepAlivePermitWithoutCalls, - keepAliveTimeoutMs: this.keepAliveTimeoutMs, - keepAliveTimeMs: this.keepAliveTimeMs, + ...this, + numClients, }); } @@ -71,21 +69,22 @@ export class StaticTopicGrpcConfiguration implements TopicGrpcConfiguration { } export class StaticTopicTransportStrategy implements TopicTransportStrategy { - private readonly grpcConfig: TopicGrpcConfiguration; + private readonly grpcConfiguration: TopicGrpcConfiguration; constructor(props: TopicTransportStrategyProps) { - this.grpcConfig = props.grpcConfiguration; + this.grpcConfiguration = props.grpcConfiguration; } getGrpcConfig(): TopicGrpcConfiguration { - return this.grpcConfig; + return this.grpcConfiguration; } withGrpcConfig( - grpcConfig: TopicGrpcConfiguration + grpcConfiguration: TopicGrpcConfiguration ): StaticTopicTransportStrategy { return new StaticTopicTransportStrategy({ - grpcConfiguration: grpcConfig, + ...this, + grpcConfiguration, }); } } diff --git a/packages/client-sdk-web/src/config/configuration.ts b/packages/client-sdk-web/src/config/configuration.ts index a89927b34..baf928f90 100644 --- a/packages/client-sdk-web/src/config/configuration.ts +++ b/packages/client-sdk-web/src/config/configuration.ts @@ -126,11 +126,8 @@ export class CacheConfiguration implements Configuration { withTransportStrategy(transportStrategy: TransportStrategy): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: transportStrategy, - middlewares: this.middlewares, - throwOnErrors: this.throwOnErrors, - readConcern: this.readConcern, + ...this, + transportStrategy, }); } @@ -140,22 +137,16 @@ export class CacheConfiguration implements Configuration { withMiddlewares(middlewares: Middleware[]): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: this.transportStrategy, - middlewares: middlewares, - throwOnErrors: this.throwOnErrors, - readConcern: this.readConcern, + ...this, + middlewares, }); } withClientTimeoutMillis(clientTimeout: number): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, + ...this, transportStrategy: this.transportStrategy.withClientTimeoutMillis(clientTimeout), - middlewares: this.middlewares, - throwOnErrors: this.throwOnErrors, - readConcern: this.readConcern, }); } @@ -165,11 +156,8 @@ export class CacheConfiguration implements Configuration { withThrowOnErrors(throwOnErrors: boolean): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: this.transportStrategy, - middlewares: this.middlewares, - throwOnErrors: throwOnErrors, - readConcern: this.readConcern, + ...this, + throwOnErrors, }); } @@ -179,11 +167,8 @@ export class CacheConfiguration implements Configuration { withReadConcern(readConcern: ReadConcern): Configuration { return new CacheConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: this.transportStrategy, - middlewares: this.middlewares, - throwOnErrors: this.throwOnErrors, - readConcern: readConcern, + ...this, + readConcern, }); } } diff --git a/packages/client-sdk-web/src/config/leaderboard-configuration.ts b/packages/client-sdk-web/src/config/leaderboard-configuration.ts index 615c1abd8..55cce47e9 100644 --- a/packages/client-sdk-web/src/config/leaderboard-configuration.ts +++ b/packages/client-sdk-web/src/config/leaderboard-configuration.ts @@ -84,10 +84,9 @@ export class LeaderboardClientConfiguration clientTimeoutMillis: number ): LeaderboardConfiguration { return new LeaderboardClientConfiguration({ - loggerFactory: this.loggerFactory, + ...this, transportStrategy: this.transportStrategy.withClientTimeoutMillis(clientTimeoutMillis), - throwOnErrors: this.throwOnErrors, }); } @@ -95,9 +94,8 @@ export class LeaderboardClientConfiguration transportStrategy: TransportStrategy ): LeaderboardConfiguration { return new LeaderboardClientConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: transportStrategy, - throwOnErrors: this.throwOnErrors, + ...this, + transportStrategy, }); } @@ -107,9 +105,8 @@ export class LeaderboardClientConfiguration withThrowOnErrors(throwOnErrors: boolean): LeaderboardConfiguration { return new LeaderboardClientConfiguration({ - loggerFactory: this.loggerFactory, - transportStrategy: this.transportStrategy, - throwOnErrors: throwOnErrors, + ...this, + throwOnErrors, }); } } diff --git a/packages/client-sdk-web/src/config/topic-configuration.ts b/packages/client-sdk-web/src/config/topic-configuration.ts index 3baf1e21b..99eb2ae07 100644 --- a/packages/client-sdk-web/src/config/topic-configuration.ts +++ b/packages/client-sdk-web/src/config/topic-configuration.ts @@ -60,8 +60,8 @@ export class TopicClientConfiguration implements TopicConfiguration { withThrowOnErrors(throwOnErrors: boolean): TopicConfiguration { return new TopicClientConfiguration({ - loggerFactory: this.loggerFactory, - throwOnErrors: throwOnErrors, + ...this, + throwOnErrors, }); } } diff --git a/packages/client-sdk-web/src/config/transport/transport-strategy.ts b/packages/client-sdk-web/src/config/transport/transport-strategy.ts index 5a9c497e4..b7161d801 100644 --- a/packages/client-sdk-web/src/config/transport/transport-strategy.ts +++ b/packages/client-sdk-web/src/config/transport/transport-strategy.ts @@ -32,6 +32,7 @@ export interface TransportStrategyProps { export class StaticGrpcConfiguration implements GrpcConfiguration { private readonly deadlineMillis: number; + constructor(props: GrpcConfigurationProps) { this.deadlineMillis = props.deadlineMillis; } @@ -42,31 +43,37 @@ export class StaticGrpcConfiguration implements GrpcConfiguration { withDeadlineMillis(deadlineMillis: number): StaticGrpcConfiguration { return new StaticGrpcConfiguration({ - deadlineMillis: deadlineMillis, + ...this, + deadlineMillis, }); } } export class StaticTransportStrategy implements TransportStrategy { - private readonly grpcConfig: GrpcConfiguration; + private readonly grpcConfiguration: GrpcConfiguration; constructor(props: TransportStrategyProps) { - this.grpcConfig = props.grpcConfiguration; + this.grpcConfiguration = props.grpcConfiguration; } getGrpcConfig(): GrpcConfiguration { - return this.grpcConfig; + return this.grpcConfiguration; } - withGrpcConfig(grpcConfig: GrpcConfiguration): StaticTransportStrategy { + withGrpcConfig( + grpcConfiguration: GrpcConfiguration + ): StaticTransportStrategy { return new StaticTransportStrategy({ - grpcConfiguration: grpcConfig, + ...this, + grpcConfiguration, }); } withClientTimeoutMillis(clientTimeout: number): StaticTransportStrategy { return new StaticTransportStrategy({ - grpcConfiguration: this.grpcConfig.withDeadlineMillis(clientTimeout), + ...this, + grpcConfiguration: + this.grpcConfiguration.withDeadlineMillis(clientTimeout), }); } }