Conversation
- Added optional `getBackendIdentifier?()` method to `BaseNotificationBackend` interface. - Updated Prisma and Medplum backends to support custom identifiers with defaults. - Created tests for backend identifier behaviors ensuring backward compatibility. - Updated documentation for Phase 1 progress and acceptance criteria.
…al predefined IDs
…ror handling and replication logging
…kend identifier management and initial test coverage
… and stats APIs; add comprehensive test coverage for edge cases
…DME, add migration guide, and implement example validation tests
Contributor
Reviewer's GuideIntroduces multi-backend support to VintaSend so a primary notification backend can be used together with additional backends, with primary-first writes, backend-targeted reads, and new sync/migration utilities, plus updated types, interface, docs, and extensive tests. Sequence diagram for multi-backend write with best-effort replicationsequenceDiagram
actor App
participant VintaSend as VintaSend
participant PrimaryBackend as PrimaryBackend
participant AdditionalBackend1 as AdditionalBackend1
participant AdditionalBackend2 as AdditionalBackend2
App->>VintaSend: createNotification(notification)
activate VintaSend
VintaSend->>VintaSend: executeMultiBackendWrite("createNotification", primaryWrite, additionalWrite)
%% Primary write
VintaSend->>PrimaryBackend: persistNotification(notification)
activate PrimaryBackend
PrimaryBackend-->>VintaSend: createdNotification
deactivate PrimaryBackend
%% Additional backends replication
loop for each additionalBackend in getAdditionalBackends()
VintaSend->>AdditionalBackend1: persistNotification(notification with id)
activate AdditionalBackend1
AdditionalBackend1-->>VintaSend: void or error
deactivate AdditionalBackend1
VintaSend->>AdditionalBackend2: persistNotification(notification with id)
activate AdditionalBackend2
AdditionalBackend2-->>VintaSend: void or error
deactivate AdditionalBackend2
end
VintaSend-->>App: createdNotification
deactivate VintaSend
Sequence diagram for backend-targeted read with optional backendIdentifiersequenceDiagram
actor Admin
participant VintaSend as VintaSend
participant PrimaryBackend as PrimaryBackend
participant ReplicaBackend as ReplicaBackend
Admin->>VintaSend: getNotification(notificationId)
activate VintaSend
VintaSend->>VintaSend: getBackend(undefined)
VintaSend-->>VintaSend: PrimaryBackend
VintaSend->>PrimaryBackend: getNotification(notificationId, false)
PrimaryBackend-->>VintaSend: notification
VintaSend-->>Admin: notification
deactivate VintaSend
Admin->>VintaSend: getNotification(notificationId, false, "replica-backend")
activate VintaSend
VintaSend->>VintaSend: getBackend("replica-backend")
VintaSend-->>VintaSend: ReplicaBackend
VintaSend->>ReplicaBackend: getNotification(notificationId, false)
ReplicaBackend-->>VintaSend: notificationReplica
VintaSend-->>Admin: notificationReplica
deactivate VintaSend
Updated class diagram for VintaSend multi-backend supportclassDiagram
class VintaSendFactory {
+create(adapters, backend, logger, contextGeneratorsMap, attachmentManager, options, gitCommitShaProvider, additionalBackends) VintaSend
}
class VintaSend {
-adapters AdaptersList
-backend Backend
-queueService QueueService
-attachmentManager AttachmentMgr
-logger Logger
-options VintaSendOptions
-gitCommitShaProvider BaseGitCommitShaProvider
-contextGeneratorsMap NotificationContextGeneratorsMap
-backends Map~string, Backend~
-primaryBackendIdentifier string
+registerQueueService(queueService) void
+getPrimaryBackendIdentifier() string
+getAllBackendIdentifiers() string[]
+getAdditionalBackendIdentifiers() string[]
+hasBackend(identifier) boolean
+createNotification(notification) Promise~DatabaseNotification~
+updateNotification(notificationId, notification) Promise~DatabaseNotification~
+createOneOffNotification(notification) Promise~DatabaseOneOffNotification~
+updateOneOffNotification(notificationId, notification) Promise~DatabaseOneOffNotification~
+getAllFutureNotifications(backendIdentifier) Promise~DatabaseNotification[]~
+getAllFutureNotificationsFromUser(userId, backendIdentifier) Promise~DatabaseNotification[]~
+getFutureNotificationsFromUser(userId, page, pageSize, backendIdentifier) Promise~DatabaseNotification[]~
+getFutureNotifications(page, pageSize, backendIdentifier) Promise~DatabaseNotification[]~
+getPendingNotifications(page, pageSize, backendIdentifier) Promise~AnyDatabaseNotification[]~
+getNotifications(page, pageSize, backendIdentifier) Promise~AnyDatabaseNotification[]~
+getOneOffNotifications(page, pageSize, backendIdentifier) Promise~DatabaseOneOffNotification[]~
+getNotification(notificationId, forUpdate, backendIdentifier) Promise~DatabaseNotification~
+filterNotifications(filter, page, pageSize, backendIdentifier) Promise~DatabaseNotification[]~
+getBackendSupportedFilterCapabilities(backendIdentifier) Promise~BackendFilterCapabilities~
+getOneOffNotification(notificationId, forUpdate, backendIdentifier) Promise~DatabaseOneOffNotification | null~
+getInAppUnread(userId, backendIdentifier) Promise~DatabaseNotification[]~
+markRead(notificationId, checkIsSent) Promise~DatabaseNotification~
+cancelNotification(notificationId) Promise~void~
+bulkPersistNotifications(notifications) Promise~NotificationIdType[]~
+verifyNotificationSync(notificationId) Promise~NotificationSyncReport~
+replicateNotification(notificationId) Promise~ReplicationResult~
+getBackendSyncStats() Promise~BackendSyncStats~
+migrateToBackend(destinationBackend, batchSize, sourceBackendIdentifier) Promise~void~
-getBackendIdentifier(backend) string
-getBackend(identifier) Backend
-getAdditionalBackends() Backend[]
-executeMultiBackendWrite(operation, primaryWrite, additionalWrite) Promise~T~
-normalizeValueForSyncComparison(value) string
}
class BaseNotificationBackend {
<<interface>>
+getBackendIdentifier() string
+getAllPendingNotifications() Promise~AnyDatabaseNotification[]~
+getPendingNotifications(page, pageSize) Promise~AnyDatabaseNotification[]~
+getAllFutureNotifications() Promise~DatabaseNotification[]~
+getAllFutureNotificationsFromUser(userId) Promise~DatabaseNotification[]~
+getFutureNotificationsFromUser(userId, page, pageSize) Promise~DatabaseNotification[]~
+getFutureNotifications(page, pageSize) Promise~DatabaseNotification[]~
+persistNotification(notification) Promise~DatabaseNotification~
+getAllNotifications() Promise~AnyDatabaseNotification[]~
+getNotifications(page, pageSize) Promise~AnyDatabaseNotification[]~
+persistOneOffNotification(notification) Promise~DatabaseOneOffNotification~
+persistOneOffNotificationUpdate(notificationId, notification) Promise~DatabaseOneOffNotification~
+persistNotificationUpdate(notificationId, notification) Promise~DatabaseNotification~
+markAsFailed(notificationId, checkIsSent) Promise~DatabaseNotification~
+markAsSent(notificationId, checkIsSent) Promise~DatabaseNotification~
+markAsRead(notificationId, checkIsSent) Promise~DatabaseNotification~
+storeAdapterAndContextUsed(notificationId, adapterKey, context) Promise~void~
+cancelNotification(notificationId) Promise~void~
+filterNotifications(filter, page, pageSize) Promise~DatabaseNotification[]~
+filterAllInAppUnreadNotifications(userId) Promise~DatabaseNotification[]~
+getFilterCapabilities() BackendFilterCapabilities
}
class NotificationInput {
+id NotificationIdType
+userId UserIdType
+notificationType NotificationType
+title string
+bodyTemplate string
+subjectTemplate string
+contextName string
+contextParameters any
+sendAfter Date
}
class NotificationResendWithContextInput {
+id NotificationIdType
+userId UserIdType
+notificationType NotificationType
+title string
+bodyTemplate string
+subjectTemplate string
+contextName string
+contextParameters any
}
class OneOffNotificationInput {
+id NotificationIdType
+emailOrPhone string
+firstName string
+lastName string
+notificationType NotificationType
+sendAfter Date
}
class OneOffNotificationResendWithContextInput {
+id NotificationIdType
+emailOrPhone string
+firstName string
+lastName string
+notificationType NotificationType
}
VintaSendFactory --> VintaSend
VintaSend --> BaseNotificationBackend : uses
VintaSend --> NotificationInput : creates
VintaSend --> OneOffNotificationInput : creates
VintaSend --> NotificationResendWithContextInput : uses
VintaSend --> OneOffNotificationResendWithContextInput : uses
BaseNotificationBackend <|.. PrimaryBackend
BaseNotificationBackend <|.. AdditionalBackend
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The fallback implementation of
getBackendIdentifier(backend-${this.backends.size}) is inconsistent with how you key thebackendsmap, so for backends without a custom identifier the identifier used in logs/replication may not match the actual map key; consider deriving the fallback identifier from the existing map key rather than recomputing it fromthis.backends.size. - The post-construction validation
if (this.getAdditionalBackends().length !== additionalBackends.length)in theVintaSendconstructor appears redundant becausegetAdditionalBackendsis derived directly from thebackendsmap you just populated fromadditionalBackends; if you intended to validate something else (e.g., uniqueness or identifier presence), consider making that check explicit or removing this block.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The fallback implementation of `getBackendIdentifier` (`backend-${this.backends.size}`) is inconsistent with how you key the `backends` map, so for backends without a custom identifier the identifier used in logs/replication may not match the actual map key; consider deriving the fallback identifier from the existing map key rather than recomputing it from `this.backends.size`.
- The post-construction validation `if (this.getAdditionalBackends().length !== additionalBackends.length)` in the `VintaSend` constructor appears redundant because `getAdditionalBackends` is derived directly from the `backends` map you just populated from `additionalBackends`; if you intended to validate something else (e.g., uniqueness or identifier presence), consider making that check explicit or removing this block.
## Individual Comments
### Comment 1
<location path="src/services/notification-service.ts" line_range="288-293" />
<code_context>
}
}
+ private getBackendIdentifier(backend: Backend): string {
+ if (typeof backend.getBackendIdentifier === 'function') {
+ return backend.getBackendIdentifier();
+ }
+
+ return `backend-${this.backends.size}`;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** getBackendIdentifier uses this.backends.size, which leads to unstable and misleading identifiers after initialization.
Because this relies on `this.backends.size`, all backends without their own `getBackendIdentifier` will share the same ID after initialization. This breaks user-facing reporting that depends on stable identifiers (e.g. `replicateNotification` successes/failures, replication logs), and those IDs won’t match `getAllBackendIdentifiers`/`hasBackend` keys. Please generate a stable ID when inserting the backend (and reuse it here), or require each backend to implement `getBackendIdentifier` in multi-backend mode.
</issue_to_address>
### Comment 2
<location path="src/services/notification-service.ts" line_range="1072-1081" />
<code_context>
async bulkPersistNotifications(
notifications: Omit<AnyNotification<Config>, 'id'>[],
): Promise<Config['NotificationIdType'][]> {
- return this.backend.bulkPersistNotifications(notifications);
+ return this.executeMultiBackendWrite(
+ 'bulkPersistNotifications',
+ async (backend) => {
+ return backend.bulkPersistNotifications(notifications);
+ },
+ async (backend, createdIds) => {
+ const notificationsWithIds = notifications.map((notification, index) => {
+ return {
+ ...notification,
+ id: createdIds[index],
+ };
+ });
+
+ await backend.bulkPersistNotifications(
+ notificationsWithIds as unknown as Omit<AnyNotification<Config>, 'id'>[],
+ );
</code_context>
<issue_to_address>
**issue (bug_risk):** Replicating bulkPersistNotifications by passing notifications with IDs into a method typed as accepting inputs without IDs is ambiguous and likely to desynchronize IDs.
In `bulkPersistNotifications` you construct `notificationsWithIds` using the generated IDs, then cast them to `Omit<AnyNotification<Config>, 'id'>[]` when calling `backend.bulkPersistNotifications`. This lies to the type system and uses an API whose contract still appears to be “backend generates IDs”. Backends that assume they own ID generation may ignore or override the provided IDs, breaking cross-backend synchronization. Consider either extending the backend bulk API to explicitly accept IDs (mirroring `persistNotification`) or performing per-notification writes through the `id?`-aware `persistNotification` instead of casting around a “create without ID” interface.
</issue_to_address>
### Comment 3
<location path="src/services/__tests__/multi-backend-management.test.ts" line_range="201-196" />
<code_context>
+ it('verifyNotificationSync reports additional field mismatches', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Extend verifyNotificationSync tests to cover normalization of complex field types (dates, objects, null/undefined).
The tests currently cover string fields and status mismatches, but the implementation also normalizes values via `normalizeValueForSyncComparison` (dates, null/undefined, and objects via `JSON.stringify`). Please add tests that exercise this behavior by:
- comparing different `Date` instances in fields like `sendAfter`, `createdAt`, or `updatedAt`,
- introducing differences in structured fields such as `contextParameters` or `extraParams`, and
- including at least one case that distinguishes `null` vs `undefined`.
This will better protect the non-primitive comparison path from regressions.
</issue_to_address>
### Comment 4
<location path="src/services/__tests__/multi-backend-error-handling.test.ts" line_range="58-67" />
<code_context>
+function createMockBackend(identifier: string): MockBackend {
</code_context>
<issue_to_address>
**suggestion:** Consider deduplicating createMockBackend and shared test setup across multi-backend test suites.
This helper (and the associated logger/templateRenderer/adapter setup) is duplicated across several suites (`multi-backend-error-handling.test.ts`, `multi-backend-writes.test.ts`, `multi-backend-reads.test.ts`, `multi-backend-management.test.ts`, plus the example tests). Extracting these into a shared test utility (e.g., `__tests__/helpers/multi-backend-mocks.ts`) would centralize backend/mock configuration, clarify each suite’s intent, and reduce divergence risk as the multi-backend API evolves. Non-blocking, but a worthwhile cleanup as the surface area grows.
</issue_to_address>
### Comment 5
<location path="src/services/notification-service.ts" line_range="333" />
<code_context>
+ return this.backends.has(identifier);
+ }
+
+ private async executeMultiBackendWrite<T>(
+ operation: string,
+ primaryWrite: (backend: Backend) => Promise<T>,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the multi-backend write path by adding a defaulted `executeMultiBackendWrite` overload and stabilizing backend identifier generation to cut duplicated logic and make backend IDs easier to reason about.
A couple of focused tweaks would significantly reduce complexity without changing behavior.
---
### 1. `executeMultiBackendWrite` API – remove duplicated lambdas at call sites
Most usages pass two lambdas with identical logic, e.g.:
```ts
await this.executeMultiBackendWrite(
'markAsFailed',
async (backend) => backend.markAsFailed(id, true),
async (backend) => backend.markAsFailed(id, true),
);
```
You can keep the “derived secondaries” capability but add a simpler overload where primary/additional behaviors are the same. That removes a lot of noisy boilerplate and makes intent clearer.
**Concrete change:**
```ts
private async executeMultiBackendWrite<T>(
operation: string,
primaryWrite: (backend: Backend) => Promise<T>,
additionalWrite?: (backend: Backend, primaryResult: T) => Promise<void>,
): Promise<T> {
const primaryResult = await primaryWrite(this.backend);
const replicationFn =
additionalWrite ??
(async (backend: Backend, result: T) => {
await primaryWrite(backend);
});
for (const additionalBackend of this.getAdditionalBackends()) {
const backendIdentifier = this.getBackendIdentifier(additionalBackend);
try {
await replicationFn(additionalBackend, primaryResult);
this.logger.info(`${operation} replicated to backend ${backendIdentifier}`);
} catch (replicationError) {
this.logger.error(
`Failed to replicate ${operation} to backend ${backendIdentifier}: ${replicationError}`,
);
}
}
return primaryResult;
}
```
Then all symmetric cases collapse to:
```ts
await this.executeMultiBackendWrite(
'markAsFailed',
(backend) => backend.markAsFailed(notificationWithExecutionGitCommitSha.id, true),
);
```
This preserves the ability to pass a custom `additionalWrite` when secondary behavior depends on the primary result (e.g. ID replication).
---
### 2. Backend identifier generation – make it stable and avoid recomputation
`getBackendIdentifier` currently depends on `this.backends.size`:
```ts
private getBackendIdentifier(backend: Backend): string {
if (typeof backend.getBackendIdentifier === 'function') {
return backend.getBackendIdentifier();
}
return `backend-${this.backends.size}`;
}
```
This can produce different identifiers for the same instance if called before/after map mutations, and it’s also re-derived in multiple places (including logging), which makes reasoning about identifiers harder.
You can keep the current behavior but ensure identifiers are **stable per backend instance** by caching them:
```ts
// Add field
private backendIdentifiers = new WeakMap<Backend, string>();
private getBackendIdentifier(backend: Backend): string {
const cached = this.backendIdentifiers.get(backend);
if (cached) return cached;
const identifier =
typeof backend.getBackendIdentifier === 'function'
? backend.getBackendIdentifier()
: `backend-${this.backendIdentifiers.size}`;
this.backendIdentifiers.set(backend, identifier);
return identifier;
}
```
That way:
- Each backend instance gets one stable identifier.
- Calls in `constructor`, `getAdditionalBackends`, and logging all use the same value.
- You can drop the constructor post-condition:
```ts
if (this.getAdditionalBackends().length !== additionalBackends.length) {
throw new Error('Invalid additional backends configuration');
}
```
since `backends` will be a direct reflection of the backends you added, and `getAdditionalBackends` is a pure projection of that map.
This keeps all current functionality while reducing surprising behavior and coupling between methods.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+288
to
+293
| private getBackendIdentifier(backend: Backend): string { | ||
| if (typeof backend.getBackendIdentifier === 'function') { | ||
| return backend.getBackendIdentifier(); | ||
| } | ||
|
|
||
| return `backend-${this.backends.size}`; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): getBackendIdentifier uses this.backends.size, which leads to unstable and misleading identifiers after initialization.
Because this relies on this.backends.size, all backends without their own getBackendIdentifier will share the same ID after initialization. This breaks user-facing reporting that depends on stable identifiers (e.g. replicateNotification successes/failures, replication logs), and those IDs won’t match getAllBackendIdentifiers/hasBackend keys. Please generate a stable ID when inserting the backend (and reuse it here), or require each backend to implement getBackendIdentifier in multi-backend mode.
Comment on lines
1072
to
+1081
| async bulkPersistNotifications( | ||
| notifications: Omit<AnyNotification<Config>, 'id'>[], | ||
| ): Promise<Config['NotificationIdType'][]> { | ||
| return this.backend.bulkPersistNotifications(notifications); | ||
| return this.executeMultiBackendWrite( | ||
| 'bulkPersistNotifications', | ||
| async (backend) => { | ||
| return backend.bulkPersistNotifications(notifications); | ||
| }, | ||
| async (backend, createdIds) => { | ||
| const notificationsWithIds = notifications.map((notification, index) => { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Replicating bulkPersistNotifications by passing notifications with IDs into a method typed as accepting inputs without IDs is ambiguous and likely to desynchronize IDs.
In bulkPersistNotifications you construct notificationsWithIds using the generated IDs, then cast them to Omit<AnyNotification<Config>, 'id'>[] when calling backend.bulkPersistNotifications. This lies to the type system and uses an API whose contract still appears to be “backend generates IDs”. Backends that assume they own ID generation may ignore or override the provided IDs, breaking cross-backend synchronization. Consider either extending the backend bulk API to explicitly accept IDs (mirroring persistNotification) or performing per-notification writes through the id?-aware persistNotification instead of casting around a “create without ID” interface.
| const report = await service.verifyNotificationSync('notif-1'); | ||
|
|
||
| expect(report.synced).toBe(false); | ||
| expect(report.discrepancies).toContain( |
Contributor
There was a problem hiding this comment.
suggestion (testing): Extend verifyNotificationSync tests to cover normalization of complex field types (dates, objects, null/undefined).
The tests currently cover string fields and status mismatches, but the implementation also normalizes values via normalizeValueForSyncComparison (dates, null/undefined, and objects via JSON.stringify). Please add tests that exercise this behavior by:
- comparing different
Dateinstances in fields likesendAfter,createdAt, orupdatedAt, - introducing differences in structured fields such as
contextParametersorextraParams, and - including at least one case that distinguishes
nullvsundefined.
This will better protect the non-primitive comparison path from regressions.
Comment on lines
+58
to
+67
| function createMockBackend(identifier: string): MockBackend { | ||
| return { | ||
| persistNotification: jest.fn(), | ||
| persistNotificationUpdate: jest.fn(), | ||
| getAllFutureNotifications: jest.fn(), | ||
| getAllFutureNotificationsFromUser: jest.fn(), | ||
| getFutureNotificationsFromUser: jest.fn(), | ||
| getFutureNotifications: jest.fn(), | ||
| getAllPendingNotifications: jest.fn(), | ||
| getPendingNotifications: jest.fn(), |
Contributor
There was a problem hiding this comment.
suggestion: Consider deduplicating createMockBackend and shared test setup across multi-backend test suites.
This helper (and the associated logger/templateRenderer/adapter setup) is duplicated across several suites (multi-backend-error-handling.test.ts, multi-backend-writes.test.ts, multi-backend-reads.test.ts, multi-backend-management.test.ts, plus the example tests). Extracting these into a shared test utility (e.g., __tests__/helpers/multi-backend-mocks.ts) would centralize backend/mock configuration, clarify each suite’s intent, and reduce divergence risk as the multi-backend API evolves. Non-blocking, but a worthwhile cleanup as the surface area grows.
| return this.backends.has(identifier); | ||
| } | ||
|
|
||
| private async executeMultiBackendWrite<T>( |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider simplifying the multi-backend write path by adding a defaulted executeMultiBackendWrite overload and stabilizing backend identifier generation to cut duplicated logic and make backend IDs easier to reason about.
A couple of focused tweaks would significantly reduce complexity without changing behavior.
1. executeMultiBackendWrite API – remove duplicated lambdas at call sites
Most usages pass two lambdas with identical logic, e.g.:
await this.executeMultiBackendWrite(
'markAsFailed',
async (backend) => backend.markAsFailed(id, true),
async (backend) => backend.markAsFailed(id, true),
);You can keep the “derived secondaries” capability but add a simpler overload where primary/additional behaviors are the same. That removes a lot of noisy boilerplate and makes intent clearer.
Concrete change:
private async executeMultiBackendWrite<T>(
operation: string,
primaryWrite: (backend: Backend) => Promise<T>,
additionalWrite?: (backend: Backend, primaryResult: T) => Promise<void>,
): Promise<T> {
const primaryResult = await primaryWrite(this.backend);
const replicationFn =
additionalWrite ??
(async (backend: Backend, result: T) => {
await primaryWrite(backend);
});
for (const additionalBackend of this.getAdditionalBackends()) {
const backendIdentifier = this.getBackendIdentifier(additionalBackend);
try {
await replicationFn(additionalBackend, primaryResult);
this.logger.info(`${operation} replicated to backend ${backendIdentifier}`);
} catch (replicationError) {
this.logger.error(
`Failed to replicate ${operation} to backend ${backendIdentifier}: ${replicationError}`,
);
}
}
return primaryResult;
}Then all symmetric cases collapse to:
await this.executeMultiBackendWrite(
'markAsFailed',
(backend) => backend.markAsFailed(notificationWithExecutionGitCommitSha.id, true),
);This preserves the ability to pass a custom additionalWrite when secondary behavior depends on the primary result (e.g. ID replication).
2. Backend identifier generation – make it stable and avoid recomputation
getBackendIdentifier currently depends on this.backends.size:
private getBackendIdentifier(backend: Backend): string {
if (typeof backend.getBackendIdentifier === 'function') {
return backend.getBackendIdentifier();
}
return `backend-${this.backends.size}`;
}This can produce different identifiers for the same instance if called before/after map mutations, and it’s also re-derived in multiple places (including logging), which makes reasoning about identifiers harder.
You can keep the current behavior but ensure identifiers are stable per backend instance by caching them:
// Add field
private backendIdentifiers = new WeakMap<Backend, string>();
private getBackendIdentifier(backend: Backend): string {
const cached = this.backendIdentifiers.get(backend);
if (cached) return cached;
const identifier =
typeof backend.getBackendIdentifier === 'function'
? backend.getBackendIdentifier()
: `backend-${this.backendIdentifiers.size}`;
this.backendIdentifiers.set(backend, identifier);
return identifier;
}That way:
- Each backend instance gets one stable identifier.
- Calls in
constructor,getAdditionalBackends, and logging all use the same value. - You can drop the constructor post-condition:
if (this.getAdditionalBackends().length !== additionalBackends.length) {
throw new Error('Invalid additional backends configuration');
}since backends will be a direct reflection of the backends you added, and getAdditionalBackends is a pure projection of that map.
This keeps all current functionality while reducing surprising behavior and coupling between methods.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add multi-backend support to the VintaSend notification service with primary-first write replication, backend-targeted reads, and synchronization utilities.
New Features:
Enhancements:
Documentation:
Tests: