search

jspuij / RESTier

A turn-key library for building RESTful services
http://odata.github.io/RESTier
Other
2 stars 0 forks source link

DbContext is exposed as a property on the EntityFrameworkApi, but should be a method. #19

Open jspuij opened 5 years ago

jspuij commented 5 years ago

DbContext is exposed as a property on EntityFrameworkApi, but added to the DI container as Scoped. It hides the fact that it returns a new instance on every request.

I propose to make it a method GetDbContext, to indicate clearly that a new instance is returned (for every request). We should use a factory to get a new instance everytime, instead of a constructor dependency on ServiceProvider.

See https://github.com/OData/RESTier/blob/master/src/Microsoft.Restier.EntityFramework/EntityFrameworkApi.cs#L46-L52

jspuij commented 5 years ago

Part of https://github.com/OData/RESTier/issues/629

jspuij commented 5 years ago

We could also make it an abstract method to force the user to implement providing a context. Using a factory implementation through the constructor will force the user to think more "DI", but will add a bit of learning curve to the library.

robertmclaws commented 5 years ago

This would break A TON of code that is currently leveraging Restier. The behavior of this property is correct, you should be getting the DbContext that exists solely for the purpose of servicing the request. I'm down for talking about this for v2, but please don't change it for v1.

jspuij commented 5 years ago

I agree, but please also see https://github.com/jspuij/RESTier/issues/14. Since ApiBase does not return a scoped serviceprovider, DbContext (and a lot of other Scoped classes) effectively return singletons.

The only way to get a scoped container is to call HttpRequestMesage.CreateRequestContainer(ODataRouteName);

This illustrates how tricky it is to use the DI correctly. I think we should at least fix these singleton / scope errors. If we insist that we provide an implementation for requisting a DB context, let's at least do it the right way ourselves.

jspuij commented 5 years ago

I'm wrong. The documentation on ApiBase suggests that it is a singleton, but it is not. It seems that it is added as scoped. It is also created from the requestcontainer in RestierController. This makes ApiBase scoped. At least the documentation should be more clear ;-) I'll have a look at the rest of my tickets, to look whether they still make sense.

jspuij commented 5 years ago

I've checked the https://github.com/OData/RESTier/pull/645 against unwanted scope issues because of my incorrect assumption, but luckily it seems that I managed to avoid them.