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: add configuration option to ignore null values in Scan #57066

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MisterSquishy
Copy link

@MisterSquishy MisterSquishy commented Dec 3, 2022

Fixes #28414

Currently, when scanning a NULL database value into a go type that is not nil-able, this package throws an error like "sql: Scan error on column index 0, name "foo": converting NULL to int is unsupported". This makes logical sense; nil and 0 are different values, and only the latter can be assigned to an int. The Null* types (NullString, NullInt, etc) account for this difference, giving programmers the ability to safely bridge the gap between nullable SQL values and primitive go types.

I propose we give programmers the option to change that behavior and instead ignore NULL values. There are a few reasons I think this option makes sense:
Alignment with programmer intent

In go, if I want a struct field with a string field, I would write type foo struct{ bar string }
In mysql, if I want a table with a string column, I would write CREATE TABLE foo (bar varchar(255));

But this produces incompatible types -- in mysql, any value can be NULL unless you explicitly say otherwise. If we wanted to enforce that our foo.bar column could never be NULL, we would have to write CREATE TABLE foo (bar varchar(255) NOT NULL); This nuance is seldom clear to programmers, and is a lesson that generally only gets learned the hard way. For instance, if you google "go sql null", you will get back numerous pages of blog posts, stack overflow posts, etc -- whether or not it is "right", the undeniable fact is that this behavior subverts programmer expectations on a regular basis.

Furthermore, when a programmer writes some go code to fetch sql data and work with it in a go program, choosing to type their variable as a non-nilable type is a signal that they do not need to write special handling for the null case. It isn't up to us to determine whether or not they should; by writing their code this way, the programmer has indicated that they have no use for this distinction. When we force them to care (at runtime, often long after the code has been written, when we unexpectedly encounter a NULL value in the database), this produces broken software that isn't always straightforward to triage/remediate.

Fundamentally, the responsibility of this package is to bridge the gap between sql and go. Accounting for the disparate treatment of NULL in these two languages is currently the responsibility of the programmer, but by reconciling this difference in the translation layer, we can prevent bugs and make it easier to write great go code.
Consistency with json unmarshaling behaviors

As shown in this playground, unmarshaling json with NULL values into non-nilable types already works according to the programmer expectations described above -- we leave these values untouched. Whether or not the json unmarshaler is "right" is up for debate (see #33835), but the fact of the matter is that this will likely have to remain the default behavior forever. The fact that these two standard libraries have opposite opinions is unfortunate, and can probably never be changed. However, we compound this issue by giving programmers no mechanism to make them behave the same way. So, every go programmer has to be aware of this distinction and correctly design their types for the environment(s) that they expect to instantiate them from. This is clunky at best, and in practice it creates vast swaths of bug habitat.

It would be great to introduce an analogous configuration option in the standard json unmarshaler (or perhaps even reuse the same configuration flag), but that is out of scope for this issue.

For the above reasons, it's important to give programmers the ability to opt out of this behavior. Using Null* types or pointers to primitives remain viable for situations where we need to distinguish between NULL and other values, but in situations where the programmer does not care about this distinction, we should allow them this freedom.

A NOTE ON ignoreNullValues
As currently written, ignoreNullValues cannot be configured by the programmer, so this PR as written doesn't actually change any behavior. The intent is to expose this as a configuration option (note that we must leave it off by default so as not to break existing code). But, I wasn't entirely sure of the most standard way to do so -- guidance here would be much appreciated.

@gopherbot
Copy link
Contributor

This PR (HEAD: 9d07d4b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/455035 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.


Please don’t reply on this GitHub thread. Visit golang.org/cl/455035.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 957d11a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/455035 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@MisterSquishy MisterSquishy force-pushed the coalesce branch 2 times, most recently from acf148d to 7501086 Compare December 3, 2022 22:14
@gopherbot
Copy link
Contributor

This PR (HEAD: 7501086) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/455035 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

This PR (HEAD: 8d5819b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/455035 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Daniel Theophanes:

Patch Set 4:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/455035.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Pete Davids:

Patch Set 4:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/455035.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Daniel Theophanes:

Patch Set 4:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/455035.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Pete Davids:

Patch Set 4:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/455035.
After addressing review feedback, remember to publish your drafts!

@MisterSquishy MisterSquishy changed the title database/sql: create coalesceNullValuesToZero option database/sql: add configuration option to ignore null values in Scan Dec 6, 2022
@gopherbot
Copy link
Contributor

This PR (HEAD: b316dbd) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/455035 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proposal: database/sql: Return zero time.Time for nil values instead of error
2 participants