Fix | The SqlConnection.FireInfoMessageEventOnUserErrors when set to true throws an exception

[arellegue]

Fixes #2388

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.84%. Comparing base (769b982) to head (2ddd21c).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2399      +/-   ##
==========================================
- Coverage   72.71%   66.84%   -5.87%     
==========================================
  Files         310      304       -6     
  Lines       61885    61576     -309     
==========================================
- Hits        44997    41158    -3839     
- Misses      16888    20418    +3530     

Flag	Coverage Δ
addons	∅ <ø> (∅)
netcore	71.01% <100.00%> (-5.91%)	⬇️
netfx	64.62% <100.00%> (-5.63%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JRahnama]

The proposed fix seems satisfactory for merging as it stands. However, it's important to note that this portion of the code was altered by #1825. Given that it introduces a new feature to the driver, it's essential to conduct a thorough review to ensure that the implementation behaves as intended in this particular scenario.

cc: @Wraith2

[DavoudEshtehari]

Fix the title with a proper description, please.

[Wraith2]

I'm intrigued. How have we managed to have to reach GetCurrentBatchCommand without executing an rpc? It should have gone through GetRPCObject in order to send anything to the server and so the rpc should be non-null.

[JRahnama]

I'm intrigued. How have we managed to have to reach GetCurrentBatchCommand without executing an rpc? It should have gone through GetRPCObject in order to send anything to the server and so the rpc should be non-null.

I am curious to understand why GetCurrentBatchCommand was added to that particular exception? GetRPCObject has not been called at that stage. I tested with Batch and it also came back null. I was wondering if we should let the default value of null to be passed on that call?

[Wraith2]

If there is never a way to call the caller of GetCurrentBatchCommand in this situation and expect there to be a batch command then we should not make the call to GetCurrentBatchCommand and just use null. If it's possible then this nullable approach is fine. It seems odd that we could raise an exception through the command reporting path without even calling the server.

[DavoudEshtehari]

LGTM, waiting for the resolution by @JRahnama and @Wraith2.

[Wraith2]

Ok. I debugged through it and understand the situation now. The library only needs to use an RPC object internally if there are parameters and in this case there aren't so it follows a path that simply sends the text without first building any sort of RPC. It'd be good to get this sort of breakdown as part of the fix so those of us following along at home can understand why the fix works.

It does raise a question or what happens if you try to setup a BatchCommand with a set of commands that have no parameters, would errors be reported correctly or crash?