fix(api): fix transaction scope in EventsSubscription #360 #364

hkawalek · 2021-09-18T08:50:00Z

Closes #360

…n_scope_in_EventsSubscription

Add PoC PrismaTransactionManager

vercel · 2021-09-18T08:50:03Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

coderscamp-website – ./

🔍 Inspect: https://vercel.com/coderscamp/coderscamp-website/JBno7CAxEpDkdwusPyDPPPXuDR4h
✅ Preview: https://coderscamp-website-git-issue-360-eventssubscr-04d698-coderscamp.vercel.app

coderscamp-storybook – ./

🔍 Inspect: https://vercel.com/coderscamp/coderscamp-storybook/2PHPBdoxKg4rZ58thbHYxfRrtC3B
✅ Preview: https://coderscamp-storybook-git-issue-360-eventssubs-c535aa-coderscamp.vercel.app

coderscamp-docs – ./

🔍 Inspect: https://vercel.com/coderscamp/coderscamp-docs/3vaQSfjrbeAzKjZpnvmsjzrXt1Xv
✅ Preview: https://coderscamp-docs-git-issue-360-eventssubscript-9911d3-coderscamp.vercel.app

codecov-commenter · 2021-09-18T10:00:32Z

Codecov Report

Merging #364 (a367956) into main (23b379d) will decrease coverage by 0.67%.
The diff coverage is 83.10%.

@@            Coverage Diff             @@
##             main     #364      +/-   ##
==========================================
- Coverage   86.81%   86.13%   -0.68%     
==========================================
  Files         125      128       +3     
  Lines        1456     1558     +102     
  Branches      191      207      +16     
==========================================
+ Hits         1264     1342      +78     
- Misses        192      216      +24

Flag	Coverage Δ
api	`83.51% <83.10%> (-0.61%)`	⬇️
ui	`98.87% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...terials-url-was-generated-event-handler.service.ts	`40.00% <0.00%> (ø)`
...-email-confirmation-application-command-handler.ts	`66.66% <ø> (ø)`
...-email-confirmation-application-command-handler.ts	`100.00% <ø> (ø)`
...le/write/shared/application/application-service.ts	`100.00% <ø> (ø)`
...odule/write/shared/application/event-repository.ts	`100.00% <ø> (ø)`
...ation/application/register-user.command-handler.ts	`100.00% <ø> (ø)`
...-repository/prisma-transaction-event-repository.ts	`21.42% <21.42%> (ø)`
...e/application-service/event-application-service.ts	`82.50% <68.18%> (-17.50%)`	⬇️
...-transaction-manager/prisma-transaction-manager.ts	`84.61% <84.61%> (ø)`
...ion/user-registration-was-started.event-handler.ts	`100.00% <100.00%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23b379d...a367956. Read the comment docs.

MateuszNaKodach

Whereas there are very smart solutions, and I learned something (like injecting transaction context - how would you do something like that in C# (reflection / aspects)? Looks like a JS hack.

I think I've made a design mistake before, and it's dangerous to follow this approach.
Expansion of transaction context is probably not a way to go.
With that, our solution is not scalable and slices logic is coupled with another slices.

So, I propose to make something easier and more scalable.
Like in every messaging system, we need to handle message duplications instead of introducing transactional consistency.
Let's assume for a while, that source for subscriptions is not the database where we're storing progress, but some message broker etc. In this case, it's impossible to have transaction with covers also the side effect.
So I propose to leave subscriptions without transaction (add it only to EventStore) and accept possible duplicates.
What do you think?
We can try to mitigate duplicates with some buffer of processed commands?.

MateuszNaKodach · 2021-09-20T08:23:31Z

...src/module/write/shared/application/prisma-transaction-manager/prisma-transaction-manager.ts

+
+  private internalState: PrismaTransactionManagerState = 'created';
+
+  public get state() {


Is it necessary? I don't see any usages of that getter.

MateuszNaKodach · 2021-09-20T08:27:25Z

...src/module/write/shared/application/prisma-transaction-manager/prisma-transaction-manager.ts

+    }
+  }
+
+  private injectContextIntoCommand(command: ApplicationCommand) {


Looks like hacks simple to introduce only in JS :D

MateuszNaKodach · 2021-09-20T08:28:11Z

...src/module/write/shared/application/prisma-transaction-manager/prisma-transaction-manager.ts

+  }
+
+  executeCommand<
+    TApplicationCommand extends ApplicationCommand<string, Record<string, unknown>, DefaultCommandMetadata>,


You can remove those generic parameters - there are just default values.

MateuszNaKodach · 2021-09-20T08:32:55Z

...src/module/write/shared/application/prisma-transaction-manager/prisma-transaction-manager.ts

+
+type PrismaTransactionManagerState = 'created' | 'executing' | 'executed';
+
+class PrismaTransactionManager implements PrismaTransactionContext {


I'm wondering how is it better (beside injecting context into command) than interactive transactions from Prisma preview?
Is it not the same implemented differently, and only we need to maintain it on our own?

MateuszNaKodach · 2021-09-20T08:34:13Z

.../api/src/module/write/shared/infrastructure/application-service/event-application-service.ts

+
+    const eventsToPublishIds = this.transactionEventRepository.write(streamName, eventsToStore, streamVersion, context);
+
+    return context.executeAfterTransaction(async (prisma) => {


Do we need callback here? What about await for transaction?

MateuszNaKodach · 2021-09-20T08:35:13Z

.../api/src/module/write/shared/infrastructure/application-service/event-application-service.ts

+    const eventsToPublishIds = this.transactionEventRepository.write(streamName, eventsToStore, streamVersion, context);
+
+    return context.executeAfterTransaction(async (prisma) => {
+      const eventsToPublish = await this.transactionEventRepository.readAll(eventsToPublishIds, prisma);


As far as I understand, we are after transaction here, so we dont need readAll from transactionEventRepository. Am I reight?

hkawalek · 2021-09-21T19:51:13Z

Whereas there are very smart solutions, and I learned something (like injecting transaction context - how would you do something like that in C# (reflection / aspects)? Looks like a JS hack.

In any OOP language, you can use a decorator pattern or inheritance e.q.

class ApplicationCommandWithContext extends ApplicationCommand {
  constructor(readonly context: PrismaTransactionContext, ...restOfParams) {
    super(...restOfParams);
  }
}

// and then
if (command instanceof ApplicationCommandWithContext) {
}

I use JS hack because it's simpler and faster to code.

I think I've made a design mistake before, and it's dangerous to follow this approach.
Expansion of transaction context is probably not a way to go.
With that, our solution is not scalable and slices logic is coupled with another slices.

So, I propose to make something easier and more scalable.
Like in every messaging system, we need to handle message duplications instead of introducing transactional consistency.
Let's assume for a while, that source for subscriptions is not the database where we're storing progress, but some message broker etc. In this case, it's impossible to have transaction with covers also the side effect.
So I propose to leave subscriptions without transaction (add it only to EventStore) and accept possible duplicates.
What do you think?

I see some pros and cons of using the current implementation of transactional consistency:

pros:
- it's the fastest possible implementation because it builds the whole transaction query and executes at once (there
  is no long-living active transaction)
- we've read model consistency for free
cons:
- it's buggy prone. Only code review can ensure that it's correctly used by other developers
- it can't be abstracted. We're bounded to PrismaPromise
- there is still need to manualy handle some errors (e.g. when executeAfterTransaction fails, external resources)

I agree with you that we should abandon transactional consistency. In the end, it's harder to ensures and code.

We can try to mitigate duplicates with some buffer of processed commands?.

As I understand, we have 3 cases when onEvent is invoked:

to update read model
to perform side-effect on external resources (e.g. learning-materials-url)
to send command

I think that we can mitigate duplicates 'globally' in the 1st and 3rd cases:

We can add the version field to each read-model. The version points to eventId or globalOrder and represents
the current state of read-model up to this event.
We can somehow use stored metadata (e.g. causation_id) to filter out duplicates.

MateuszNaKodach · 2021-09-22T08:48:41Z

Created issue for further development:
#371

hkawalek added 3 commits September 16, 2021 10:27

Add pgadmin as a tool to docker-compose

2632374

Merge branch 'main' into issue-360-_EventsSubscription_Fix_transactio…

31140d1

…n_scope_in_EventsSubscription

fix(api): Fix transaction scope in EventsSubscription

c488f40

Add PoC PrismaTransactionManager

hkawalek requested a review from MateuszNaKodach as a code owner September 18, 2021 08:50

KonradSzwarc temporarily deployed to coderscamp-issue-360--e-elc2aa September 18, 2021 08:50 Inactive

fix test

a367956

vercel bot deployed to Preview – coderscamp-docs September 18, 2021 09:57 View deployment

vercel bot deployed to Preview – coderscamp-website September 18, 2021 09:57 View deployment

vercel bot deployed to Preview – coderscamp-storybook September 18, 2021 09:57 View deployment

KonradSzwarc temporarily deployed to coderscamp-issue-360--e-elc2aa September 18, 2021 09:57 Inactive

MateuszNaKodach reviewed Sep 20, 2021

View reviewed changes

MateuszNaKodach mentioned this pull request Sep 22, 2021

[EventsSubscription] Deduplication for consumers #371

Open

MateuszNaKodach closed this Sep 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api): fix transaction scope in EventsSubscription #360 #364

fix(api): fix transaction scope in EventsSubscription #360 #364

hkawalek commented Sep 18, 2021

vercel bot commented Sep 18, 2021 •

edited

Loading

codecov-commenter commented Sep 18, 2021 •

edited

Loading

MateuszNaKodach left a comment •

edited

Loading

MateuszNaKodach Sep 20, 2021

MateuszNaKodach Sep 20, 2021

MateuszNaKodach Sep 20, 2021

MateuszNaKodach Sep 20, 2021

MateuszNaKodach Sep 20, 2021

MateuszNaKodach Sep 20, 2021

hkawalek commented Sep 21, 2021

MateuszNaKodach commented Sep 22, 2021


		private internalState: PrismaTransactionManagerState = 'created';

		public get state() {


		type PrismaTransactionManagerState = 'created' \| 'executing' \| 'executed';

		class PrismaTransactionManager implements PrismaTransactionContext {


		const eventsToPublishIds = this.transactionEventRepository.write(streamName, eventsToStore, streamVersion, context);

		return context.executeAfterTransaction(async (prisma) => {

fix(api): fix transaction scope in EventsSubscription #360 #364

fix(api): fix transaction scope in EventsSubscription #360 #364

Conversation

hkawalek commented Sep 18, 2021

vercel bot commented Sep 18, 2021 • edited Loading

coderscamp-website – ./

coderscamp-storybook – ./

coderscamp-docs – ./

codecov-commenter commented Sep 18, 2021 • edited Loading

Codecov Report

MateuszNaKodach left a comment • edited Loading

Choose a reason for hiding this comment

MateuszNaKodach Sep 20, 2021

Choose a reason for hiding this comment

MateuszNaKodach Sep 20, 2021

Choose a reason for hiding this comment

MateuszNaKodach Sep 20, 2021

Choose a reason for hiding this comment

MateuszNaKodach Sep 20, 2021

Choose a reason for hiding this comment

MateuszNaKodach Sep 20, 2021

Choose a reason for hiding this comment

MateuszNaKodach Sep 20, 2021

Choose a reason for hiding this comment

hkawalek commented Sep 21, 2021

MateuszNaKodach commented Sep 22, 2021

vercel bot commented Sep 18, 2021 •

edited

Loading

codecov-commenter commented Sep 18, 2021 •

edited

Loading

MateuszNaKodach left a comment •

edited

Loading