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.

Sign up for GitHub

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

support handling multiple events #363

Closed
wants to merge 7 commits into from
+359 −22
Closed

support handling multiple events #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
953f761
support handling multiple events
ido-namely May 24, 2021
3ccac7a
update comment
ido-namely May 24, 2021
daac94f
Merge branch 'master' into ido-multiple-events-per-command
ido-namely May 24, 2021
a1ba3c9
Merge branch 'master' into ido-multiple-events-per-command
Tochemey May 25, 2021
fce1933
Merge branch 'master' into ido-multiple-events-per-command
Tochemey Jun 1, 2021
4fc09f5
Merge branch 'master' into ido-multiple-events-per-command
Tochemey Jun 1, 2021
dad125c
Merge branch 'master' into ido-multiple-events-per-command
Tochemey Jun 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 35 additions & 20 deletions code/service/src/main/scala/com/namely/chiefofstate/AggregateRoot.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,40 +178,55 @@ object AggregateRoot {
protosValidator: ProtosValidator,
data: Map[String, com.google.protobuf.any.Any]): ReplyEffect[EventWrapper, StateWrapper] = {

val handlerOutput: Try[WriteHandlerHelpers.WriteTransitions] = commandHandler
.handleCommand(command, priorState)
.map(_.event match {
case Some(newEvent) =>
protosValidator.requireValidEvent(newEvent)
WriteHandlerHelpers.NewEvent(newEvent)

case None =>
WriteHandlerHelpers.NoOp
})
.flatMap({
case WriteHandlerHelpers.NewEvent(newEvent) =>
// state will be updated with the resulting state of each HandleEventResponse
var state = priorState.getState
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this line as it is an anti-pattern in scala.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what would you suggest to do instead? what exactly is the anti pattern?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're mutating the "state" as you're running your code without a real reason to do so. In functional programming, you typically want to pass around immutable objects.


val handlerOutput: Try[WriteHandlerHelpers.WriteTransitions] =
commandHandler
.handleCommand(command, priorState)
// use events or fallback to event in case events is empty
.map(response =>
response.events.isEmpty match {
case true =>
response.event match {
case Some(newEvent) => Seq(newEvent)
case None => Seq.empty[any.Any]
}
case _ => response.events
})
Comment on lines +189 to +196
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
response.events.isEmpty match {
case true =>
response.event match {
case Some(newEvent) => Seq(newEvent)
case None => Seq.empty[any.Any]
}
case _ => response.events
})
if (response.events.isEmpty) {
response.event match {
case Some(newEvent) => Seq(newEvent)
case None => Seq.empty[any.Any]
}
} else {
response.events
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Though I am kind of curious here, is i t possible for events and event to both be populated? If so, do we want to preserve both events and event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My preference is that we use events unless it is not populated.
I stated it in the proto change https://github.com/namely/chief-of-state-protos/pull/23/files

Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually fail if both are set.

.map(_.filter(x => x != any.Any()) // ignore empty events and validate types
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.map(_.filter(x => x != any.Any()) // ignore empty events and validate types
.map(_.filter(_ != any.Any.defaultInstance) // ignore empty Google Any

.map(newEvent => {
protosValidator.requireValidEvent(newEvent)
newEvent
}))
// call handleEvent for each event, while passing the updated resulting state from previous call
.map(_.map(newEvent => {
val newEventMeta: MetaData = MetaData()
.withRevisionNumber(priorState.getMeta.revisionNumber + 1)
.withRevisionDate(Instant.now().toTimestamp)
.withData(data)
.withEntityId(priorState.getMeta.entityId)
.withHeaders(command.persistedHeaders)

val priorStateAny: com.google.protobuf.any.Any = priorState.getState
val priorStateAny: com.google.protobuf.any.Any = state
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
val priorStateAny: com.google.protobuf.any.Any = state
val priorStateAny: com.google.protobuf.any.Any = priorState.getState


eventHandler
.handleEvent(newEvent, priorStateAny, newEventMeta)
.map(response => {
require(response.resultingState.isDefined, "event handler replied with empty state")
protosValidator.requireValidState(response.getResultingState)
WriteHandlerHelpers.NewState(newEvent, response.getResultingState, newEventMeta)
state = response.getResultingState
WriteHandlerHelpers.NewState(newEvent, state, newEventMeta)
Comment on lines +218 to +219
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
state = response.getResultingState
WriteHandlerHelpers.NewState(newEvent, state, newEventMeta)
WriteHandlerHelpers.NewState(newEvent, response.getResultingState, newEventMeta)

})

case x =>
Success(x)
})
.recoverWith(makeFailedStatusPf)

})
// extract WriteTransitions of each Try or fail, then get the last one (if any exists)
.map(_.get).lastOption)
Copy link
Contributor

@AntonioYuen AntonioYuen May 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
.map(_.get).lastOption)
.map(_.get).lastOption
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wasn't that supposed to get auto formatted or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe there's an auto format option for where parenthesis end. I just figure it would be much easier to understand this snippet since the ) has nothing to do with that line.

.map(x =>
x match {
case None => WriteHandlerHelpers.NoOp
case Some(newState) => newState
})
Comment on lines +224 to +228
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.map(x =>
x match {
case None => WriteHandlerHelpers.NoOp
case Some(newState) => newState
})
.map {
case None => WriteHandlerHelpers.NoOp
case Some(newState) => newState
}

Copy link
Contributor

Choose a reason for hiding this comment

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

put the parentheses tho

.recoverWith(makeFailedStatusPf)
handlerOutput match {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a line break above.

case Success(NoOp) =>
Effect.reply(replyTo)(CommandReply().withState(priorState))
Expand Down
Loading