cosmicpython / code

Example application code for the python architecture book
Other
2.14k stars 946 forks source link

Chapter 6, 7: Is it good to let UoW hold specified repo? #18

Open Bryant-Yang opened 4 years ago

Bryant-Yang commented 4 years ago

It seems that the service layer should play with repos(based on aggregation root), and use uow to deal with transactions, looks like: https://stackoverflow.com/questions/9808577/multiple-generic-repositories-in-unitofwork).

Let's say we have a use case that one customer in a biz organization need to confirm his/her purchase order, which will change the order status then the product supplier will send product to the customer with express, also record the total cost of the customer (may be by sending an event message to user management domain)

Suppose we have "customer" repo with models of "user", "organization", "customer" aggregated, and "purchase_order" repo with models of "sku", "customer", "supplier", or may be an "order" model with some value objects such as "shipping address" aggregated.

We do not need to create "customer uow", "purchase order uow" right? Should we just do:

class PurchaseOrderService(SomeBaseService):
    order_repo = PurchaseOrderRepository()
    @classmethod
    def OrderConfirm(customer: CustomerRepo, order_no: int):
        with SqlalchemyUnitOfWork as uow:
            order = purchase_order_repo.get_by_order_no(order_no)
                order.confirm_by_customer(customer)
            customer.add_total_cost(order.price * order.line_qty)
            uow.commit()
Bryant-Yang commented 4 years ago

well, as Martin Fowler said https://www.martinfowler.com/bliki/DDD_Aggregate.html: OBJECT Aggregates are the basic element of transfer of data storage - you request to load or save whole aggregates. Transactions should not cross aggregate boundaries.

So transaction of one aggregate(accessed by repo) should be operated in one uow session, it make sense to specify an aggregate root model to a uow do avoid crossing aggregate boundary. (as the fake code above, two aggregates order and customer are put in one uow session, it's not good if we are ok about the design of each aggregate.)

I'm just thinking about avoiding my own project from over design, from some declaretive db model classes to lots of model classes, repo classes and uow classes.

Any way it's a good start to think in ddd way.

hjwp commented 4 years ago

this is a good point actually. since you shouldn't be making changes to two different aggregates within the same unit of work, maybe it doesn't make sense to have multiple repositories available on the UoW class... i wonder what @bobthemighty thinks.

i suspect we mostly do it for convenience. having a different UoW class for each repo would feel overcomplicated, bearing in mind they would be the same in all other respects...

hjwp commented 4 years ago

incidentally, i'm curious about what you're putting in SomeBaseService?

bobthemighty commented 4 years ago

i suspect we mostly do it for convenience.

This is the answer. It's simple to get the repositories off the Unit Of Work, having started the transaction. I've worked on systems where the UOW and repositories were completely disconnected from a client perspective, but you still need to wire them up somehow, so it just means more magic.

We do not need to create "customer uow", "purchase order uow" right?

No, a unit of work is just a bunch of things that have to happen together. It's not specific to any one aggregate.

Should we just do

class PurchaseOrderService(SomeBaseService):
    order_repo = PurchaseOrderRepository()
    @classmethod
    def OrderConfirm(customer: CustomerRepo, order_no: int):
        with SqlalchemyUnitOfWork as uow:
            order = purchase_order_repo.get_by_order_no(order_no)
                order.confirm_by_customer(customer)
            customer.add_total_cost(order.price * order.line_qty)
            uow.commit()

Here we're modifying two aggregates though, right? You probably want something like

def confirm_order(orders: OrderRepository, order_no: int, confirmed_by: int):
    order = orders.get_by_order_no(order_no)
    order.confirm_by_customer(customer)    # Raises OrderConfirmed

def on_order_confirmed(customers: CustomerRepository, event: OrderConfirmed):
    customer = customers.get_by_id(event.confirmed_by)
    customer.add_total_cost(event.total_price)
Bryant-Yang commented 4 years ago

@hjwp

incidentally, i'm curious about what you're putting in SomeBaseService?

by now, it's just a simple protocol says "we need uow in our service":

class AbstractService(abc.ABC):
    @property
    @abc.abstractmethod
    def uow(self) -> AbstractUnitOfWork:
        raise NotImplementedError

I use explicit injection to let the uow to hold repo, there are 2 questions(Q1, Q2) in following details:

class UserService(AbstractService):
    uow: AbstractUnitOfWork 
    # user_repo: UserRepository  # should be initialized by the same session of uow
    # other_repo
    @classmethod
    def create_user(cls, user_name: str, full_name: str) -> UserModel:
        with cls.uow:
            user = cls.uow.repo.add(user_name=user_name, full_name=full_name)
            # Q1: Should we just let the UserService access UserRepository directly, instead of via uow? 
            # user = cls.user_repo.add(user_name=user_name, full_name=full_name)
            cls.uow.commit()
        return user

    @classmethod
    def get_user_info(cls, user_id: int) -> UserModel:
        return cls.uow.repo.get(user_id)

the test:

def test_create_user():
    # inject the repo to only one uow instead of creating each uow for each repo 
    UserService.uow = SqlAlchemyUnitOfWork(
        session=test_session_maker(),
        repo_cls=UserRepository
    )

    user = UserService.create_user(user_name=f'test_user_{randrange(1, 10000)}', full_name='Zelda')
    assert user == UserService.get_user_info(user_id=user.id)

    # Q2: Should uow make and close each session in every with-block?:
    # DetachedInstanceError will be raised, if we access the mapper model after sqlalchemy session closed (in uow.__exit__)
    # so I don't let uow to make each session and close it, but close in some other teardown.
    # eg. The flask-sqlalchemy shutdown session in @app.teardown_appcontext

I think a pure UoW need to hold and trace modified things, so if we don't have sqlalchemy session, it's necessary to put things we changed in to an UoW (like uow._added_repos, uow._updated_repos), but it's not necessary to let service layer access repository strictly via UoW. Another similar thread: https://softwareengineering.stackexchange.com/questions/406729/is-unit-of-work-pattern-really-needed-with-repository-pattern

I‘ll focus on the usage of uow about domain event.

Bryant-Yang commented 4 years ago

@bobthemighty

No, a unit of work is just a bunch of things that have to happen together. It's not specific to any one aggregate.

I incline to this, for me it's enough to see sqlalchemy session (commit, flush, rollback) as uow, about transaction.

Here we're modifying two aggregates though, right? You probably want something like

Yes, this point is clear with domain event

bobthemighty commented 4 years ago

Q1: Should we just let the UserService access UserRepository directly, instead of via uow?

If you like. It really doesn't matter. What's important is the principles: each piece has one job, our interesting logic lives in the domain model, and the domain model is kept pure of infrastructural concerns.

Q2: Should uow make and close each session in every with-block? DetachedInstanceError will be raised, if we access the mapper model after sqlalchemy session closed

Why would you need to use an object after you've finished the unit of work in which you loaded it? By definition, it's not in use any more. That aside, you can change this behaviour by using a different session maker. That's all the flask-alchemy library is doing IIRC, it's storing a session on the web request so that it can be used throughout the request lifecycle.

Bryant-Yang commented 4 years ago

@bobthemighty thanks, these won't be big problems, I take time to make basic conception more clear.