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

Open
Nanyan wants to merge 1 commits from Nanyan/master into master
Nanyan commented 3 years ago (Migrated from github.com)
Owner

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

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 3 years ago (Migrated from github.com)
Owner

Codecov Report

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

Impacted file tree graph

@@           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).
Owner

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

@Nanyan nice work! could you add some unit tests?
Nanyan commented 3 years ago (Migrated from github.com)
Poster
Owner

@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?
Owner

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 3 years ago (Migrated from github.com)
Poster
Owner

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 2 years ago
lunny modified the milestone from 1.0.0 to 1.1.1 2 years ago
lunny added the
kind/breaking
label 4 months ago
lunny modified the milestone from 1.1.1 to 1.2.0 4 months ago
lunny added 1 commit 2 months ago
lunny modified the milestone from 1.2.0 to 1.2.1 2 months ago
lunny modified the milestone from 1.2.1 to 1.2.2 2 months ago
lunny modified the milestone from 1.2.2 to 1.2.3 1 month ago
lunny modified the milestone from 1.2.3 to 1.3.0 1 month ago
All checks were successful
continuous-integration/drone/pr Build is passing
Required
Details
This pull request has changes conflicting with the target branch.
engine.go
Sign in to join this conversation.
Loading…
There is no content yet.