search

RailsEventStore / rails_event_store

A Ruby implementation of an Event Store based on Active Record
http://railseventstore.org
MIT License
1.41k stars 121 forks source link

Fix snapshot_repository aggregate state loading from snapshot #1706

Closed DeVeLo closed 7 months ago

DeVeLo commented 9 months ago

Marshal.load creates new object with brand new object id. Current implementation breakes events publishing after first snapshot is created.

pjurewicz commented 9 months ago

Hello @DeVeLo, thank you for your contribution! Can you document with a simple test the issue this PR is resolving?

DeVeLo commented 9 months ago

Hello @pjurewicz, when I find a free moment. Meanwhile, I've found another minor error, which I will post here for correction when I have time. It is that if we emit 2 consecutive events in the aggregator (as part of handling one command), the next event will end with a 500 and a version mismatch error.

pjurewicz commented 9 months ago

I'm also curious under what circumstances the JSON is malformed. Can you provide an example? We cannot introduce base64 encoding just like that as it is a breaking change. Our snapshot repository is still an experimental feature, and we value all feedback that we receive.

DeVeLo commented 9 months ago

As it was said, snapshot implementation is an experimental feature and in my opinion is unusable in current form. Of course, maybe in some specific environment it works just fine. But, in my case, when I tried to emit snapshot it throws an exception "illegal/malformed utf-8". Which is a result of marshaling an object. Maybe this is related to db engine, I use PostgreSQL 15.

pjurewicz commented 8 months ago

Right, marshal dumps would not work with JSON events serializer without introducing encoding. However, you can have any other configured that accepts ASCII-8 encoded strings

DeVeLo commented 8 months ago

Anyway, the solution does not work out of the box for PostgreSQL and jsonb. Any constructive conclusions regarding this PR?

pjurewicz commented 8 months ago

We won't introduce base64 encoding as a scope of this PR. We need a migration strategy for other users that is not covered by this PR. According to the first commit, I miss the test helping understand what this change fixes and preventing regression.