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: document allowed Scan conversions #9157

Closed
dadkins opened this issue Nov 24, 2014 · 6 comments
Closed

database/sql: document allowed Scan conversions #9157

dadkins opened this issue Nov 24, 2014 · 6 comments

Comments

@dadkins
Copy link

@dadkins dadkins commented Nov 24, 2014

What does 'go version' print?

go version go1.3.3 darwin/amd64

What steps reproduce the problem?
If possible, include a link to a program on play.golang.org.

http://play.golang.org/p/mk7wJKtz4H

What happened?

NullString.Scan didn't accept a time.Time value:

error: unsupported driver -> Scan pair: time.Time -> *string

What should have happened instead?

It should have converted the time.Time value into a string, just it does with any other
type that Scan accepts.

Please provide any additional information below.

The contract for Scanner says:

        // Scan assigns a value from a database driver.
        //
        // The src value will be of one of the following restricted
        // set of types:
        //
        //    int64
        //    float64
        //    bool
        //    []byte
        //    string
        //    time.Time
        //    nil - for NULL values
        //
        // An error should be returned if the value can not be stored
        // without loss of information.
        Scan(src interface{}) error

All of the other types work by formatting the values as strings. Given that time.Time
has a String() function, I would assume it would be handled as well.
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 24, 2014

Comment 1:

Labels changed: added release-go1.5, repo-main.

@dadkins dadkins added new labels Nov 24, 2014
@johto
Copy link
Contributor

@johto johto commented Dec 14, 2014

On the other hand, the comment for Scan() in your paste says: An error should be returned if the value can not be stored without loss of information. Time.String() throws away quite a lot of information about the time zone and location: http://play.golang.org/p/3QqXgTwwZ1

@bradfitz bradfitz modified the milestone: Go1.5 Dec 16, 2014
@rsc rsc removed the repo-main label Apr 14, 2015
@syst3mw0rm
Copy link
Contributor

@syst3mw0rm syst3mw0rm commented May 10, 2015

As mentioned by @johto, there will be loss of information while transforming time.Time to string. I think it is best to remove time.Time from the restricted set of src types Scan(src interface{}) operates on.

Thoughts?

@rsc
Copy link
Contributor

@rsc rsc commented Jul 14, 2015

database/sql is really very light on docs.
It should be clearer about what works and what does not.
There should be a section about Scan conversions,
and that section can explain what the rules are.

That said, I don't think we should change NullString.
It is what it is, for better or worse.
Note that it is fairly straightforward to implement
a custom NullStringOrTime that picks off the
"value has type time.Time" case and handles it as
desired and then delegates to NullString for the rest.

@rsc rsc changed the title database/sql: fails to convert time.Time to NullString database/sql: document allowed Scan conversions Jul 14, 2015
@rsc rsc modified the milestones: Go1.6, Go1.5 Jul 14, 2015
@dadkins
Copy link
Author

@dadkins dadkins commented Aug 27, 2015

It is easy to work around the problem, but it is also surprising (to me). The reason is that error says
error: unsupported driver -> Scan pair: time.Time -> *string
instead of something like
error: sql/driver: couldn't convert 5 into type bool
which is what you get run into a truly lossy conversion.

At the very least, the current behavior should be documented. But I don't think this behavior is correct and think we should fix.

It seems like a cop-out to use the loss of information rule as an excuse for not converting time.Time to a string. You can certainly encode a time, including its time zone, as a string without losing any information.

I would make the argument that time.Time should be converted to string because all of the other types mentioned in the Scanner interface are successfully converted into string. It is surprising that time.Time is not. It feels like an oversight, not an intentional decision.

In fact, are there are any Scanners in the standard library which handle time.Time?

The reason I bring this up is in the first place is that the only way to read a timestamp from a database using Scan is to know a columns contains a timestamp and provide a time.Time (or NullTime in libpq). Otherwise, it's straightforward to dump a table's contents into strings if you don't care. Instead, that's an error and you have to workaround the problem for times only. It's not like timestamps are a corner case of database either. They're very prevalent.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 26, 2016

CL https://golang.org/cl/18935 mentions this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.