Open jspuij opened 5 years ago
This is probably one of the main reasons why I wanted to start from scratch for Restier V2 (the version that will be exclusively .NET Core). It is VERY difficult to run multiple Restier instances in the same app currently. I don't think the original developers envisioned this, but I've run into several use cases where it is necessary.
I would suggest being careful about how you proceed based on this information. I HATE the DI implementation in Restier, and you're on the right track. But I'm really nervous about making this many changes so close to shipping V1.
I very much appreciate you doing this, but we need to think about at what point have we fixed DI enough for V!, and at which point do we branch to V2 and rebuild this thing to use Pipes and be Async everywhere and be streamlined AF for the next version.
I'm always a little wary to start over, because it will introduce a lot of bugs that were fixed before, you lose the collective knowledge that was accumulated in the project. Nonetheless, the code base as it is is arguably worse than when it started in 2014. A lot of the fixes I've done are more or less reversals from stuff done over the years. It does not hurt to take a look at how everything worked 5 years ago and incorporate that into a future design.
However, my preferred route would be the following:
Fix the most obvious problems in this codebase for an interim V1.x version. This will make participants (like me, but hopefully more) familiar with the codebase. The only way for me to get to know code, is to work with it.
Create serious unit tests for everything. The amount of unit tests is very small and mostly test edge cases for stuff that was not working in the past. A comprehensive set of unit tests, will make it very easy to achieve feature parity in the future, track progress, and catch those regressions that will take place once you start over. This includes meaning full feature or integration tests. You've started those already, and I think there could be a lot more.
Document the current code base, use stylecop analyzers. This way at least we have something that looks tidy. This will encourage future developers to adhere to the styles and document everything.
Release V1.x
Branch and refactor, tests and interfaces first. Then work to achieve 100% working tests again.
Regarding .NET core only. Is there a pressing reason to do that? Do you mean .NET core or ASP.NET core? .NET core means EF core as well, and there are some major issues open in EF core that prevent us from migrating to EF core.
If you look at https://github.com/aspnet/EntityFrameworkCore/issues?q=is%3Aissue+is%3Aopen+sort%3Acomments-desc there are some serious issues open that do work in EF6, and are postponed indefinitely. It seems the kind of sad state a lot of MS projects are in lately. Dropping support for EF6 will force users to go to EF core.
I'm not completely against, because at some time in the future I will have to move away from EF6 anyway, so I might choose to write a restier provider for NHibernate or LLBLGen, both of which I have better experiences with.
Wow, I just learned about https://devblogs.microsoft.com/dotnet/announcing-entity-framework-6-3-preview-with-net-core-support/. Seems that isn't a concern anymore.
Api.GetModelAsync is an asynchronous operation that asynchronously builds a model. This can potentially be an operation that takes a little time, with possibly I/O which makes it a good candidate to execute it asynchronously. Once the model is Built, It's cached internally in the API method and added to the DI container.
This creates multiple problems:
Not every method in the project is asynchronous. This creates problems when trying to execute Api.GetModelAsync inside a synchronous method. Calling api.GetModelAsync().Result (or GetAwaiter().GetResult() will block the calling context and potentially cause a deadlock. This is done a few times in the code base. It's beginners mistake nr #1 with async code.
Multiple methods call GetService() and other methods call api.GetModelAsync. Now we may hope that the service registration for IEdmModel calls api.GetModelAsync, (and the default implementation does) but if it does not, we have different models in different places.
A bigger problem is that we cannot add multiple IEdmModel instances anymore into the DI container, so now we need multiple containers, one for each ApiBase instance. IF you put both instances in a single container you get random (or defined by the DI implementation) behaviour. And GetService() != api.GetModelAsync anymore. What a mess.
You hide the fact that model building takes time behind GetService() if the DI supports lazy initialization and can create blocking code like this.
To counter this I propose a simple solution:
This is way better than the SO COOL lazy initialization of the model, which will bite you the way it's implemented now. If you want lazy initialization you can always do that through DI as most DI frameworks can delay instantiation of a dependency until it's necessary and can execute a delegate that calls the GetModelAsync (GenerateModelAsync would be a better name) after instantiation but before it's injected as a dependency.