fix: when time is zero, let it use db default value if possible #1164

lunny · 2018-12-04T10:27:52Z

Nanyan commented

2018-12-04 10:27:52 +00:00

(Migrated from github.com)

fix: for time.Time field, when time is zero and db field has default value, then return nil to use db default value

codecov-io commented

2018-12-04 11:19:17 +00:00

(Migrated from github.com)

Codecov Report

Merging #1164 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1164   +/-   ##
=======================================
  Coverage   54.69%   54.69%           
=======================================
  Files          42       42           
  Lines        7646     7646           
=======================================
  Hits         4182     4182           
  Misses       2935     2935           
  Partials      529      529

Impacted Files	Coverage Δ
engine.go	`61.59% <100%> (ø)`	⬆️
xorm.go	`64.61% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b07c406...75e384d. Read the comment docs.

# [Codecov](https://codecov.io/gh/go-xorm/xorm/pull/1164?src=pr&el=h1) Report > Merging [#1164](https://codecov.io/gh/go-xorm/xorm/pull/1164?src=pr&el=desc) into [master](https://codecov.io/gh/go-xorm/xorm/commit/b07c4067034594d1a43e314b10153ff9cfc58f86?src=pr&el=desc) will **not change** coverage. > The diff coverage is `100%`. [![Impacted file tree graph](https://codecov.io/gh/go-xorm/xorm/pull/1164/graphs/tree.svg?width=650&token=yB5nO1krEe&height=150&src=pr)](https://codecov.io/gh/go-xorm/xorm/pull/1164?src=pr&el=tree) ```diff @@ Coverage Diff @@ ## master #1164 +/- ## ======================================= Coverage 54.69% 54.69% ======================================= Files 42 42 Lines 7646 7646 ======================================= Hits 4182 4182 Misses 2935 2935 Partials 529 529 ``` | [Impacted Files](https://codecov.io/gh/go-xorm/xorm/pull/1164?src=pr&el=tree) | Coverage Δ | | |---|---|---| | [engine.go](https://codecov.io/gh/go-xorm/xorm/pull/1164/diff?src=pr&el=tree#diff-ZW5naW5lLmdv) | `61.59% <100%> (ø)` | :arrow_up: | | [xorm.go](https://codecov.io/gh/go-xorm/xorm/pull/1164/diff?src=pr&el=tree#diff-eG9ybS5nbw==) | `64.61% <0%> (ø)` | :arrow_up: | ------ [Continue to review full report at Codecov](https://codecov.io/gh/go-xorm/xorm/pull/1164?src=pr&el=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/go-xorm/xorm/pull/1164?src=pr&el=footer). Last update [b07c406...75e384d](https://codecov.io/gh/go-xorm/xorm/pull/1164?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

lunny commented

2018-12-05 07:58:05 +00:00

@Nanyan nice work! could you add some unit tests?

Nanyan commented

2018-12-06 04:46:50 +00:00

(Migrated from github.com)

@lunny Can you show some unit tests currently exist for the function formatColTime or for its caller function?

@lunny Can you show some unit tests currently exist for the function _formatColTime_ or for its caller function?

lunny commented

2018-12-08 06:55:03 +00:00

You can follow f16ce722ec/time_test.go as examples.

You can follow https://github.com/go-xorm/xorm/blob/f16ce722ec150ade2c2e5a08a6fed64706c79465/time_test.go as examples.

Nanyan commented

2018-12-13 11:21:39 +00:00

(Migrated from github.com)

emm..., I'm a little bit sorry about this, now I realize this is quite complicated when considering different database.
When I have a field such as create_time timestamp default CURRENT_TIMESTAMP not null in MySQL Data table, then the tool xorm reverse will generate a field in struct as: CreateTime time.Time xorm:"not null default 'CURRENT_TIMESTAMP' TIMESTAMP" .
Before I commit this fix, when I leave CreateTime as default, then in db the create_time field will be as 0001-01-01 00:00:00, So I fix this problem. It works for MySQL database!
Now I realize that, it works ok for MySQL with the INSERT sql of "INSERT INTO tbname (..., create_time) VALUES (..., null);"; BUT at least it's NOT work for Sqlite3 (which is what the testEngine used).
That is, if the time field IsZero, then it's ok for formatColTime to return nil if the col has default value for MySQL, but is not ok for Sqlite3 (and I don't known what about the others database).

As above, I thought whether we should just omit the time field if it's zero and the table's field has default setting?

emm..., I'm a little bit sorry about this, now I realize this is quite complicated when considering different database. When I have a field such as `create_time timestamp default CURRENT_TIMESTAMP not null` in MySQL Data table, then the tool `xorm reverse` will generate a field in struct as: CreateTime time.Time `xorm:"not null default 'CURRENT_TIMESTAMP' TIMESTAMP"` . Before I commit this fix, when I leave CreateTime as default, then in db the create_time field will be as `0001-01-01 00:00:00`, So I fix this problem. It works for MySQL database! Now I realize that, it works ok for MySQL with the INSERT sql of "INSERT INTO tbname (..., create_time) VALUES (..., null);"; BUT at least it's NOT work for Sqlite3 (which is what the testEngine used). That is, if the time field IsZero, then it's ok for formatColTime to return nil if the col has default value for MySQL, but is not ok for Sqlite3 (and I don't known what about the others database). As above, I thought **whether we should just omit the time field if it's zero and the table's field has default setting?**

lunny added this to the 1.0.0 milestone 2020-02-24 06:45:11 +00:00

lunny modified the milestone from 1.0.0 to 1.1.1

2020-03-01 13:25:21 +00:00

lunny added the

kind

breaking

label 2021-06-07 08:26:24 +00:00

lunny modified the milestone from 1.1.1 to 1.2.0

2021-06-08 12:23:52 +00:00

lunny added 1 commit 2021-07-20 15:38:20 +00:00

fix: for time.Time field, when time is zero and db field has default value, then return nil to use db default value

continuous-integration/drone/pr Build is passing