serverlesstechnology / cqrs

A lightweight, opinionated CQRS and event sourcing framework.
Other
370 stars 40 forks source link

Control over view_id when creating a view #94

Closed carlos-verdes closed 3 months ago

carlos-verdes commented 5 months ago

Related with #90

When we use CQRS the main idea is to segregate commands and queries right? On this crate we have control over the aggregate_id when we are managing commands, however when we build a view the view_id is populated always with the aggregate_id of an specific aggregate.

This second part for me breaks the main advantage of CQRS, first of all I should be able to use events from 2 different aggregates and secondly (as the problem on issue #90) in many cases the query result (let's call it view record) will not match 1:1 with an aggregate nor have the same id.

image You can see in the picture how the Query and Command model are not 1:1 (and they should not)

Example where aggregate id is different from view id:

The Portfolio aggregate commands will use a generated id for identity and will add the user_id as part of the metadata. The view will use user_id as a key, and in every PortfolioCreated event will decide if it updates the view or not.

Example with different aggregates:

This case can be managed if we consider PortfolioUsdValue as an aggregate, but the reality is just a view (we will never execute a command from users). We need to be able on this scenario to receive events from Portfolio and Market aggregates.

What I found

When we submit a command we are able to provide 3 parameters:

However when we write a query we can only update the payload of the view record.

When I see the implementation of execute_with_metadata I see why the aggregate_id becomes the view_id (this is only a specific case but not the general expected behavior of a query entity)

pub async fn execute_with_metadata(
        &self,
        aggregate_id: &str,
        command: A::Command,
        metadata: HashMap<String, String>,
    ) -> Result<(), AggregateError<A::Error>> {
       ...
        for processor in &self.queries {
            let dispatch_events = committed_events.as_slice();
            processor.dispatch(aggregate_id, dispatch_events).await;
        }
        Ok(())
    }

Then I see the trait Query has couple of things I'm not fully aligned:

Current implementation:

#[async_trait]
pub trait Query<A: Aggregate>: Send + Sync {
    /// Events will be dispatched here immediately after being committed.
    async fn 
    dispatch(&self, aggregate_id: &str, events: &[EventEnvelope<A>]);
}

Proposal

So maybe we could change this into something like:

#[async_trait]
pub trait Query<A: ViewRecord>: Send + Sync {
    /// no need for aggregate_id parameter
    async fn dispatch(&self, events: &[EventEnvelope<A>]);
}

#[async_trait]
pub trait ViewRecord: Default + Serialize + DeserializeOwned + Sync + Send {

    fn view_record_id() -> String;
    fn apply(&mut self, event: Self::Event);
}

And to not disrupt current implementation we can provide From implementations like:

impl From<Aggregate> for ViewRecord ...

Share your thoughts I may be missing something.

davegarred commented 3 months ago

when we build a view the view_id is populated always with the aggregate_id of an specific aggregate

The GenericQuery reuses the aggregate_id as the view_id, but this is certainly not the only way to do it. It is a common way to handle it and handles many use cases, which is why it is included here.

Note that the dispatch method of Query receives the entire event envelope. For cases where you would you do not want the aggregate_id linked with the view_id, you should build a custom query that looks deeper into the event to find the relevant details for that query.

first of all I should be able to use events from 2 different aggregates

Absolutely, but this is a case where you would want a custom query to handle this logic.

The GenericQuery is useful for simple cases but for anything more complex it should be used as a reference in building a custom query. This may be a documentation failure on my part in making this point clear.

carlos-verdes commented 3 months ago

Thanks for your answer, but I think the main issue is that both the Query and the CQRS framework only accept one Aggregate type, so I still can't implement a Query which accepts events from two or more aggregates.

For the Query implementation I would lax the definition and instead of asking for the Aggregate type I would just ask for an Event type, then you can do things like:


use aggregate1::Aggregate1Event;
use aggregate2::Aggregate2Event;

enum QueryEvents {
    Aggregate1Event(Aggregate1Event),
    Aggregate2Event(Aggregate2Event),
}

impl DomainEvent for QueryEvent {
    fn event_type(self) -> String {
        match self {
            QueryEvent::Aggregate1Event(event) => event.event_type(),
            QueryEvent::Aggregate2Event(event) => event.event_type(),
        }
    }
}

impl From<Aggregate1Event> for QueryEvents {
    fn from(event: Aggregate1Event) -> Self {
        QueryEvent::Aggregate1Event(event)
    }
}

impl From<Aggregate2Event> for QueryEvents {
    fn from(event: Aggregate2Event) -> Self {
        QueryEvent::Aggregate2Event(event)
    }
}

For the CQRS framework I would add a new abstraction called BoundedContext where you can define all Aggregate types that are part of that bounded context, this way you can manage all commands and events related with the bounded context using a single framework.

davegarred commented 3 months ago

I think the main issue is that both the Query and the CQRS framework only accept one Aggregate type, so I still can't implement a Query which accepts events from two or more aggregates.

I see your point now. With two aggregates you certainly need two different Query implementations even though it they have the same backing storage and view projection. It would certainly make sense to reduce some of this overhead.

I also like the idea of a bounded context tying multiple aggregates together, that has been a point of confusion.

@carlos-verdes would you mind opening a new issue with your thoughts on these changes?

carlos-verdes commented 1 month ago

Created issues #96 and #97