Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PG-172]: add 'simpler' aggregate trait #75

Merged
merged 5 commits into from
Dec 20, 2021

Conversation

emturner
Copy link
Contributor

@emturner emturner commented Dec 17, 2021

https://prima-assicurazioni-spa.myjetbrains.com/youtrack/issue/PG-172

closes #68

Renames Aggregate to AggregateManager and introduces a new, simplified Aggregate trait. The aim is to reduce the implementation complexity of aggregates for the majority of use-cases (ie pure aggregates).

There is a default implementation of AggregateManager for Aggregate - so that implementors of the Aggregate trait get the correct 'pipelining' behaviour for free.

demo tracking branch of platform-global

#69 is in progress - but was getting rather large so wanted to split this out first

potential future improvements:
- the Aggregate should not need an `event_store` method on it; could
break this out into another trait
- `handle_command` could return `Vec<Events>` to be saved transactionally
@emturner emturner changed the title [PG-172]: spike - ESRS aggregator API updates [PG-172]: add 'simpler' aggregate trait Dec 17, 2021
Copy link
Contributor

@cottinisimone cottinisimone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR and for the documentation you added

@@ -58,8 +55,8 @@ impl Aggregate for CreditCardAggregate {
}

// No state for credit_card aggregate
fn apply_event(_id: &Uuid, state: CreditCardState, event: &StoreEvent<Self::Event>) -> CreditCardState {
match event.payload() {
fn apply_event(state: CreditCardState, event: &Self::Event) -> CreditCardState {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the event wrapped inside the StoreEvent because I thought that maybe one of the inner field could have been useful for the state. Eg. assumptions over sequence_number or occurred_on. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it makes sense for projectors etc to take the StoreEvent - since any metadata currently lives there. (separately - I guess it could be worth thinking about how we handle metadata/tracing properly? (e.g. how pass through correlation_id through the system)

but it feels a bit odd to me for the core aggregate to have access to things like occured_on/sequence_number - depending on sequence_number in the state feels a bit flaky. And as with occured_on, if the aggregate needs a sequence or a timestamp then potentially they should be placed in the commands/events explicitly?

matteosister
matteosister previously approved these changes Dec 20, 2021
Copy link
Member

@matteosister matteosister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great!

src/esrs/aggregate.rs Outdated Show resolved Hide resolved
@emturner emturner force-pushed the emturner/simplified-aggregate-trait branch from 61a9116 to 6db8e19 Compare December 20, 2021 11:30
@cottinisimone cottinisimone merged commit 2fba578 into master Dec 20, 2021
@cottinisimone cottinisimone deleted the emturner/simplified-aggregate-trait branch December 20, 2021 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aggregate state is modified by do_handle_command
3 participants