NES.StringWay

[charlessolar]

Is your intention with this new namespace to make a NES that allows both Guid and string ids?
I am still playing with the v5 branch version and happen to need aggregates with string ids for a specific root.

The implementation looks like that is what you are aiming for, but IRepositoryAdd still only accepts an AggregateBase with a Guid id so its kind of useless for adding.

I am attempting to fix it up but need to know what your intentions for the namespace are

Thanks

[marinkobabic]

Hi Charles

This was exactly the intention. Are you going to use string as id?

Thanks Marinko

From: Charles Solar [mailto:notifications@github.com] Sent: Dienstag, 23. Dezember 2014 23:25 To: elliotritchie/NES Subject: [NES] NES.StringWay (#33)

Is your intention with this new namespace to make a NES that allows both Guid and string ids?

I am still playing with the v5 branch version and happen to need aggregates with string ids for a specific root.

The implementation looks like that is what you are aiming for, but IRepositoryAdd still only accepts an AggregateBase with a Guid id so its kind of useless for adding.

I am attempting to fix it up but need to know what your intentions for the namespace are

Thanks

— Reply to this email directly or view it on GitHubhttps://github.com/elliotritchie/NES/issues/33.

[charlessolar]

Only for 1 root (planned) all the rest will be guid ids.

Ok I will continue working here then. I'll update my fork of NES to support both Guids and Strings adding and getting and let you know

[charlessolar]

Ok I just got all the tests green for a Guid OR String capable NES

I ended up doing a bit more rearranging and trimming than you may like though.. haha

I hit a couple snags probably in the same places you did, namely UnitOfWork.cs and EventSourceMapper.cs

The cache mechanism in the UOW class needed some love, with String id's being supported now it is in theory possible for a user to get a collision if 2 event sources shared a same id. The problem being that repository can be used to request different types of event sources. So if event source Item has id "1" and I then requested User id "1" I would get back Item "1". I fixed that by appending the type's hash code onto the key. I also made the cache object a thread-safe dictionary for faster lookups and better thread consistency.

In both UnitOfWork.cs and EventSourceMapper.cs the problems mainly boiled down to getting the ID from the event source without knowing if the ID was a string or guid. My first stab at this I actually had NES accepting ANY type of id and parameterized all the elements that needed an id with <TId>

This worked but it required the user to append needless type information when they called _repository.get or add

Eg - _repository.get<Item>('id') became _repository.get<Item, String>('id') and _repository.add(item) became _repository.add<Item, String>(item)

Which I hated so I took a different route. I basically made 2 event source types, IStringEventSource and IGuidEventSource. This specialization means we can write repository and unit of work methods based on a single template which the compiler can resolve for us. Which restores the original api calls. However the user will have to update their code from using AggregateBase. AggregateBase inherits from IEventSource<TId> which the compiler can't resolve without that added code. So I added 2 new classes, StringAggregate, and GuidAggregate. To use _repository.add(item) the item class must inherit from either String or Guid aggregate.

Thats about all I can think of to mention at the moment. I will submit a pull request after getting the sample app running and possibly adding some more tests to cover the new code areas. In the meantime however you can always check out my fork at https://github.com/volak/NES

[marinkobabic]

Hi

The original idea was to have only string id's on the database which is a fact. So you don't need to worry if it's a string or any other type. It's always a string. If you need an other type you will convert it to whatever type your aggregate base class supports. This approach is much simpler than having the TId everywhere. Your code looks now like my first draft.

If you have just guid like we had until now, the unique id was never a problem. Why should it become a problem using different types? We are still using DDD. Using your approach I'm not able to read the event's from store without knowing the type which raised them. This problem we didn't had before. I think it's responsibility of developers to ensure uniqueness independent of the type.

The fact is that repository persists ids as string. The NES framework has to deal with it in a simple way.

Your thoughts?

Thanks Marinko

Von Samsung Mobile gesendet

-------- Ursprüngliche Nachricht -------- Von: Charles Solar Datum:24.12.2014 09:03 (GMT+01:00) An: elliotritchie/NES Cc: Babic Marinko Betreff: Re: [NES] NES.StringWay (#33)

Ok I just got all the tests green for a Guid OR String capable NES

I ended up doing a bit more rearranging and trimming than you may like though.. haha

I hit a couple snags probably in the same places you did, namely UnitOfWork.cs and EventSourceMapper.cs

The cache mechanism in the UOW class needed some love, with String id's being supported now it is in theory possible for a user to get a collision if 2 event sources shared a same id. The problem being that repository can be used to request different types of event sources. So if event source Item has id "1" and I then requested User id "1" I would get back Item "1". I fixed that by appending the type's hash code onto the key. I also made the cache object a thread-safe dictionary for faster lookups and better thread consistency.

In both UnitOfWork.cs and EventSourceMapper.cs the problems mainly boiled down to getting the ID from the event source without knowing if the ID was a string or guid. My first stab at this I actually had NES accepting ANY type of id and parameterized all the elements that needed an id with

This worked but it required the user to append needless type information when they called _repository.get or add

Eg - _repository.get('id') became _repository.get<Item, String>('id') and _repository.add(item) became _repository.add<Item, String>(item)

Which I hated so I took a different route. I basically made 2 event source types, IStringEventSource and IGuidEventSource. This specialization means we can write repository and unit of work methods based on a single template which the compiler can resolve for us. Which restores the original api calls. However the user will have to update their code from using AggregateBase. AggregateBase inherits from IEventSource which the compiler can't resolve without that added code. So I added 2 new classes, StringAggregate, and GuidAggregate. To use _repository.add(item) the item class must inherit from either String or Guid aggregate.

Thats about all I can think of to mention at the moment. I will submit a pull request after getting the sample app running and possibly adding some more tests to cover the new code areas. In the meantime however you can always check out my fork at https://github.com/volak/NES

Reply to this email directly or view it on GitHubhttps://github.com/elliotritchie/NES/issues/33#issuecomment-68034882.

[charlessolar]

The issue I originally had with the StringWay implementation was missing IRepositoryAdd for string ids. I originally tried to just add one, but then realized that if IRepository is specialized into only accepting Strings or Guids then in the event handlers we would need two repositories if the handler needed to access both types of entities.

Even though NEventStore is converting the ids to strings at the end, I don't think we should only support string ids. I just committed a new change which incorporates some of your ideas. AggregateBase will now calculate an 'EventSourceId' which is just the Id.ToString() This eventsourceid will be used for all interactions with the event store, but still allow the user to use whatever id they want.

I also changed the structure a bit to remove the need to reference either NES.Contracts or NES.StringWay which makes for a cleaner library.

In regards to the caching, it kind of makes more sense to me to place this duty in the event store adaptor. I was looking at the NEventStore.CommonDomain project and they are caching the streams and snapshots very nicely https://github.com/NEventStore/NEventStore/blob/39117af576256f93f4b61f3bd5786a5dee9a7336/src/NEventStore/CommonDomain/Persistence/EventStore/EventStoreRepository.cs

Also, if I go ahead and change that I am thinking about ax'ing the EventSourceMapper class entirely and put repositories on the UOW object itself. Which would mean users would use

class Handler : IHandleMessages<Created> {
    IUnitOfWork _uow

    public void Handle( Created event ) {
        var item = new Item( event.id );
        _uow.Repository<Item>().Add(item);
    }
}

The extra call in there will create a new repo class per type that is used in a UOW, which would solve ANY collision issues as well as allow IEventSource to be a bit more robust since it will have a better idea of what type its committing. When we have a typed repository I can implement the repository pattern from CommonDomain which will in the end allow me to remove EventSourceMapper

Thoughts?

[charlessolar]

Right after typing all that I had second thoughts about putting unit of work in event handlers.

Perhaps something like _repository.For<Item>().Get('id') would be better.. or we could even force Repos to be declared for a single type like

private readonly IRepository<Item> _repository hmm

I really wish the compiler was smart enough to figure out

_repository.Get<Item>('id') from definition T Get<T, TId>( TId id ) where T : IEventSource<TId>

[marinkobabic]

Hi Chirles

Thanks for helping me :-)

As you can see there is already a virtual method which let's you covert any id to type of string.

I like your ideas and will analyze your changes as soon as possible and give you a feedback before we merge the code.

In generally I think that IRepository must be type specific. So there is some refactoring required and therefore I will check your code.

Thanks a lot Marinko

Von Samsung Mobile gesendet

-------- Ursprüngliche Nachricht -------- Von: Charles Solar Datum:24.12.2014 17:59 (GMT+01:00) An: elliotritchie/NES Cc: Babic Marinko Betreff: Re: [NES] NES.StringWay (#33)

The issue I originally had with the StringWay implementation was missing IRepositoryAdd for string ids. I originally tried to just add one, but then realized that if IRepository is specialized into only accepting Strings or Guids then in the event handlers we would need two repositories if the handler needed to access both types of entities.

Even though NEventStore is converting the ids to strings at the end, I don't think we should only support string ids. I just committed a new change which incorporates some of your ideas. AggregateBase will now calculate an 'EventSourceId' which is just the Id.ToString() This eventsourceid will be used for all interactions with the event store, but still allow the user to use whatever id they want.

I also changed the structure a bit to remove the need to reference either NES.Contracts or NES.StringWay which makes for a cleaner library.

In regards to the caching, it kind of makes more sense to me to place this duty in the event store adaptor. I was looking at the NEventStore.CommonDomain project and they are caching the streams and snapshots very nicely https://github.com/NEventStore/NEventStore/blob/39117af576256f93f4b61f3bd5786a5dee9a7336/src/NEventStore/CommonDomain/Persistence/EventStore/EventStoreRepository.cs

Also, if I go ahead and change that I am thinking about ax'ing the EventSourceMapper class entirely and put repositories on the UOW object itself. Which would mean users would use

class Handler : IHandleMessages { IUnitOfWork _uow

public void Handle( Created event ) {
    var item = new Item( event.id );
    _uow.Repository<Item>().Add(item);
}

}

The extra call in there will create a new repo class per type that is used in a UOW, which would solve ANY collision issues as well as allow IEventSource to be a bit more robust since it will have a better idea of what type its committing. When we have a typed repository I can implement the repository pattern from CommonDomain which will in the end allow me to remove EventSourceMapper

Thoughts?

Reply to this email directly or view it on GitHubhttps://github.com/elliotritchie/NES/issues/33#issuecomment-68063916.

[marinkobabic]

To fix your original issue we need to change the constraint of IRepositoryAdd to IEventSourceBase instead of IEventSource.

But like said I will go through your code :-)

[charlessolar]

If/when you check out the code, you'll want to look at the tree at this point https://github.com/volak/NES/tree/07e9466f89e5904de6717403f34481a88a864345

The latest code on that branch is broken as I have been working on the new repo/uow stuff but at that commit everything works fine.

[marinkobabic]

Hi Charles

I like a lot of changes you have made. I simplifies the implementation of supporting multiple types. Here some feedbacks, based on your actual version v5:

Please remove the type from the id of the eventstore like already mentioned. Id should be just an id and not concatenated with type. The domain has to ensure that it's still unique resp. the domain can do the concatenation if needed for the uniqueness.

The conversion to EventSourceId is in your code Id.ToString(). Because we support all types the Id can be null, a custom class or a value type. If it's a value type we can do ToString. If it's a custom class, then ToString is maybe not a good idea. Maybe they can implement a specific interface for conversion to string. If the interface is not implemented, we will do a ToString. Actually you can fail because of nullreferenceexception if the id is null.

The actual IRepository should support by default the Guid so that the change to v5 is simple. My expectation was that the consumer will use int, string or guid for the domain. The mix of multiple id types should be rare. This is the reason of separate namespace StringWay. So we can have additional nuget packages for string support, int support and so on. Therefore I kept the interface name and didn't change it.

The rest of your changes is very good, so just go on and make a pull request as soon as you are done :-)

Thanks Marinko

[charlessolar]

I created a new branch of my v5 branch called v5-pr which will contain the changes you requested. It it available here https://github.com/volak/NES/tree/v5-pr

So far I have removed the type.GetHashCode code from the cache key and added a default AggregateBase definition for Guid ids.

IRepository as it is now will accept only String, int, and Guid ids for Get'ing, the user could add extension methods if he wants to do custom types. Which is the reason I was not too concerned about them using a type that was not supported.

Since in my branch I don't have StringWay anymore you'll have to divide up the implementations into the different namespaces if that is really what you want.

Actually as I mentioned before, over the weekend I got really aggressive with the codebase and rewrote essentially everything. It started with a realization that NES.NServiceBus and NES.NEventStore didn't need to be their own projects, and keeping it that way actually made the project 10x more complex. What we are doing here is just creating an event sourced root, committing changes to event store, then hooking into dispatch to publish the events out to nservicebus. Seperating these projects creates a whole headache of duplicate implementations. ie: EventFactory, ISnapshot, EventConverter, BucketSupport - and the list goes on. These features are implemented in NServicebus or NEventStore but because NES is a separate project the main classes can't use them.

So over the weekend I merged all the projects together and deleted almost all the duplicate work, then I reintegrated NEventstore and NServicebus and deleted more code. Then wrote some of my own stuff, and eventually my new project Aggregates.NET was born. Its ideas draw heavily from NEventstore.CommonDomain and NES but it does things very differently.

For one, it supports the idea of an event router, which will allow me to implement aggregate entities in the near future. It will also be able to do conflict resolution and should support basic business constructs such as specifications out of the box. And because it directly references NServicebus and NEventstore I get all their features for free such as buckets, event conversion and versioning, etc.

Anyway let me know what else you'd like me to change on v5-pr, once everything looks good I'll submit the pull request. Most of my time though will be spent on the new stuff though.

Cheers

[marinkobabic]

The implementation has been simplified.