Fix protocol desync while reading output parameters from a function #5840
Conversation
|
#5645 🙃 |
Sadly, it's probably not going to fix the bug :( |
|
Right, so the approach I took there was to actually log an error for the failed parameter (in line with us ignoring returnvalue or parameters with a non matching name) and leave it at that, so I'm not sure how we would be able to get an exception up into NextResult at all. Of course whether we want to log + ignore the offending param vs terminate + consume is something to discuss. |
|
Ah, I missed the part around |
That's a good argument. I don't like that part in general about the output param flow, there is no way to feed back status to users (unless we add a ton of api surface to parameters). |
| Direction = ParameterDirection.InputOutput, | ||
| Value = 1 | ||
| }); | ||
| Assert.ThrowsAsync<InvalidCastException>(cmd.ExecuteNonQueryAsync); |
There was a problem hiding this comment.
Just for my mental picture, what was the reason we would throw here originally? Don't we skip over parameters/outputs that we haven't defined in the collection?
There was a problem hiding this comment.
There is a weird behavior we have in that we try to populate unmatched parameters with unmatched values. We have #2252 open to investigate and potentially remove this.
There was a problem hiding this comment.
I modified the test to also have out param in case we someday remove that behavior
There was a problem hiding this comment.
Yes I wrote about the behavior in https://github.com/npgsql/npgsql/pull/5645/files#diff-9508a102a198d47c59b1dc9bd1da180a57e6a47d800e6b9639609a27539e4980R305-R311
de69a96 to
5d1a004
Compare
Fixes #5820