Fix | Default SqlCommand's transaction to owning SqlConnection's local transaction #330
Conversation
…cy, but kept validations in place. (#28)
…it the new paradigm.
Made a small change in SqlInternalTransaction's pre-BeginTransaction() validation.
…er after removing tests that became irrelevant.
…on validation against zombied Transactions to throw. Also changed SqlConnection.CurrentTransaction to return null for zombied Transactions.
…gument for the test to pass.
…aking a test fail. It made user/pass empty and removed integrated security breaking login. Changed it to respect integrated security from the input connection string builder.
…it succeeded. The value inserted is actually '2000-12-31', checked in unencrypted table. Noticed in the SqlParameter a .CoercedValue that was 2000/12/31 so that's why it inserts the value. Changed this value to '0/12/32' for it to actually fail.
…cateUtilityWin.CreateCertificate.
…Transaction) and external (vial TSQL) overlapped transactions.
|
@CodeAngry please address failing builds from logs by making necessary changes. Thanks! |
|
Thank you! I'll finalize all things by tomorrow. PS: I forgot it's older C# language version. I use |
…testing. Even if I find this useful, it could be added later on if/when necessary.
Fixed a forgotten netfx block code that only had effect before the change. Also moved a check in a netcore function few lines up to reduce pointless initializations if we throw.
I don't have anything Azure related.
| @@ -376,14 +373,6 @@ private SqlCommand(SqlCommand from) : this() | |||
| } | |||
| } | |||
|
|
|||
| // Check to see if the currently set transaction has completed. If so, | |||
There was a problem hiding this comment.
Behavior change. I have a command lying around and the transaction was changed for the command. Now I associate the new connection with it, and it magically overwrote previous transaction.
Can we stick to the lowest common denominator on how this convenience of deriving the transaction for this command is going to work, while preserving most of the old behavior ?
There was a problem hiding this comment.
@saurabh500 Behavior change. I have a command lying around and the transaction was changed for the command. Now I associate the new connection with it, and it magically overwrote previous transaction.
Since a transaction's lifetime is BeginTransaction - Commit/Rollback/throw, and a single connection has a single active transaction, how can a command be lying around with a different transaction that magically changes? If it had a different transaction reference associated, and the connection ran another transaction, then it's a complete transaction which used to be nullified internally in the old functionality too.
If you associate a new connection to it, you don't do it in the hopes of keeping the old transaction on the .Transaction property and a different connection in the .Connection. This would not have passed validation previously also.
A few lines of code explaining this behavior would be helpful.
There was a problem hiding this comment.
Could you provide a few lines of code to verify how these code changes would break?
Also removed my SqlBulkCopy tests.
…ails on Azure but works locally. Have no experience in TransactionScope, I'll just remove it and someone else can add something if needed.
|
I've done all I can on my side. It's out of my hands now. I'll personally use this changed version in my projects from now. It's nice that @roji and the npgsql team got this right the first time. I've added several tests, I don't see how this changes can affect existing code in any way, given how connections, transactions and commands relate to each other and their lifespans. Also, the I'm open to any questions but if someone says this breaks backward compatibility, it should be accompanied by a few actual lines of code showcasing the issue. |
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionTest.cs
Show resolved
Hide resolved
|
So, it'a been a while... |
|
Any update on this, please? Making the project open source and not accepting changes for months (and years) makes Microsoft's open source commitment just a demagogy. /cc @yorek, I saw your recent post about this project being open sourced and recently released. |
|
@abatishchev I'm done here. I'm not even trying to get this in. If they want it, they have it here. Imagine having this dangling but complete for 1 year. Where else but with Microsoft could that fly? If my devs left something done hanging for 1 year I'd fire them all. Almost one year ago when this was complete, it passed all tests. Not I see it has conflicts. Code changes were trivial at best but 0 **** were given. One MS dev could have wrapped this up in 15mins considering I made the changes with 0 prior knowledge of the code base or GIT. I personally quit bothering them. They are TOO busy. Maybe in 20 years something might change. Who knows?! |
|
Please accept our apologies for not getting this addressed soon. We were held upon an unresolved comment above (#330 (comment)) that seemed like a backward compatibility concern, but I believe we need to resume testing this again thoroughly for any possible breaking changes. Would you be able to resolve conflicts that came in recently, please? while we resume review. |
cheenamalhotra
requested review from
karinazhou,
DavoudEshtehari,
JRahnama and
johnnypham
There was a problem hiding this comment.
- I know you suggest this change a while ago, one of the added features is
EventSource, it is required to add some lines to cover logging. You can follow the recent code pattern to discover it. - Also, by the purpose of running manual tests on Azure SQL Server, we create a unique table instead of temporary tables by using
DataTestUtility.GetUniqueNameForSqlServer. - You can use
DataTestUtility.ConnectionStringsto cover all variants of a connection string. - Remove all unused
namespacesandparametersjust for a neat code.
|
@CodeAngry , @abatishchev I'm very sorry for what happened, and definitely we need to make this right. I asked our engineering team to take a look at this, and I see that they already responded. We are absolutely serious about open source commitment: for how bad this may look, please do not assume this is the norm. Actually many PR has been submitted and accepted. So, we can do better? For sure. Should be judged by just one mistake? Hopefully not :) Thanks, a looking forward to see your work included in SqlClient! |
| { | ||
| var csb = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) | ||
| { | ||
| MultipleActiveResultSets = false, |
There was a problem hiding this comment.
Your change has affected the MARS behavior. Please, add more test units to cover it.
| if (value is null) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I understand that a null value here wouldn't matter as the command will just use the connection's transaction. But since this setter is validating the input, shouldn't null also throw? A user assigning null to this Transaction property will think the command will execute under that transaction, which they believe to be null.
|
@CodeAngry we have been waiting for your response to address the comments and concerns. Please update the PR. If the PR is not updated a parallel PR will close this one. |
|
Looks like there is conflict in |
Is a parallel PR being worked on or planned, or is this just a hope that a parallel PR will be submitted by someone? |
|
Hi @initram We were waiting for @CodeAngry to update the PR, as there are quite a many feedback comments waiting resolution. As of now, our team is occupied with other priority activities, but it is in our backlog. |
|
Don't wait for me. I don't work in year or 6mo increments. As I said, any MS developer that's slightly intimate with this library can fix this in 15minutes with a coffee break. I can't!... wait months to anything. Anyone who wants a fix for this is to never use MARS, SqlTransaction, Scoped Transaction and always use If you want just create a |
Hi @CodeAngry
Opening PR from your branch to engage reviewers and understand changes better!
You may continue to work on the same branch while you receive reviews/feedback.
Fixes #28