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 projections faster by only reading Events that have handlers #569

Closed

Conversation

Gordin
Copy link

@Gordin Gordin commented Apr 6, 2019

No description provided.

@mpraglowski
Copy link
Member

I have mixed feelings about that PR. It looks like a nice improvement but how it will work if someone uses the event type remapping feature we have implemented in DefaultMapper?
The remapping of event type (event's class) is performed while reading from event repository by mapper. I have no problem when someone explicte uses the of_type method because it's his/hers responsibility to provide correct types... but here it will be easy to miss the use case when some events won't be ready due to remapping feature.
I.e. class A given in when method, but it's stored as X in event_type and is only remapped to A while reading from event repository. Now it will work correctly because X will be remapped to A before it is processed by projection. With filtering by event type events of type X won't be read and won't be processed.

@Gordin
Copy link
Author

Gordin commented Apr 17, 2019

I didn't know about the remapping feature, I'll look into it ^^. Maybe I can just also include the events that will be mapped.

I was gonna ask if you have any idea why the mutation testing for this fails fails in the EncryptionMapper, but I'm guessing it could have something to do with the remapping as well. I'll try to fix this as well.

@mostlyobvious
Copy link
Member

I was gonna ask if you have any idea why the mutation testing for this fails fails in the EncryptionMapper

Sorry for the trouble with EncryptionMapper, the reason for this is #567

@mostlyobvious
Copy link
Member

@Gordin that issue with EncryptionMapper should be gone if you rebase with master

@mostlyobvious
Copy link
Member

Thank you! I've rebased your branch in #709 and this is now merged.

Remapping is yet to be done, but we did not support it in Projection previously either (#710).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants