Skip to content
This repository was 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
CumpsD wants to merge 2 commits into
bcuff:masterfrom
CumpsD:feature/ef-implementation
Open

Add wrapper for DbCommand, DbConnection and DbTransaction to support EF#4
CumpsD wants to merge 2 commits into
bcuff:masterfrom
CumpsD:feature/ef-implementation

Conversation

@CumpsD

@CumpsD CumpsD commented Apr 29, 2018

Copy link
Copy Markdown
Collaborator

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

CumpsD commented May 3, 2018

Copy link
Copy Markdown
Collaborator Author

/poke @bcuff any feedback on merging this?

@bcuff

bcuff commented May 3, 2018

Copy link
Copy Markdown
Owner

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

@bcuff bcuff left a comment

Copy link
Copy Markdown
Owner

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

CumpsD commented May 7, 2018

Copy link
Copy Markdown
Collaborator Author

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

bcuff commented May 7, 2018

Copy link
Copy Markdown
Owner

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