Skip to content
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

Open
odeke-em opened this issue Apr 21, 2021 · 13 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@odeke-em
Copy link
Member

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:

package main

import (
	"database/sql"
)

func main() {
	f32 := &sql.NullInt32{}
	err := f32.Scan([]byte("p10004"))
	if f32.Valid && err != nil {
		panic("inconsistency, .Valid yet err != nil")
	}
}

What did you expect to see?

No panic

What did you see instead?

panic: inconsistency, .Valid yet err != nil

goroutine 1 [running]:
main.main()
	/tmp/sandbox708621220/prog.go:11 +0xd6

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

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

        err := convertAssign(&n.Time, value)
        n.Valid = err == nil
        return err
@odeke-em odeke-em added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 21, 2021
@a8m
Copy link
Contributor

a8m commented Apr 21, 2021

@odeke-em, following the doc, the Valid flag is true if the scanned value is not NULL.
If we fix this, we should update the doc as well.

@odeke-em
Copy link
Member Author

@odeke-em, following the doc, the Valid flag is true if the scanned value is NULL.
If we fix this, we should update the doc as well.

@a8m, I don't follow what you mean, where does it say Valid flag is true if the scanned value is NULL? I actually see the opposite that Valid is true if the scanned value is not NULL per
Screen Shot 2021-04-21 at 11 56 29 AM
Screen Shot 2021-04-21 at 11 56 38 AM
Screen Shot 2021-04-21 at 11 56 48 AM
Screen Shot 2021-04-21 at 11 57 01 AM
Screen Shot 2021-04-21 at 11 57 11 AM
Screen Shot 2021-04-21 at 11 57 21 AM

@a8m
Copy link
Contributor

a8m commented Apr 21, 2021

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 Valid flag?

@odeke-em
Copy link
Member Author

odeke-em commented Apr 21, 2021 via email

@a8m
Copy link
Contributor

a8m commented Apr 21, 2021

OK, so does it make sense to change also the godoc for the Valid field?
Do you want me to take this?

@odeke-em
Copy link
Member Author

I shall send a CL for this, thank you for the volunteering though :-)

@odeke-em
Copy link
Member Author

Kindly cc-ing @kardianos and @bradfitz too

@ulikunitz
Copy link
Contributor

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.

@odeke-em
Copy link
Member Author

@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.

@ulikunitz
Copy link
Contributor

@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.

@odeke-em
Copy link
Member Author

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.

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.

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.

I don't see how passing []byte("this is not an int") into (*NullInt64).Scan on creating .Valid=true and then an error ensures, and we accept that as "undefined" behavior and allow people to incorrectly accept .Valid=true.
We scanned data from a value, that value was clearly incorrect, but the internal state was changed. That is simply an oversight that sure took a keen eye and 9 years to notice, but nonetheless is a bug. Thus I don't see why/how this could be a proposal.
The Go compatibility promise only holds for valid Go programs, if someone was using .Valid in the event of an error, that program is incorrect and unfortunately the result was pushed by us. It is much worse for a user to have relied on just the state of Null.Valid=true and then passed in zero values after they ignored the error :-( If we let users ignore errors, at least we shouldn't falsely tell them that the parsing resulted in a valid value.

@ulikunitz
Copy link
Contributor

ulikunitz commented Apr 23, 2021

I wonder what you think about this program: https://play.golang.org/p/SPAzdQay-YG

Scan(nil) returns no error but the field Valid is false. It is the documented way to set the value to NULL.

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 NotNull.

We have now three behaviors that the method Scan might have when an error is returned.

  1. The receiver can have an arbitrary value.
  2. The receiver is not modified. (What I wrongly assumed you intended.)
  3. The receiver is NULL, requiring the field Valid to be false.

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. x.Scan(""), ignoring the error.

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.

@kardianos
Copy link
Contributor

@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
I'm not happy with that CL.

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants