zhuravlevma / typescript-ddd-architecture

Typescript DDD architecture for nest.js, subdomains, clean architecture, domain model, aggregates, event-driven ⚡
https://learning.oreilly.com/library/view/learning-domain-driven-design/9781098100124/
MIT License
94 stars 4 forks source link

Architecture suggestions #8

Open TbotaPhantA opened 8 months ago

TbotaPhantA commented 8 months ago

Hey, I like how you implemented ideas from Domain-Driven Design and Clean Architecture, I'm in the process of doing the same thing, but before I implement my own version, I'm studying how other people implement it in typescript. After studying your repository I can add a little bit of criticism and suggestions to your implementation, I hope it will be helpful for you.

  1. Your entities don't encapsulate data. All fields are public, therefore business logic can easily leak from the entity into other places and the object doesn't really control it's validity. I prefer not to use setters and getters, make all fields private and put as much business logic into entities and value objects as possible. If you need to export your data to the database or to your unit tests, you can use export method for that purpose which returns raw object with encapsulated data, but this method won't be used in the business logic.
  2. Outbox can be overloaded. You don't want waves of load for one aggregate to affect another aggregate's event sending, therefore I prefer to create separate outbox table for every aggregate. You can also use tools like debezium to send messages from the outbox table. It doesn't necessarily have to be a relay. But relay is also okay, there's nothing wrong with that.
  3. Outbox naming suggestions. Message which will be sent to outbox table can usually be either event or command. The tuple which is located in the outbox table is called Message. You also miss a little bit of data in the outbox table, therefore my preferred outbox naming would be:
    class <AggregateName>Message {
    messageId: number; // usually autoincrement, because it'll be easier to understand the order of messages
    messageType: MessageTypeEnum; // event or command
    messageName: string; // name of the event or command "WarehouseCreated"(event), "AddOrder"(command)
    aggregateId: string; // necessary for partitioning
    aggregateName: string; // Can be often useful (for example for humans or for id prefixes)
    contextName: string; // Can be often useful (for example for humans or for id prefixes)
    correlationId: string; // usually uuid or ulid, useful for log tracing and sagas.
    payload: Record<string, unknown>;
    // ...rest
    }
  4. In my opinion, the more complicated your domain is, the less CRUD-like it should be. Because CRUD doesn't tackle complexity very well. In complex domains code will be much cleaner with more meaningful business commands. For example UpdateProduct(price, quantity) should be replaced by terms from the domain like ReceiveProduct(which will increase quantity), ShipProduct(which will decrease quantity), AdjustInventory(which will change the quantity depending on inventorization). These meaningful command will result more meaningful events which are much more convenient to work with.

If I'll find something else I'll make sure to write about it.

zhuravlevma commented 8 months ago

Thank you so much for the feedback @TbotaPhantA! I really appreciate that you detailed your thoughts. It's important to clarify that my example doesn't quite qualify as a production solution. The primary goal was to demonstrate how to develop a system using DDD concepts.

I'm particularly intrigued by point 1 regarding private fields. Could you please share an example of an export that makes fields accessible for mapping?

Regarding the outbox pattern, I agree that it can be extended for each aggregate. I'll expand the set of fields and rename everything soon.

Concerning CRUD, I also agree. I might even implement something similar in the near future through https://cloud.google.com/apis/design/custom_methods.

If you have any more ideas, I'd be happy to hear them! Feel free to submit pull requests as well.

TbotaPhantA commented 8 months ago

I'll implement the example with private fields and send it to you later.

zhuravlevma commented 8 months ago

@TbotaPhantA Another question. Is correlationId a field for establishing a connection between different events? In our system, messageId has the type number, while correlationId has the type string. Or is it the ID of a request in correlationId?

TbotaPhantA commented 8 months ago

@TbotaPhantA Another question. Is correlationId a field for establishing a connection between different events? In our system, messageId has the type number, while correlationId has the type string. Or is it the ID of a request in correlationId?

CorrelationId is the trace id. We can track how certain request was flowing through the system. We can search logs by correlationId and it will show us all the logs which were caused by particular request. So correlationId is usually requestId. correlationId is also necesssary for implementing sagas, because sagas are basically asynchronous request/response. You send a command via broker with correlationId and you're waiting for a response to come via broker. The response must contain the correlationId.

messageId is the identificator of the message. Because relay guarantees at least once delivery it means that messages can potentially be duplicated and sent twice. These duplicated events will have the same messageId and it will be easier for consumers to recognize the duplicates and implement idempotent mechanisms.

zhuravlevma commented 8 months ago

@TbotaPhantA Thank you for the clarification! I'll be waiting for your implementation with private fields

TbotaPhantA commented 8 months ago

Here is an example of the product entity with encapsulated data. (repository is unfinished it will evolve)

https://github.com/TbotaPhantA/event-driven-loosely-coupled-monolith/blob/main/apps/backend/src/sales/domain/product/product.ts

It is an event sourcing style entity which tracks events for the entity(ProductCreated, PriceAdjusted, ProductInfoUpdated, ProductRemoved). Everything is private, no setters, no getters. It's fully covered with unit tests.

I implemented it today and I'm in the little bit of a hurry, so feel free to refactor it. If you'll come up with a better way to organize it, let me know.

As a side note. I didn't decide yet, whether it's good or bad to store the entire before and after state of the entity in the event. Heard different opinions, but I added before and after states as an example

TbotaPhantA commented 8 months ago

Here is the service which uses this entity https://github.com/TbotaPhantA/event-driven-loosely-coupled-monolith/blob/main/apps/backend/src/sales/application/product/services/adjustPrice.service.ts

Here are the unit tests: https://github.com/TbotaPhantA/event-driven-loosely-coupled-monolith/tree/main/apps/backend/test/unit/sales/domain/product

zhuravlevma commented 8 months ago

@TbotaPhantA, thank you so much for the example. I took your implementation of the export() and applied it in the accounting context (reports subdomain - https://github.com/zhuravlevma/typescript-ddd-architecture/tree/main/src/accounting/reports/report). I moved export() to abstract classes to avoid unnecessary logic in the domain layer and also changed the messages, more clearly separating them into events and commands. I also moved all of this behind abstract classes.

What do you think of the new implementation? Do you see any issues anywhere?

TbotaPhantA commented 8 months ago
  1. You don't need to put correlationId into the event. I retreive it from async context storage when I insert event into the outbox. https://github.com/TbotaPhantA/event-driven-loosely-coupled-monolith/blob/main/apps/backend/src/infrastructure/messages/services/salesProductMessages.service.ts and the event itself is here https://github.com/TbotaPhantA/event-driven-loosely-coupled-monolith/blob/main/apps/backend/src/sales/domain/product/events/priceAdjusted.ts .
  2. apply method represents state changes. Make my apply method public and try to implement projection from event sourcing. Or here is the video explaining what apply method is and what projection is https://youtu.be/AUj4M-st3ic?list=PLThyvG1mlMzkRKJnhzvxtSAbY8oxENLUQ. All the business logic should be inside adjustPrice, updateProductInfo... methods, because these methods are command handlers, business logic should usually be in these command handlers, not in the apply method. Look at my apply methods. They're dead simple. Business invariants should also be in the command handlers.
  3. naming: apply<EventName>. If you have event called ReportValidated you should name apply method applyReportValidated
zhuravlevma commented 8 months ago

@TbotaPhantA

  1. But I don't have event sourcing. It's currently a choreography, where relay service publishes an event (in some Kafka, for example), and then all consumers read it in the controller. So, each controller implements this apply logic (just need to name it apply) for its respective topic. https://github.com/zhuravlevma/typescript-ddd-architecture/blob/main/src/delivery/curiers/curier/deliveryman.controller.ts#L53
TbotaPhantA commented 8 months ago

I use the same style of event tracking even when I don't use event sourcing, It's probably more of a matter of taste.