Skip to content
This repository has been archived by the owner on Sep 7, 2021. It is now read-only.
This repository is currently being migrated. It's locked while the migration is in progress.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nanyan
Copy link

@Nanyan Nanyan commented Dec 4, 2018

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

…value, then return nil to use db default value
@codecov-io
Copy link

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.

1 similar comment
@codecov-io
Copy link

codecov-io commented Dec 4, 2018

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.

@lunny
Copy link
Member

lunny commented Dec 5, 2018

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

@Nanyan
Copy link
Author

Nanyan commented Dec 6, 2018

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

@lunny
Copy link
Member

lunny commented Dec 8, 2018

@Nanyan
Copy link
Author

Nanyan commented Dec 13, 2018

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?

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

Successfully merging this pull request may close these issues.

3 participants