NH-3912 Oracle IBatcher impl instance is not reusable after failed operation. #513
Conversation
|
@waynesayonara I've restricted tests to the Oracle specific dialects only, fixed the indents. So I had to force push your changes with rebase on current master. |
|
@hazzik Do I need to fix unsuccessful checks at this point (3 of them) or you will merge it anyway? |
|
They are unrelated, these tests are fluffy. |
| catch (DbException e) | ||
| { | ||
| // Message from DbException could be useful | ||
| var additionalMessage = string.IsNullOrEmpty(e.Message) ? "." : string.Concat(": ", e.Message); |
There was a problem hiding this comment.
Not sure why this is required. I've not run it, but looking at the code is seems the exception e should eventually end up as inner exception inside the "converted" exception and the message can and should be read from that.
There was a problem hiding this comment.
@oskarb this is how it was already implemeted. The real change here is additional try-finally around the execution block. So zeroing of varaibles go to finally.
I think it shall go to 4.1
There was a problem hiding this comment.
additionalMessage in string.Format("could not execute batch command{0}", additionalMessage) is new. That's what I'm referring to.
There was a problem hiding this comment.
In MinimalSQLExceptionConverter and SQLStateConverter the actual exception will end up as an inner exception of GenericADOException. So, I think it's fine. Or, we can amend the PR to remove this change.
There was a problem hiding this comment.
Yes, that's my point, it's already there. Based on the comment in the PR and also in the code, the additionalMessage seems to have been added based on the idea that without it, the message from the original exception was somehow lost. As far as I can tell, the inner exception will be there as expected and then for anything that does log the exception chain properly, this will just be unfortunate duplication. t's not a big issue and I don't care too much either way, but I think it should be removed.
hazzik
force-pushed
the
NH3912
branch
2 times, most recently
from
3f18333 to
87b8d9c
Compare
…pected rows count after single failed operation.
|
@oskarb I've changed the PR |
|
Sure we can have this in 4.1. |
|
I've missed one point on this pull request... The user shall never reuse session after an exception. Ever. |
Batch operations with the same IBatcher instance fail on expected rows count after single failed operation.
Also, passed DbException message for oracle batcher to outer message.
This issue regards OracleDataClientBatchingBatcher. It seems that in the scenario where two batch commands are performed using the same instance of OracleDataClientBatchingBatcher, second batch command will nearly always fail on expected rows count assert. This will only happen if first batch command failed before the second one (any error will do). The reason is that internal counter _totalExpectedRowsAffected isn't reset after first failed batch command.
Example:
BATCH1:
_totalExpectedRowsAffected += 1 (now 1, was 0)
Insert into table and fail on unique constaint violation for instance
_totalExpectedRowsAffected is not reset.
BATCH2:
_totalExpectedRowsAffected += 1 (now 2, was 1)
Insert into table and succeed
Assert Expectations.VerifyOutcomeBatched fails because _totalExpectedRowsAffected and only one row was actually affected.
Every consecutive batch command will also fail.
Pull requests contains following changes: