year 0 in time.Time is valid but always throws an error

[gkdr]

Issue description

after upgrade from v1.5.0 to v.1.6.0, a time.Time representing an actual time without a date cannot be inserted into a column of type TIME any longer, as it has year 0 set. funnily, this value was previously even retrieved from the DB. year 0 is also in general not an invalid time.Time value for real dates.

from https://golang.org/pkg/time/#Parse :

Elements omitted from the value are assumed to be zero or, when zero is impossible, one, so parsing "3:04pm" returns the time corresponding to Jan 1, year 0, 15:04:00 UTC (note that because the year is 0, this time is before the zero Time). Years must be in the range 0000..9999.

the following recently added line throws an error when the year is 0:

mysql/utils.go

Line 285 in 417641a

return buf, errors.New("year is not in the range [1, 9999]: " + strconv.Itoa(year)) // use errors.New instead of fmt.Errorf to avoid year escape to heap

as far as i can see, appendDateTime(), the function which contains this line, is called twice, and both times when the given time.Time does not have its zero value as determined by IsZero(). however, the zero value for a time.Time is in year 1, see https://golang.org/pkg/time/#Time.IsZero :

IsZero reports whether t represents the zero time instant, January 1, year 1, 00:00:00 UTC.

what is the reason the range was picked to be [1, 9999] here?

if you can confirm this is actually an issue, i'm willing to try and fix it 🙂

Example code

package main

import (
	"database/sql"
	"fmt"
	"time"

	_ "github.com/go-sql-driver/mysql"
)

func main() {
	db, err := sql.Open("mysql", "user:pw@instance:3306/db")
	if err != nil {
		panic(err)
	}

	if _, err := db.Exec("CREATE TEMPORARY TABLE IF NOT EXISTS `time_test` (`id` int not null, `some_time` time not null);"); err != nil {
		panic(err)
	}

	if _, err := db.Exec("insert into `time_test` values(1, '08:30:30');"); err != nil {
		panic(err)
	}

	var someTime string
	row := db.QueryRow("select some_time from `time_test` where id = 1;")
	if err := row.Scan(&someTime); err != nil {
		panic(err)
	}

	fmt.Printf("this worked: %v\n", someTime)

	mysqlTimeFormat := "15:04:05"
	parsedTime, err := time.Parse(mysqlTimeFormat, someTime)
	if err != nil {
		panic(err)
	}

	// this works in 1.5.0, panics in 1.6.0
	if _, err := db.Exec("update `time_test` set some_time = ? where id = 1;", parsedTime); err != nil {
		panic(err)
	}

	fmt.Println("success!")
}

Error log

the error i receive is the error from the line i specified above:
panic: year is not in the range [1, 9999]: 0

Configuration

Driver version (or git SHA): v1.6.0

Go version: 1.16.2

Server version: MySQL 8.0.20

Server OS: official mysql docker container, iirc its based on debian but should be irrelevant.

[proyb6]

A quick guess, is your issue related to this?
#1199

[gkdr]

hi @proyb6, thanks for the hint.
i checked it out and i don't think it's related: 08:30:30 is a valid and full value of a column with type TIME, see the documentation for the TIME type, and parsing it actually works without a problem, as the month is ignored. inserting such a value back into the DB works with driver version 1.5.0, but changes introduced in #1118 cause the error i described.

(unrelated to my issue, and related to the one you linked: since 0000-00-00 is a special case that seems to be written in case the time.Time has its zero value, maybe it would be possible to have a special case for that when parsing too, so the parseTime does not have to be deactivated?)

[zihengCat]

@gkdr
Take a look at appendDateTime unit test case. Maybe your issue is same as the case.

mysql/utils_test.go

Lines 348 to 357 in 75d09ac

	// year out of range
	{
	v := time.Date(0, 1, 1, 0, 0, 0, 0, time.UTC)
	buf := make([]byte, 0, 32)
	_, err := appendDateTime(buf, v)
	if err == nil {
	t.Error("want an error")
	return
	}
	}

[gkdr]

hi @zihengCat, yes, that is exactly it! year 0 is in fact "out of range" for a time.Time which represents a date, however if the time.Time represents just a time (e.g. 08:30:30) the year is defined to be 0. so i'm not sure check is correct, or the code needs to be more specific depending on whether the column is of SQL DATETIME or TIME type.

[zihengCat]

Well, let the maintainers @julienschmidt @methane @shogo82148 join the discussion.

Seems that it's a breaking change between v1.5.0 and v.1.6.0, but isn't described clearly in release notes.

Should the MySQL TIME type be treated specially to support zero year, month, day?

[gkdr]

i updated the description with a minimal reproducible example.
while creating it, i realized that parsing a column of type mysql TIMEinto a time.Time is actually not supported by the driver and there is some other client code handling that in the code base. nevertheless, the insert from time.Time definitely works in 1.5.0, but does not in 1.6.0, as reported.

[shogo82148]

We shouldn't treat MySQL TIME type as Go time.Time type.

In the following case, the driver converts parsedTime to "0000-01-01 08:30:30".

db.Exec("update `time_test` set some_time = ? where id = 1;", parsedTime)

However, it is not valid value for MySQL TIME type.

mysql> update `time_test` set some_time = "0000-01-01 08:30:30" where id = 1;
Query OK, 1 row affected, 1 warning (0.02 sec)
Rows matched: 1  Changed: 1  Warnings: 1

mysql> SHOW WARNINGS;
+-------+------+-----------------------------------------------------------------------------+
| Level | Code | Message                                                                     |
+-------+------+-----------------------------------------------------------------------------+
| Note  | 1292 | Incorrect time value: '0000-01-01 08:30:30' for column 'some_time' at row 1 |
+-------+------+-----------------------------------------------------------------------------+
1 row in set (0.01 sec)

And, MySQL TIME values may range from '-838:59:59' to '838:59:59'.

https://dev.mysql.com/doc/refman/8.0/en/time.html
MySQL retrieves and displays TIME values in 'hh:mm:ss' format (or 'hhh:mm:ss' format for large hours values). TIME values may range from '-838:59:59' to '838:59:59'. The hours part may be so large because the TIME type can be used not only to represent a time of day (which must be less than 24 hours), but also elapsed time or a time interval between two events (which may be much greater than 24 hours, or even negative).

time.Time can't handle large hours values, such as -838:59:59.

https://play.golang.org/p/tiJAvWu4vt2

package main

import (
	"fmt"
	"time"
)

func main() {
	mysqlTimeFormat := "15:04:05"
	fmt.Println(time.Parse(mysqlTimeFormat, "-838:59:59"))
	// Output:
	// 0001-01-01 00:00:00 +0000 UTC parsing time "-838:59:59" as "15:04:05": cannot parse "-838:59:59" as "15"
}

[zihengCat]

Time conversion codes used in db.Exec:

mysql/connection.go

Lines 244 to 254 in 75d09ac

	case time.Time:
	if v.IsZero() {
	buf = append(buf, "'0000-00-00'"...)
	} else {
	buf = append(buf, '\'')
	buf, err = appendDateTime(buf, v.In(mc.cfg.Loc))
	if err != nil {
	return "", err
	}
	buf = append(buf, '\'')
	}

It's hard to pre-detect MySQL TIME datatype in driver layer.

If top layer passes a time.Time variable (driver.Value), how could driver knows whether it's a TIME or DATETIME or TIMESTAMP?

[zihengCat]

Leave things to the top layer is much simpler, just using string.

func main() {
	someTime := time.Date(0, 0, 0, 8, 30, 30, 0, time.UTC)
	parsedTime := timeParse(someTime) // "08:30:30"

	if _, err := db.Exec("update `time_test` set some_time = ? where id = 1;", parsedTime); err != nil {
		panic(err)
	}
}

func timeParse(t time.Time) string {
	var buf bytes.Buffer

	h, m, s := t.Clock()
	if h < 10 {
		buf.WriteString("0")
	}
	buf.WriteString(strconv.Itoa(h))
	buf.WriteString(":")

	if m < 10 {
		buf.WriteString("0")
	}
	buf.WriteString(strconv.Itoa(m))
	buf.WriteString(":")

	if s < 10 {
		buf.WriteString("0")
	}
	buf.WriteString(strconv.Itoa(s))

	return buf.String()
}

[gkdr]

@shogo82148 thank you for looking at it the issue. i'm of course fine with you saying that throwing that error is an intended fix. it's a breaking change though so in that case it should be noted somewhere. i must admit i didn't check the mysql warnings, it's indeed not fully correct in 1.5.0 and it just happened to be working.

We shouldn't treat MySQL TIME type as Go time.Time type.

sorry for the confusion. my issue is only about the writing of the time.Time type, which you already do (in the snippet @zihengCat provided). i think it would be easy to fix the warning you mentioned in 1.5.0, and the error thrown in 1.6.0, by handling the year 0 differently. as the time package documentation says, year 0 just means that no date is set. i think it is 100% correct to write out the hh:mm:ss format in that case. anything else is a user error.

about parsing (which, again, the issue isn't about): you are right that it would not be correct to generally use time.Time when
parsing a TIME column, since it could also be a timespan. that would be more fitting of the time.Duration type. if you want to support it with the parseTime option, maybe it would be possible to check which type the user supplied? the hh:mm:ss format could be either time.Time or time.Duration, but the -hhh:mm:ss format could only be a time.Duration, with an error thrown if the wrong type is supplied. what do you think of that?

@zihengCat

• the driver already assumes a time.Time is a DATETIME/TIMESTAMP. the format for those types is the same, so it doesn't really matter. the permitted values for these types are different, but i think it's okay to just pass that on to the DB.
• if the year is 0, time.Time by definition does not contain a date, see the docs i linked in the issue description. this 100% corresponds to the "time of day" format of the TIME type, hh:mm:ss. at least i don't see how such a time.Time could mean anything else 😄 if the user creates a time.Time like you do in your example and does not mean that it doesn't contain a date, it just goes against the docs of the time package!
• time.Duration could be used for the -(h)hh:mm:ss format, i.e. the "time interval" format of the TIME type. but that's just to continue my suggestion from above, i don't believe it's necessary (in this issue).

your code can be much simpler too:

someTime := time.Date(0, 0, 0, 8, 30, 30, 0, time.UTC)
mysqlTimeFormat := "15:04:05"
fmt.Println(someTime.Format(mysqlTimeFormat))

my issue isn't with getting it to work, it's that writing such a value "worked" in 1.5.0, but doesn't in 1.6.0, and i was wondering if it's fully intentional 🙂

edit: if you are not completely against it, i can open a pr with my suggestion, and we could discuss some actual code.

[zihengCat]

@gkdr

MySQL TIME type could be used to represent a time interval, the range for TIME values is -838:59:59.000000 to 838:59:59.000000.

Now, the question is, what Go data type should we use to map MySQL TIME ?

• time.Duration
• time.Time with zero year, month, day

time.Duration seems better because MySQL TIME could be negative (fits time interval definition).

However time.Duration is not a valid driver.Value...

https://golang.org/pkg/database/sql/driver/#Value

[zihengCat]

@gkdr
Another interesting discussion about time.Duration supports .
#1217

[gkdr]

@zihengCat This issue is not about how to "represent" a MySQL TIME column in Golang code. It is purely about the appendDateTime() function. In 1.5.0, it wrote out the string 0000-01-01 08:30:30 for a zero-year time.Time, which happened to be working with a warning. In 1.6.0, zero-year time.Time throws an error. My only suggestion is to correctly write the string 08:30:30 instead. In my opinion, this has nothing to do with parsing or representation of MySQL types in Golang. In this direction, i.e. time.Time to string, there are no ambiguities. Am I missing something?

(Again, this is unrelated to the issue, but time.Duration (or any type, really) could easily be supported in the same manner. The description of the driver.Value type is: Value is a value that drivers must be able to handle. It is either nil, a type handled by a database driver's NamedValueChecker interface, or an instance of one of these types: [...]. So it is entirely dependent on the driver itself which types are supported. driver.Value is an empty interface.)

[methane]

as the time package documentation says, year 0 just means that no date is set.

I don't think time package say so. It is just a time.Parse() behavior for missing values.

Is there any other formal document about "time of the day" ("time without date") in time Pacakge?
If "0000-01-01 12:34:56" is "time without date", how about "0000-02-02 12:34:56"?

[gkdr]

@methane just to make sure we are on the same page, i'm talking about this paragraph:

Elements omitted from the value are assumed to be zero or, when zero is impossible, one, so parsing "3:04pm" returns the time corresponding to Jan 1, year 0, 15:04:00 UTC (note that because the year is 0, this time is before the zero Time). Years must be in the range 0000..9999. The day of the week is checked for syntax but it is otherwise ignored.

this is quoted from the description of time.Parse(), so yes you are definitely right that this is a description of this function. what makes me think this is very intentional is this sentence: note that because the year is 0, this time is before the zero Time, i.e. it is intentionally defined so that "zero time" can be discerned from "no date set".

another point is the following quote, from the paragraph above the one already quoted:

Predefined layouts ANSIC, UnixDate, RFC3339 and others describe standard and convenient representations of the reference time. For more information about the formats and the definition of the reference time, see the documentation for ANSIC and the other constants defined by this package.

and the value of the constant time.Kitchen is "3:04PM", so it is intended that just a time without a date can be parsed. just the US format is not very helpful in this case, so we had to define our own.

as far as i can tell, there is no other way of representing just a time without a date mentioned in the package description.

about your other question:

If "0000-01-01 12:34:56" is "time without date", how about "0000-02-02 12:34:56"?

i think i kept saying the wrong thing because i abbreviated it too much. of course you are right: based on the description i quoted, it's not just about the year being 0, but the other date values being not set as well. so any time.Time with year 0, month 1, day 1 is "just a time", and therefore your second example "0000-02-02 12:34:56" is definitely a time and a date, and also not a zero date.

note that your code currently does not accept your second example, and the full range of valid years: "Years must be in the range 0000..9999". year 0 should be allowed no matter how you stand on the "time without a date" issue, and it is currently not.