Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jspuij opened this issue Jul 19, 2019 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@jspuij
Copy link
Owner

jspuij commented Jul 19, 2019

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 jspuij added the enhancement New feature or request label Jul 19, 2019
@jspuij jspuij self-assigned this Jul 19, 2019
@jspuij
Copy link
Owner Author

jspuij commented Jul 19, 2019

Part of OData#629

@jspuij
Copy link
Owner Author

jspuij commented Jul 19, 2019

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
Copy link

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
Copy link
Owner Author

jspuij commented Jul 23, 2019

I agree, but please also see #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
Copy link
Owner Author

jspuij commented Jul 23, 2019

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
Copy link
Owner Author

jspuij commented Jul 23, 2019

I've checked the OData#645 against unwanted scope issues because of my incorrect assumption, but luckily it seems that I managed to avoid them.

jspuij added a commit that referenced this issue Jul 26, 2019
jspuij added a commit that referenced this issue Aug 5, 2019
jspuij added a commit that referenced this issue Aug 5, 2019
jspuij added a commit that referenced this issue Aug 16, 2019
robertmclaws pushed a commit that referenced this issue Sep 18, 2019
robertmclaws pushed a commit to OData/RESTier that referenced this issue Sep 18, 2019
* Removed ApplyTo methods and using Microsoft.Extensions.DependencyInjection statements from Conventions classes.
Fixes #1

* Made conventionbased classes public.
Fixes jspuij#2.

* Moved extensions methods into the Invocation context.
References jspuij#5.

* Remove enumeration GetApiService as it isn't used
anywhere.

References jspuij#5

* Created constructor parameters for DefaultSubmitHandler.
Closes jspuij#6.

* Fixed unit tests. As IChangeSetInitializer and ISubmitExecutor instances now need to be resolved by the DI container on creation of an ApiBase descendant, you must register implementations.

* Made ServiceProvider private.
Fixes jspuij#9.

* Added a reference to an ApiBase instance in InvocationContext.
References jspuij#10

* Replaced DI calls to ApiBase with a reference.
Fixes jspuij#10

* Replaced a few GetApiService calls with constructor injection.
References: jspuij#16

* Get the model from the API using GetModelAsync.
References jspuij#16.

* Added constructor arguments to DefaultQueryHandler.
References jspuij#17

* Fixed unit tests. earlier dependency checks.
References jspuij#17

* Changed ApiBase to a constructor dependency for RestierBatchChangeSetRequestItem.
References jspuij#21

* Replaced Service location on Restier Serializers with constructor arguments.
Fixes jspuij#22.

* Replaced IServiceProvider with IEdmModel in EdmHelpers.
Fixes jspuij#23.

* Move ApplyTo To ServiceCollectionExtensions.
Fixes jspuij#25

* Moved RestierOperationModelBuilder.ApplyTo to ServiceCollectionExtensions.
Fixes jspuij#26.

* Removed dependency on Microsoft.Extensions.DependencyInection.
References jspuij#27

* Creates constructor arguments for RestierController.
References jspuij#27.

* Introduced interface for DbContext.
References jspuij#19

* Added await to pre- and postevents.
Fixes jspuij#15.

* Don't get the provider from the request message if all constructor arguments are supplied.

* Revert "Introduced interface for DbContext."

This reverts commit 39e5208.

* Replace IEdmModel lookup with constructor argument.
Fixes jspuij#32.

* - Updating to latest OData packages
- Adding Authenticode signing
- Removing legacy test projects.

* More project cleanup.

* Adding global.json

* Strong name signing (#647)

* Attempt to get strong name signing working

Gonna need to call in the Cavalry for this.

* Strong name key signing (unfortunately breaks unit tests).

* Updating to the new logo system to fix the unnecessary iconurl deprecation.

* Update Breakdance to a strong-named version so that the tests compile again.

* Removed ApplyTo methods and using Microsoft.Extensions.DependencyInjection statements from Conventions classes.
Fixes #1

* Made conventionbased classes public.
Fixes jspuij#2.

* Moved extensions methods into the Invocation context.
References jspuij#5.

* Remove enumeration GetApiService as it isn't used
anywhere.

References jspuij#5

* Created constructor parameters for DefaultSubmitHandler.
Closes jspuij#6.

* Fixed unit tests. As IChangeSetInitializer and ISubmitExecutor instances now need to be resolved by the DI container on creation of an ApiBase descendant, you must register implementations.

* Made ServiceProvider private.
Fixes jspuij#9.

* Added a reference to an ApiBase instance in InvocationContext.
References jspuij#10

* Replaced DI calls to ApiBase with a reference.
Fixes jspuij#10

* Replaced a few GetApiService calls with constructor injection.
References: jspuij#16

* Get the model from the API using GetModelAsync.
References jspuij#16.

* Added constructor arguments to DefaultQueryHandler.
References jspuij#17

* Fixed unit tests. earlier dependency checks.
References jspuij#17

* Changed ApiBase to a constructor dependency for RestierBatchChangeSetRequestItem.
References jspuij#21

* Replaced Service location on Restier Serializers with constructor arguments.
Fixes jspuij#22.

* Replaced IServiceProvider with IEdmModel in EdmHelpers.
Fixes jspuij#23.

* Move ApplyTo To ServiceCollectionExtensions.
Fixes jspuij#25

* Moved RestierOperationModelBuilder.ApplyTo to ServiceCollectionExtensions.
Fixes jspuij#26.

* Removed dependency on Microsoft.Extensions.DependencyInection.
References jspuij#27

* Creates constructor arguments for RestierController.
References jspuij#27.

* Introduced interface for DbContext.
References jspuij#19

* Added await to pre- and postevents.
Fixes jspuij#15.

* Don't get the provider from the request message if all constructor arguments are supplied.

* Revert "Introduced interface for DbContext."

This reverts commit 39e5208.

* Replace IEdmModel lookup with constructor argument.
Fixes jspuij#32.

* PR Adjustments

- Making DbContext public again (EasyAF uses it extensively)
- Make the sample app run again.
robertmclaws pushed a commit to OData/RESTier that referenced this issue Dec 19, 2022
* Removed ApplyTo methods and using Microsoft.Extensions.DependencyInjection statements from Conventions classes.
Fixes #1

* Made conventionbased classes public.
Fixes jspuij#2.

* Moved extensions methods into the Invocation context.
References jspuij#5.

* Remove enumeration GetApiService as it isn't used
anywhere.

References jspuij#5

* Created constructor parameters for DefaultSubmitHandler.
Closes jspuij#6.

* Fixed unit tests. As IChangeSetInitializer and ISubmitExecutor instances now need to be resolved by the DI container on creation of an ApiBase descendant, you must register implementations.

* Made ServiceProvider private.
Fixes jspuij#9.

* Added a reference to an ApiBase instance in InvocationContext.
References jspuij#10

* Replaced DI calls to ApiBase with a reference.
Fixes jspuij#10

* Replaced a few GetApiService calls with constructor injection.
References: jspuij#16

* Get the model from the API using GetModelAsync.
References jspuij#16.

* Added constructor arguments to DefaultQueryHandler.
References jspuij#17

* Fixed unit tests. earlier dependency checks.
References jspuij#17

* Changed ApiBase to a constructor dependency for RestierBatchChangeSetRequestItem.
References jspuij#21

* Replaced Service location on Restier Serializers with constructor arguments.
Fixes jspuij#22.

* Replaced IServiceProvider with IEdmModel in EdmHelpers.
Fixes jspuij#23.

* Move ApplyTo To ServiceCollectionExtensions.
Fixes jspuij#25

* Moved RestierOperationModelBuilder.ApplyTo to ServiceCollectionExtensions.
Fixes jspuij#26.

* Removed dependency on Microsoft.Extensions.DependencyInection.
References jspuij#27

* Creates constructor arguments for RestierController.
References jspuij#27.

* Introduced interface for DbContext.
References jspuij#19

* Added await to pre- and postevents.
Fixes jspuij#15.

* Don't get the provider from the request message if all constructor arguments are supplied.

* Revert "Introduced interface for DbContext."

This reverts commit 39e5208.

* Replace IEdmModel lookup with constructor argument.
Fixes jspuij#32.

* - Updating to latest OData packages
- Adding Authenticode signing
- Removing legacy test projects.

* More project cleanup.

* Adding global.json

* Strong name signing (#647)

* Attempt to get strong name signing working

Gonna need to call in the Cavalry for this.

* Strong name key signing (unfortunately breaks unit tests).

* Updating to the new logo system to fix the unnecessary iconurl deprecation.

* Update Breakdance to a strong-named version so that the tests compile again.

* Removed ApplyTo methods and using Microsoft.Extensions.DependencyInjection statements from Conventions classes.
Fixes #1

* Made conventionbased classes public.
Fixes jspuij#2.

* Moved extensions methods into the Invocation context.
References jspuij#5.

* Remove enumeration GetApiService as it isn't used
anywhere.

References jspuij#5

* Created constructor parameters for DefaultSubmitHandler.
Closes jspuij#6.

* Fixed unit tests. As IChangeSetInitializer and ISubmitExecutor instances now need to be resolved by the DI container on creation of an ApiBase descendant, you must register implementations.

* Made ServiceProvider private.
Fixes jspuij#9.

* Added a reference to an ApiBase instance in InvocationContext.
References jspuij#10

* Replaced DI calls to ApiBase with a reference.
Fixes jspuij#10

* Replaced a few GetApiService calls with constructor injection.
References: jspuij#16

* Get the model from the API using GetModelAsync.
References jspuij#16.

* Added constructor arguments to DefaultQueryHandler.
References jspuij#17

* Fixed unit tests. earlier dependency checks.
References jspuij#17

* Changed ApiBase to a constructor dependency for RestierBatchChangeSetRequestItem.
References jspuij#21

* Replaced Service location on Restier Serializers with constructor arguments.
Fixes jspuij#22.

* Replaced IServiceProvider with IEdmModel in EdmHelpers.
Fixes jspuij#23.

* Move ApplyTo To ServiceCollectionExtensions.
Fixes jspuij#25

* Moved RestierOperationModelBuilder.ApplyTo to ServiceCollectionExtensions.
Fixes jspuij#26.

* Removed dependency on Microsoft.Extensions.DependencyInection.
References jspuij#27

* Creates constructor arguments for RestierController.
References jspuij#27.

* Introduced interface for DbContext.
References jspuij#19

* Added await to pre- and postevents.
Fixes jspuij#15.

* Don't get the provider from the request message if all constructor arguments are supplied.

* Revert "Introduced interface for DbContext."

This reverts commit 39e5208.

* Replace IEdmModel lookup with constructor argument.
Fixes jspuij#32.

* PR Adjustments

- Making DbContext public again (EasyAF uses it extensively)
- Make the sample app run again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants