Set a nil date to NULL rather than 0000-00-00

[Freeaqingme]

I've got a row defined: "date_disconnect" datetime DEFAULT NULL

I'm also running mysql in strictmode:

> select @@global.sql_mode\G
*************************** 1. row ***************************
@@global.sql_mode: REAL_AS_FLOAT,PIPES_AS_CONCAT,ANSI_QUOTES,IGNORE_SPACE,ANSI,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION

I set these modes because if I want to indicate the absence of a data, I like to use NULL, rather than set an "arbitrary" date 2015 years ago. In line with ANSI SQL.

The DSN used: root:@tcp(localhost:3306)/cluegetter?sql_notes=false&strict=true

Please let me know what you think of this PR. I'll get some tests if you agree on the chosen approach.

[Freeaqingme]

@julienschmidt Do you have any considerations about this change?

[AlekSi]

👍

[julienschmidt]

I'm not entirely convinced. There is not really a "nil date" since time.Time is a number. Therefore it's default value is 0, not nil. There is also a semantic difference between interface{}(nil) and interface{}(t) with t being an uninitialized time.Time value.

On the other hand, 0000-00-00 makes absolutely no sense in strict mode. Therefore this approach is feasible.

@arnehormann @xaprb
Yea or Nay?

[AlekSi]

There is not really a "nil date" since time.Time is a number.

*time.Time can be nil. Right now there is no way to store it in database.

[julienschmidt]

*time.Time can be nil. Right now there is no way to store it in database.

But *time.Time is not a valid database/sql Value. Is it converted to an uninitialized time.Time value?

[AlekSi]

Nor *string, *bool, etc., but they work across most drivers. Probably due to different interpretations of

It is either nil or an instance of one of these types

Specifically, my internal code works with *time.Time, *string and *bool with github.com/mattn/go-sqlite3, github.com/lib/pq and mostly with this package – except *time.Time.

[julienschmidt]

database/sql converts all these values to the values the driver must be able to handle:
http://golang.org/src/database/sql/convert.go#L35

We don't know which of the values the user originally passed. So we don't know when you passed *time.Time. Looking at http://golang.org/src/database/sql/driver/types.go#L234, I think it is converted to nil.

[arnehormann]

I really dislike all the type handling magic. Especially when it comes in form of dsn parameters - and this could easily become yet another one.
How about dropping that stuff alltogether? We could add a mapping function to sanitize inputs as a var - like the logger... per default doing what the driver does right now with variants for the current dsn-stuff.

[julienschmidt]

I really dislike all the type handling magic.

I especially hate the magic the database/sql package does

[arnehormann]

I really dislike all the type handling magic.

I especially hate the magic the database/sql package does

Me too... I hate that it's needed at all.

[Freeaqingme]

@julienschmidt This issue has been open for a while now, perhaps we can do something about it?

[dveeden]

Maybe fix the tests so it can be merged?
I do think 0/nil → NULL makes a lot more sense than 0/nil → 0000-00-00.
A mapping function (w/ default == current behavior) would be a good and flexible solution. This would add a lot of flexibility to it.

[Freeaqingme]

I just spent 1-2 hours (longer than the time spent on the change itself) on trying to comprehend how the TestDateTime() stuff works. During that time I wasn't able to come up with a way that would fail without my change, and pass with my change. The TestDateTime tries to cover (imho) too many things at once, and it's taking me too long to come up with something sensible.

If you have any ideas how to test this sensibly I'd be happy to hear it. If not, I'll close this pull request.

[ghufftesla]

I am running into this exact issue with strict set on in mysql 5.7.10 using gorm with time.Time fields in structs. It seems very reasonable to me that a time.Time{} (uninitialized/zero value) for a NULLable database datetime column should be inserted as NULL (not '0000-00-00'). Is there any reason not to accept this PR? With strict=true the existing code simply doesn't work in 5.7 (zero dates are not allowed).

[methane]

0000-00-00 is not accepted by default from MySQL 5.7.
So I'm +1 for NULL rather than 0000-00-00.

[julienschmidt]

This PR needs an update then

[tab1293]

+1 for using nil as well. Gorm with zero date 0000-00-00 causes issues with MySQL 5.7.

@Freeaqingme thoughts on updating?

[Freeaqingme]

@tab1293 Sorry, right now I've got different priorities. Though, please feel free to check out my branch and do whatever needs to be done.

[methane]

I tried but it's difficult than I thought. Currently, this driver support passing time.Time for TIME column.
It means "0000-00-00 12:34:56" is valid for TIME column, but "0000-00-00 00:00:00" is not valid.

And strict mode is deprecated for now.
How about closing this pull request for now and create new issue for discussion?

[julienschmidt]

This should be fixed soon, but the chances that we do something wrong that breaks things is high.
Thus, I'd suggest to postpone any changes until directly after the v1.4.0 release. Then we have enough time before the next release to gather feedback.

[dolmen]

Related: #783

[wgpshashank]

Any update on this guys ?

[methane]

There are no valid solution.
Use just nil, don't use zero value of time.Time.

[wgpshashank]

You can not assign null value to time.Time variable

[methane]

You can pass nil to DB.Exec() and DB.Query().
No need to assign nil to time.Time variable.

[methane]

I tried but it's difficult than I thought. Currently, this driver support passing time.Time for TIME column.
It means "0000-00-00 12:34:56" is valid for TIME column, but "0000-00-00 00:00:00" is not valid.

Now I think we should drop support passing time.Time argument for TIME column.
When scanning, we don't support scan TIME column into time.Time.

At some point, civil.Time may be supported.

[vacoo]

use gorm.Expr("NULL")

[kolkov]

What new about this problem?

[methane]

No plan to convert Zero Time to NULL. You can use NullTime instead.

I suggest closing this issue because the merit of this change is not large enough
to break backward compatibility. "00:00:00" is a valid value for time column.
And driver doesn't know the type of the column. So we can not use nil for date and datetime
column but "00:00:00" for time column.

When we want to set NULL, we can use nil instead of zero Time.

[methane]

There is a chance to do this, but there are no timeline for it because of the upstream issue.

1. Go adds more types for date and time: proposal: time: add civil time package golang/go#19700
2. We support them, especially for TIME columns.
3. Deprecate using Time value for TIME columns.
4. Do the change.

It may take more than one year. Don't wait this issue is fixed. Use NullTime or nil instead.

[bidyashish]

sql.NullTime{Valid: false}

It work for me to set SQL timestamp to null