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

Make external event payloads strongly typed #907

Merged
merged 11 commits into from
Oct 14, 2024

Conversation

tnotheis
Copy link
Member

@tnotheis tnotheis commented Oct 11, 2024

Readiness checklist

  • I added/updated unit tests.
  • I added/updated integration tests.
  • I ensured that the PR title is good enough for the changelog.
  • I labeled the PR.

So far the payloads of the external events have been anonymous objects. This isn't ideal, so this PR introduces a subclass of ExternalEvent for each external event type.

@tnotheis tnotheis added the refactoring Refactoring of code label Oct 11, 2024
@tnotheis tnotheis self-assigned this Oct 11, 2024
@tnotheis tnotheis requested review from a user and HunorTotBagi October 14, 2024 07:04
Copy link
Member

@HunorTotBagi HunorTotBagi left a comment

Choose a reason for hiding this comment

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

I noticed that in DomainEvents.Incoming most of the files we have a Handle method and a CreateExternalEvents but in

  • IdentityDeletionProcessStartedDomainEventHandler
  • IdentityDeletionProcessStatusChangedDomainEventHandler
  • RelationshipStatusChangedDomainEventHandler

we don't follow this "pattern". So for consistency sake I would refactor the above three mentioned files to have a Handle and a CreateExternalEvents method. What do you think about this?

@tnotheis tnotheis marked this pull request as ready for review October 14, 2024 08:48
@tnotheis tnotheis requested a review from HunorTotBagi October 14, 2024 08:57
@HunorTotBagi
Copy link
Member

I noticed that in DomainEvents.Incoming most of the files we have a Handle method and a CreateExternalEvents but in

  • IdentityDeletionProcessStartedDomainEventHandler
  • IdentityDeletionProcessStatusChangedDomainEventHandler
  • RelationshipStatusChangedDomainEventHandler

we don't follow this "pattern". So for consistency sake I would refactor the above three mentioned files to have a Handle and a CreateExternalEvents method. What do you think about this?

@tnotheis Thoughts on this? It seems you missed this one.

ghost
ghost previously approved these changes Oct 14, 2024
@tnotheis
Copy link
Member Author

@HunorTotBagi Sorry, you're right. I didn't see that.

I just fixed it. I also renamed the methods so they contain the name of the event that is created. So if there is another external event created by the same domain event handler, we would do this in another private method.

Further, I moved the try/catch block to the Handle method, so that we don't have to catch exceptions in all of the private methods.

Btw: I think we could even remove the try/catch blocks, because the EventBus classes already log descriptive error messages. But I will not remove them in this PR.

@HunorTotBagi
Copy link
Member

@HunorTotBagi Sorry, you're right. I didn't see that.

I just fixed it. I also renamed the methods so they contain the name of the event that is created. So if there is another external event created by the same domain event handler, we would do this in another private method.

Further, I moved the try/catch block to the Handle method, so that we don't have to catch exceptions in all of the private methods.

Btw: I think we could even remove the try/catch blocks, because the EventBus classes already log descriptive error messages. But I will not remove them in this PR.

Thanks, I like the changes.

I also noticed that the naming of the parameters in the Handle and in the private methods are all different for example we can see domainEvent, @event and integrationEvent. We should choose in what method which naming to use and apply all over them.

@tnotheis
Copy link
Member Author

I also noticed that the naming of the parameters in the Handle and in the private methods are all different for example we can see domainEvent, @event and integrationEvent. We should choose in what method which naming to use and apply all over them.

Done

@tnotheis tnotheis merged commit 6ed871a into main Oct 14, 2024
24 checks passed
@tnotheis tnotheis deleted the make-external-event-payloads-strongly-typed branch October 14, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants