Skip to content
This repository has been archived by the owner on Nov 9, 2022. It is now read-only.

Add wrapper for DbCommand, DbConnection and DbTransaction to support EF #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add wrapper for DbCommand, DbConnection and DbTransaction to support EF #4

wants to merge 2 commits into from

Conversation

CumpsD
Copy link
Collaborator

@CumpsD CumpsD commented Apr 29, 2018

Entity Framework is not able to deal with IDbCommand and IDbConnection, instead it wants the abstract class DbConnection, DbCommand. Internally it also does a reference compare between the connection and the connection of a transaction, because of this, we also wrap a DbTransaction in a TraceDbTransaction, as a nice side effect, it allows us to add spans for commit and rollback.

@CumpsD CumpsD assigned CumpsD and unassigned CumpsD Apr 29, 2018
@CumpsD CumpsD requested a review from bcuff April 29, 2018 00:10
@CumpsD CumpsD mentioned this pull request Apr 29, 2018
@CumpsD
Copy link
Collaborator Author

CumpsD commented May 3, 2018

/poke @bcuff any feedback on merging this?

@bcuff
Copy link
Owner

bcuff commented May 3, 2018

@CumpsD yeah. it looks good on first glance. I just haven't had the time yet. I'll get back to you soon. Thanks.

Copy link
Owner

@bcuff bcuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not have separate code paths for TraceDbConnection for entity framework and otherwise. Why not modify the existing TraceDbCommand/Connection to inherit DbConnection instead?

@CumpsD
Copy link
Collaborator Author

CumpsD commented May 7, 2018

I didnt want to introduce breaking changes. People might be using the IDbConnection one.

If you dont mind I can replace the existing one with this implementation?

Dont forget, for EF I also had to introduce TraceDbTransaction.

@bcuff
Copy link
Owner

bcuff commented May 7, 2018

Yes, go ahead and modify the existing one rather than creating new ones. Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants