envato / event_sourcery

A library for building event sourced applications in Ruby
MIT License
84 stars 10 forks source link

Support event schema validation #174

Closed macosgrove closed 7 years ago

macosgrove commented 7 years ago

My team is working on an Event Sourcery app. We want to introduce event schema validation because:

Other Envato apps have enforced validation of their event schemas by introducing parallel methods to apply_event and emit_event named validate_and_apply_event and validate_and_emit_event. I believe it would be better for schema validation to be directly supported by Event Sourcery rather than for each Event Sourcery app to implement it independently.

This PR adds that support. Note that event schema validation is entirely optional for apps; I have introduced a trivial #valid? method on EventSourcery::Event to ensure this change is backwards compatible.

Our app uses dry-validation for defining and enforcing the schemas, but that particular mechanism is not enforced here. It is only necessary for events to understand #valid? and provide a hash via #validation_errors

An additional change is required for event_sourcery-postgres. They should be released together.

mjward commented 7 years ago

You are spot on @macosgrove. This has been the direction for awhile now. I had intentions of pushing this in a month or so ago, but went on parental leave, so many thanks for the effort of making the PR!

Last I spoke with @stevehodgkiss about this he had some ideas around Events that I thought may be conflicting to my vision moving forward. We never did get a chance to circle back to have that discussion so I will be interested to hear whether this aligns with how @stevehodgkiss sees things as well.

jiexinhuang commented 7 years ago

Personally, I feel validation rules could be separated from persistence layer? In that case, https://github.com/envato/event_sourcery-postgres/pull/36 may not be required if validation rules are injected at the right spot?

macosgrove commented 7 years ago

@jiexinhuang @mjward Your suggestions are the exact opposite of each other. Do we need to get some people together to discuss this?

macosgrove commented 7 years ago

@jiexinhuang I know what you mean though. I felt really dirty having to update the Postgres gem to get the reactor to validate when it emits an event. It seems very strange to me that there is no Reactor class in Event Sourcery. It exists only in the Postgres gem.

Now I find myself wondering how (or if) it's possible to build an app with an in-memory event store if there is no Reactor class for it. @leonm have you encountered that in your in-memory explorations?

macosgrove commented 7 years ago

It also seems strange to me that sink is defined for the in memory EventStore in EventSourcery but for the Postgres EventStore in the Postgres gem. I wonder if we should have an abstract EventStore class in EventSourcery with a template method for sink that delegates to an underlying store (either in memory or Postgres).

stevehodgkiss commented 7 years ago

@macosgrove I agree the Reactor module should be a concept in EventSourcery not just the Postgres::Reactor. Also, +1 to the idea of extracting adapters for the underlying EventStore storage logic. A lot of code is repeated with the current EventStore setup.

jiexinhuang commented 7 years ago

I think validate on Event.new is reasonable. It means we don't have to inject validation check to both aggregate and reactor. It also allows us to test events without having a event store.(Like projecting in memory events)

grassdog commented 7 years ago

Not sure I understand @jiexinhuang. I would have thought aggregates would validate input parameters etc... passed to them and its their job to construct a valid event which is sinked to the store.

Reactors should only receive events after they've been successfully persisted so I'm not sure why they would need to validate the events being passed to them again?

Are you saying reactors and aggregates would validate the events they've constructed before passing them off to be sunk?

leonm commented 7 years ago

@macosgrove I haven't gotten as far as a Reactor in the InMemory store. I've done commands and projectors. Reactors to come soon :-)

jiexinhuang commented 7 years ago

@grassdog As processors and aggregates are loosely coupled, one issue we have had in RSS is a processor can make false assumptions of attributes of events it subscribes to. We are validating inputs on command side, but it doesn't solve the problem. So we could end up with passing unit tests for all processors but failed integration test if we have good coverage in there, or production incidents if the mismatch is not covered in integration tests.

macosgrove commented 7 years ago

@jiexinhuang We can't validate the event on instantiation anyway, as our Reactor.emit_event method currently instantiates the event before its content is finalised. In here, action.call is often used to modify the event body.

grassdog commented 7 years ago

@jiexinhuang Constructor initialisation aside. I think it's important to tease apart what we mean by event validation here. I see it as ensuring we only store (and therefore read) events that have an expected shape.

If you need validation of inputs to an aggregate or reactor layer I'm not sure this would be the mechanism for that. Happy to discuss further and be convinced otherwise 😄 Perhaps at the next brownbag.

mjward commented 7 years ago

I agree @macosgrove - glad we are taking the time to think about this instead of rushing something in and really appreciate your efforts in championing it. I think its a great candidate for brownbag. Lets clear the agenda so we can discuss.

There seems to be multiple things at play here. I'll try and hit some of you up individually over the coming days to help ensure I have a solid grasp of what all these concerns are so we can table it with some clarity for those that may be coming at this cold at the brownbag.

macosgrove commented 7 years ago

Closed in favour of validation on sink, in a separate gem.