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

DefaultEventSerializer - throw error when Type not found #4

Closed
StevenBlair123 opened this issue Apr 13, 2021 · 9 comments
Closed

DefaultEventSerializer - throw error when Type not found #4

StevenBlair123 opened this issue Apr 13, 2021 · 9 comments
Labels
enhancement New feature or request question Further information is requested
Projects

Comments

@StevenBlair123
Copy link

Having a play around with this framework and struggled to get the Subscription service working.
I pulled down your repository so I could debug it, and it turned out I hadn't registered my Type.
Opted for the DefaultEventSerializer, which silently handles unknown events.

Wonder if an option to throw an error on this would be useful?
Might be better that happen, than skip passed an event because someone forgot to register the Event.

@alexeyzimarev
Copy link
Contributor

Have you checked the serialization docs?

Types must be registered in the TypeMap as we don't want to depend on CLR type names when deserialising previously persisted events. I usually do it in Startup, but mappings themselves live in one place per domain model functional component.

For example, in the test project, all the mappings are registered in the static constructor of the events holder itself:

public static void MapBookingEvents() {
TypeMap.AddType<RoomBooked>("RoomBooked");
TypeMap.AddType<BookingPaymentRegistered>("BookingPaymentRegistered");
TypeMap.AddType<BookingCancelled>("BookingCancelled");
TypeMap.AddType<BookingImported>("BookingImported");
}

@alexeyzimarev alexeyzimarev added the question Further information is requested label Apr 23, 2021
@StevenBlair123
Copy link
Author

Alexey,

I had looked over that when I was working out what I had missed out.
It was more a suggestion to help diagnose a missing TypeMap entry

So something like:

DefaultEventSerializer defaultEventSerializer = new(true); //throw an exception if we don't find a Type

And with your example:

public static void MapBookingEvents() { 
     TypeMap.AddType<RoomBooked>("RoomBooked"); 
     TypeMap.AddType<BookingPaymentRegistered>("BookingPaymentRegistered"); 
     //TypeMap.AddType<BookingCancelled>("BookingCancelled"); //user forgets to add this.
     TypeMap.AddType<BookingImported>("BookingImported"); 
 }

@alexeyzimarev alexeyzimarev added this to Triage in Eventuous Apr 23, 2021
@alexeyzimarev
Copy link
Contributor

But how can it know what types are needed? It's just a serializer. Until you hit an event, which hasn't been mapped, it's impossible to know.

I believe it's something that's easily caught in tests.

@alexeyzimarev
Copy link
Contributor

A long time ago I was using a fallback to the CLR type name. However, it did more damage than good. When the serializer throws, you see it very fast. If it keeps storing events, which weren't mapped, you will be caught by surprise when you look at the store and see the wrong event type there, which is impossible to fix without migrating the streams.

@alexeyzimarev
Copy link
Contributor

I just got an idea. What if we can create a static analyser, which will trigger warnings in the editor/IDE if you use event types in When for the aggregate state, subscriptions, etc, but they aren't mapped anywhere. I am not sure if it's even possible, but maybe it's something to consider.

@StevenBlair123
Copy link
Author

Alexey,

Yes, I think that would be great if it was possible. Knowing about this at compile time would be great.
So somehow get the IDE to check your aggregate / projection and make sure we have a corresponding entry in TyepMap.

@alexeyzimarev
Copy link
Contributor

@StevenBlair123 have to park the code analysis idea facing some substantial work around subscriptions and diagnostics.

However, I got some ideas from other projects. For example, would an event type name attribute be a better solution? At least it will be obviously visible in the code.

@alexeyzimarev alexeyzimarev added the enhancement New feature or request label Aug 4, 2021
alexeyzimarev added a commit that referenced this issue Aug 5, 2021
@alexeyzimarev
Copy link
Contributor

Check the PR 👆

@alexeyzimarev
Copy link
Contributor

More checks added by #48

Eventuous automation moved this from Triage to Done Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
No open projects
Development

No branches or pull requests

2 participants