Reshaped MongoDBEventStore initialisation #136

oskardudycz · 2024-12-06T13:24:00Z

1. Reshaped MongoDBEventStore initialisation to allow passing client or connection string.

That should make it more accessible.

Internally, it'll ensure that the client is connected and allow closing the client through the exposed close method if it was created by the event store.

Also hidden is the class and exposed type to replace MongoDBEventStore with in-memory implementation for integration tests.

Passing clients will enable more advanced features further on, like sessions, transactions, etc.

Having collection per stream type will also make easier indexing of inline projections and other optimisations.

2. Renamed events to messages

In the longer term, we want to store also commands sometimes, not only events. This change should stabilise the payload.

Reshaped MongoDBEventStore initialisation to allow passing client or connection string. That should make it more accessible. Internally it'll ensure that client is connected, and allow closing client through exposed close method if it was created by the event store. Hidden also class and exposed type to allow replacing MongoDBEventStore by inMemory implementation for integration tests. Passing client will enable more advanced features further on like sessions, transactions, etc. Having collection per stream type will also make easier indexing of inline projections and other optimisations.

oskardudycz · 2024-12-06T13:27:20Z

@alex-laycalvert, could you have a look? 🙏 🙂

In the longer term, we want to store also commands sometimes, not only events. This change should stabilise the payload.

alex-laycalvert · 2024-12-06T22:56:36Z

src/packages/emmett-mongodb/src/eventStore/mongoDBEventStore.ts

@@ -246,13 +284,13 @@ export class MongoDBEventStore implements EventStore<MongoDBReadEventMetadata> {
        readModels: stream?.projections ?? {},
        events: eventsToAppend,
        projections: this.inlineProjections,
-        collection: this.collection,
+        collection,
        updates,
        client: {},


I didn't see this on the updates to projections, but for inline projections a client isn't necessary right? Or is it just because the context for inline projections and async projections (once implemented) will be the same type?

Yes, that's true: for now. If we have a strategy to have a collection per stream type, then technically, this could be placed in a separate Mongo database. For that, we'd need the client to resolve the collection. Having collection in different databases would help to have e.g. separation of data storage per module or per tenant and still use a single event store singleton setup.

alex-laycalvert · 2024-12-06T23:05:48Z

src/packages/emmett-mongodb/src/eventStore/mongoDBEventStore.ts

+  private readonly client: MongoClient;
+  private readonly defaultOptions: {
+    database: string | undefined;
+    collection: string | undefined;


I think we should add some explicit documentation that not providing a collection would have each stream type separated into a separate coll., but providing a collection would only use that for everything. Just to avoid it not being obvious that this changes how the streams are stored.

Maybe we also allow for a collectionFor option to allow the dev the option to deal with this.

I think we should add some explicit documentation that not providing a collection would have each stream type separated into a separate coll., but providing a collection would only use that for everything. Just to avoid it not being obvious that this changes how the streams are stored.

Yes, that's a good call, what would you say if we force user to make an explicit choice through the option and have event store options that'd have a field in event store option { storage: 'SINGLE_COLLECTION' } or { storage: 'COLLECTION_PER_STREAM_TYPE' } , that'd allow to discriminate user choice and force to e.g. provide collection for the first option, and client for the second (and having that mutually exclusive).

Maybe we also allow for a collectionFor option to allow the dev the option to deal with this.

Just to double-check: would you give user an option to provide their own resolution function through store options? I like the idea!

Yeah choosing single/multi collection storage I think is the way to go. I think something like this:

type Options = | { type: 'SINGLE_COLLECTION', collection?: string } | { type: 'COLLECTION_PER_STREAM_TYPE', collectionFor?: (streamType: string) => Promise<Collection> | Collection }

I think it can be the same type def as the private collectionFor. Could allow for grouping certain stream types together into the same collection, not that it's necessarily useful.

I added issue for that: #142

alex-laycalvert · 2024-12-06T23:10:55Z

src/packages/emmett-mongodb/src/eventStore/mongoDBEventStore.ts

+    if (this.isClosed) return Promise.resolve();
+
+    this.isClosed = true;
+    if (!this.shouldManageClientLifetime) return Promise.resolve();


Probably should add some docs either on close or on the client option to make sure the user knows that we only attempt to close the client if we are managing it. Looking at the code I can see this and understand why but I can imagine a scenario where a user attempts to close the event store but they had passed a client, and even though this.isClosed is true, the client is still open and calling this function has no effect on it.

We might also want to add some checks for isClosed in collectionFor or the individual functions for interacting with the event store. I think if the client is actually closed it'll throw an error when you try to use it and it's been closed, but in the above example, you could technically call close on the eventstore even if you passed your own client and it essentially does nothing.

I might be missing something obvious in why this was intended, just my thoughts.

Probably should add some docs either on close or on the client option to make sure the user knows that we only attempt to close the client if we are managing it.

Yes, makes sense. Besides docs, I could also make some typescript Voodoo with type interference and expose the close method only if the user passed the client through options (so essentially, we'd have different types for managed event store and "ambient" one). But I'm not sure if it'd be more intuitive or not. Currently calling close is a safe default for both options, as it shouldn't cause any side-effects. Thoughts? 🤔

We might also want to add some checks for isClosed in collectionFor or the individual functions for interacting with the event store.

I was thinking on it, but then if someone closed the event store then it'll get the same MongoDB errors as they'd get from a regular usage. But it may be worth indeed wrapping it with some helper and more user-friendly Emmett errors. Thoughts? 🤔

Yeah that makes sense, I think that's fine. Some TS magic to not expose close if client is provided would probably be enough.

Added issue for that: #143

alex-laycalvert

I think we might just want to add some end user docs before things get too fancy. It might just be me but there's definitely some functionality in here that's not entirely obvious from what this module exports that could cause unexpected behavior.

oskardudycz · 2024-12-07T08:35:10Z

@alex-laycalvert thank you a lot for a honest review, that's much appreciated!

oskardudycz · 2024-12-10T13:28:32Z

@alex-laycalvert, per our agreement on Discord, I've set up issues for the follow-up typing improvements. As they're not blocking, let's get this PR in, and I'll work in the next few days to fill the missing gap.

oskardudycz added enhancement New feature or request event store breaking change mongodb labels Dec 6, 2024

oskardudycz added this to the 0.23.0 milestone Dec 6, 2024

Renamed events to messages

455e3ec

In the longer term, we want to store also commands sometimes, not only events. This change should stabilise the payload.

oskardudycz force-pushed the mongodb_event_store_initialisation branch from f4ee088 to 455e3ec Compare December 6, 2024 13:36

Bumped to 0.23.1-alpha.1

2f6d2eb

oskardudycz force-pushed the mongodb_event_store_initialisation branch from 02f99c9 to 2f6d2eb Compare December 6, 2024 15:23

alex-laycalvert reviewed Dec 6, 2024

View reviewed changes

This was referenced Dec 6, 2024

[MongoDB EventStore] Inline Projection Helper for reading from read models #137

Closed

[MongoDB EventStore] mongoose model support #138

Closed

alex-laycalvert mentioned this pull request Dec 7, 2024

[MongoDB EventStore] Add query functions for inline projections #139

Merged

1 task

oskardudycz force-pushed the mongodb_event_store_initialisation branch from c02d61d to 874a976 Compare December 9, 2024 09:34

Fixed setting streamName and streamPosition on inserting new stream

50b31e7

oskardudycz force-pushed the mongodb_event_store_initialisation branch from 874a976 to 50b31e7 Compare December 9, 2024 09:42

This was referenced Dec 10, 2024

Add an explicit switch for MongoDB collection storage #142

Open

Wrap MongoDB setup to return explicit store with close method when it manages Mongo client #143

Closed

oskardudycz merged commit d3303a4 into main Dec 10, 2024
3 of 4 checks passed

oskardudycz deleted the mongodb_event_store_initialisation branch December 10, 2024 13:28

oskardudycz mentioned this pull request Dec 17, 2024

Made the MongoDB event store explicit on whether it can close MongoDBclient or not #150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reshaped MongoDBEventStore initialisation #136

Reshaped MongoDBEventStore initialisation #136

oskardudycz commented Dec 6, 2024 •

edited

Loading

oskardudycz commented Dec 6, 2024 •

edited

Loading

alex-laycalvert Dec 6, 2024

oskardudycz Dec 7, 2024

alex-laycalvert Dec 6, 2024

oskardudycz Dec 7, 2024

alex-laycalvert Dec 7, 2024 •

edited

Loading

oskardudycz Dec 10, 2024

alex-laycalvert Dec 6, 2024

alex-laycalvert Dec 6, 2024

oskardudycz Dec 7, 2024

alex-laycalvert Dec 7, 2024

oskardudycz Dec 10, 2024

alex-laycalvert left a comment

oskardudycz commented Dec 7, 2024

oskardudycz commented Dec 10, 2024

Reshaped MongoDBEventStore initialisation #136

Reshaped MongoDBEventStore initialisation #136

Conversation

oskardudycz commented Dec 6, 2024 • edited Loading

1. Reshaped MongoDBEventStore initialisation to allow passing client or connection string.

2. Renamed events to messages

oskardudycz commented Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-laycalvert Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-laycalvert left a comment

Choose a reason for hiding this comment

oskardudycz commented Dec 7, 2024

oskardudycz commented Dec 10, 2024

oskardudycz commented Dec 6, 2024 •

edited

Loading

oskardudycz commented Dec 6, 2024 •

edited

Loading

alex-laycalvert Dec 7, 2024 •

edited

Loading