-
-
Notifications
You must be signed in to change notification settings - Fork 44
Feature/collection mapping #97
base: develop
Are you sure you want to change the base?
Conversation
Merge develop to master
Merge to master
Merge to master
Merge to master
Merge to master
2.0 to master
Merge to master
Latest stable to master
Merge 2.4.2 into master
Merge to master
Merge 2.5.0 to master
Merging 2.7.0
Merge version 2.8.1 to master
…eature/collection-mapping
…feature/collection-mapping
Hey @rob thanks for that. It looks really good. A few comments before I do a full review. Can you please uncomment the commented out tests? There also seem to be some merge conflicts that need to be addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several comments. Also you will need to pull the latest serialization work I did.
@@ -24,6 +25,8 @@ public sealed class CosmosStore<TEntity> : ICosmosStore<TEntity> where TEntity : | |||
|
|||
public ICosmonautClient CosmonautClient { get; } | |||
|
|||
internal EntityCollectionMapping EntityCollectionMapping { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of setting this in the ctor as the return value of InitialiseCosmosStore
can we add an internal setter and set it in there? I don't like the fact that InitialiseCosmosStore
doesn't imply what the method will be returning. It's an internal property anyway so it should be fine.
_databaseCreator = databaseCreator ?? new CosmosDatabaseCreator(CosmonautClient); | ||
InitialiseCosmosStore(overriddenCollectionName); | ||
|
||
EntityCollectionMapping = InitialiseCosmosStore(overriddenCollectionName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned at line https://github.com/Elfocrash/Cosmonaut/pull/97/files#diff-56d4febe3cf61714e5bc8bd6579d82d8R28 can we set that in the InitialiseCosmosStore
method?
@@ -38,15 +41,15 @@ public CosmosStore(CosmosStoreSettings settings, string overriddenCollectionName | |||
var documentClient = DocumentClientFactory.CreateDocumentClient(settings); | |||
CosmonautClient = new CosmonautClient(documentClient, Settings.InfiniteRetries); | |||
if (string.IsNullOrEmpty(Settings.DatabaseName)) throw new ArgumentNullException(nameof(Settings.DatabaseName)); | |||
_collectionCreator = new CosmosCollectionCreator(CosmonautClient); | |||
_collectionCreator = new CosmosCollectionCreator(CosmonautClient, settings.EntityConfigurationProvider); | |||
_databaseCreator = new CosmosDatabaseCreator(CosmonautClient); | |||
InitialiseCosmosStore(overriddenCollectionName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming you also have to set the EntityCollectionMapping
here as well right?
Ok...leave this with me. When I get a mo, I'll try to sort them out. Thanks :) |
As per the issue I raised:
#83
I've created fluent mapping methods which allow the consumer to map entities through an interface rather than decoratively through attributes. This approach gives the consumer both options. With the fluent approach, entities can be agnostic of the Cosmonaut library and the mapping concerns can be decoupled.
The attribute approach is still very valid and useful for most scenarios and will continue to work as before.