search

AleRoe / LiteDB.Extensions

Extension methods for setting up LiteDB in Microsoft.Extensions.DependencyInjection.
MIT License
11 stars 5 forks source link

Suggestion for registering collection #2

Closed timemaster67 closed 2 years ago

timemaster67 commented 3 years ago

Hello, I've created this extension after using your library. Perhaps you would be interested ? Up to you to decide if you do something with it or not.

https://gist.github.com/timemaster67/6135f0a04225d348b598187e0c04d88d This allow you to receive a ILiteCollection instance in the constructor of your class, instead of a LiteDatabase instance.

The first method was designed so that I can add as many call to collection.AddTransient as I have LiteDb entity. I would have one call to AddLiteDbRegistries in my startup. Thought it might not be library friendly....

I've added the second method, where you can easily add all your entity to the startup like this services.AddLiteDbRegistries<Persistence.Entity.xxxxx>(); services.AddLiteDbRegistries<Persistence.Entity.yyyy>();

Do note that I am still new to LiteDb and I am unsure if those should be Transient, scoped or Singleton. You're experience here might know the answer it so I will still share this unfinished idea.

AleRoe commented 3 years ago

Could you provide a sample of how your are fetching the ILiteCollection instance please.

timemaster67 commented 3 years ago

Sure, take a look at https://github.com/timemaster67/LiteDbExtensionExample on how I retrieve a ILiteCollection basically instead of retrieving the LiteDatabase, then the collection, you directly get the collection. If you need two collection, get two dependencies in your constructor. Most of the time, you don't need the whole db in 1 class.

changes : I've renamed the AddLiteDbRegistries from the gist to AddLiteDbCollection in example to be more consistent with litedb, which I think is more important. At work we uses the repository pattern and we get our EntityRepository kind of like this. Also note that I've deleted one of the method, it was not useful once the other was done.

Again, I'm fine with keeping this small piece of code on my side if it does not fit your plan. At the time, I thought that I would make all the suggestion if I was to anyway support the case of no connection string.

And by the way thanks for the update of the library, I've pulled your latest version in my project.

AleRoe commented 3 years ago

Hi @timemaster67 thanks for the sample. Personally, I think AddLiteDbCollection<>() is not ideal as it requires that AddLiteDatabase() is also called. As an implementor, I would not expect an extension method having a dependency on another extension method. That said, AddLiteDBCollection<>() should be an extension method for AddLiteDatabase(). Something like: services.AddLiteDatabase().AddLiteDBCollection<T>(). This would require AddLiteDatabase() to return a builder instead of the service collection so that AddLiteDBCollection() can only be called on AddLiteDatabase() to make it consistent. What do you think?

Many thanks again for pointing out the issue with the ConnectionString, I hope it works out for you now. And thanks for using my library! Let me know if you have any other suggestions or comments.

Cheers Alexander

timemaster67 commented 3 years ago

I did not know about the builder design pattern, it was a nice read, thanks. I've thought about that during the week and I think I will close this issue without change. I don't see the builder design fitting when you have only one method in it (ie the new suggestion).

If we need to put time to make a builder, I would instead go to the complete/proper solution of registering my component by convention/by interface and scanning the assembly. I would inherit each of my entity inside of LiteDb with a ILiteDbEntity interface, and would create a method so that the DI container/method scan the assembly, find all the types that implement ILiteDbEntity, and register them with the ILiteCollection like I did previously in the extension method.

This extension was to keep things simple when not going overboard with the full assembly scanning. Going though the builder with only one method, does not seems that useful. I can simply call services.AddLiteDBCollection(); multiple times.

Perhaps this library configuration might have been a better fit for the builder than this AddLiteDBCollection. You expressed in my previous pull request that you wanted the core function to always be there, and it could be overwritten by other calls to add options, connection string, and etc. This seems by design compatible with a builder.
AddLiteDb() .WithConnectionString(":memory:") .WithConfiguration( new LiteDatabaseServiceOptions() { // oops connection string would be overwritten with null } ); ? Anyway, I won't need this builder, so I leave you to close this issue. I'm still open to suggestion. If you do really want a builder I can try doing one just for fun even if there is no full use case. Thanks for you time ! :)