New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
database/sql: the Null* types naively/prematurely set .Valid=true before converting and scanning data #45662
Comments
@odeke-em, following the doc, the |
@a8m, I don't follow what you mean, where does it say |
Hey @odeke-em, hehe, yeah my bad. I meant NOT NULL. Now the question is if the value is NOT NULL, but it's invalid (like your case above), what should be the value of the |
Not a problem :-) If invalid data then Valid should be false, which is the
reason for this issue :-)
…On Wed, Apr 21, 2021 at 1:06 PM Ariel Mashraki ***@***.***> wrote:
Hey @odeke-em <https://github.com/odeke-em>, hehe, yeah my bad. I meant
NOT NULL.
Now the question is if the value is NOT NULL, but it's invalid (like your
case above), what should be the value of the Valid flag?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45662 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFL3V72MXT6J3T5TH4P3YTTJ4V43ANCNFSM43JWPP6A>
.
|
OK, so does it make sense to change also the godoc for the |
I shall send a CL for this, thank you for the volunteering though :-) |
Kindly cc-ing @kardianos and @bradfitz too |
This is not a bug. The documentation says nothing about what value Scan has to provide if it returns an error and the Valid field makes no statement about the result of the Scan function. It only says that the Int32 field contains a valid integer, which is 0 here. But since Scan returned an error, the value should be ignored anyway. |
@ulikunitz thanks for chiming in! A logical inconsistency if not acknowledged nor contradicted by the documentation doesn't mean that is not an inconsistency. The point of this issue is that .Valid should not be set to true callously, we should fix it and ensure that an error returned doesn't change the state of the indicator of validity. |
@odeke-em Thank you for your response. The behavior is not inconsistent with the documentation and also not with the fact that an error is returned. It may be that it is inconsistent with an expectation that one might have regarding the behavior of Scan when it returns an error. But this is not a bug. You may argue that the value of Scan after the return of the function is undefined behavior and you want to define that the value is not changed when Scan returns. This makes this issue a proposal. But I see very little benefit for such a proposal with the risk of breaking programs that depend on the current behavior. |
If I may reiterate, the documentation is oblivious to what the code does, not that the code is inconsistent with the documentation. The documentation isn't tight hence a vast sea of unknowns, whereas the code has a limited state of unknowns like logical bugs and in this case .Valid=true being prematurely set.
I don't see how passing |
I wonder what you think about this program: https://play.golang.org/p/SPAzdQay-YG
So you cannot assume that Valid is true after Scan is successful. So why must it be false if Scan is unsuccessful? The field Valid makes no statement about the result of Scan. It only determines whether the value is NULL or not NULL. A better name for the field might have been We have now three behaviors that the method Scan might have when an error is returned.
Since nothing is said in the documentation about the value of the receiver after Scan returns with an error, behavior 1 has to be assumed. It would be better if the documentation of the Scanner interface would state that explicitly. Behavior 2 and 3 make additional guarantees and would need to be documented and would change the current behavior. If behavior 3 is chosen, there would be multiple ways to set x NULL: e.g. The Go compatibility guarantee allows the change of undefined behavior and programs relying on it are allowed to be broken. But as I stated I see little benefit in doing this change. I suggest describing behavior 1 explicitly in the documentation of the Scanner interface. |
@odeke-em I just saw this issue (though I see I was tagged above, sorry). I agree with @odeke-em : just because an error is returned, does NOT mean (in all cases) the other value should be discarded (see io.Writer that can return both the amount of bytes correctly written AND and error). Given that the Null types have a field name of "Valid" rather then "Null", I'm inclined to agree with @odeke-em on the issue. However, I'm not clear if the backwards compatibility guarentee would allow us to change this behavior. Perhaps we should document this behavior. I'm exploring what a generic nullable would look like: https://go-review.googlesource.com/c/go/+/366294 |
What version of Go are you using (
go version
)?Go tip f53c2fa
Does this issue reproduce with the latest release?
Yes!
What operating system and processor architecture are you using (
go env
)?N/A
What did you do?
https://play.golang.org/p/WPwq58LV2ym or inlined below:
What did you expect to see?
No panic
What did you see instead?
I noticed these just now while reviewing @a8m's CL for NullByte and NullInt64 with the pattern that the already existing code naively/prematurely sets .Valid=true before converting and assigning values per
go/src/database/sql/sql.go
Lines 199 to 200 in f53c2fa
go/src/database/sql/sql.go
Lines 225 to 226 in f53c2fa
go/src/database/sql/sql.go
Lines 251 to 252 in f53c2fa
go/src/database/sql/sql.go
Lines 277 to 278 in f53c2fa
go/src/database/sql/sql.go
Lines 303 to 304 in f53c2fa
go/src/database/sql/sql.go
Lines 329 to 330 in f53c2fa
The argument that folks should be checking the error always doesn't correctly stand because
Null*.Scan(value) error
is a method on an already created Null* type's instance, and that Null could be reused and accidentally cause inconsistencies.That code can all be fixed to the following pattern
The text was updated successfully, but these errors were encountered: