Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

distinguish between schema.Time and tag time #133

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

black-06
Copy link
Contributor

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

fix go-gorm/gorm#6033

User Case Description

type Test struct {
    Date time.Time `gorm:"type:date"`
    Time time.Time `gorm:"type:time"`
    DefaultDateTime time.Time
}
CREATE TABLE `tests` (`date` date,`time` time,`default_date_time` datetime)

@black-06 black-06 marked this pull request as draft February 20, 2023 08:17
@black-06 black-06 marked this pull request as ready for review March 14, 2023 02:51
@jinzhu jinzhu merged commit cb2c532 into go-gorm:master Apr 11, 2023
@johnmai-dev
Copy link

johnmai-dev commented Apr 12, 2023

serializer_test.go:189: failed to query data, got error unsupported Scan, storing driver.Value type string into type *time.Time

image

@black-06
Copy link
Contributor Author

serializer_test.go:189: failed to query data, got error unsupported Scan, storing driver.Value type string into type *time.Time

image

I will check it...

@johnmai-dev
Copy link

serializer_test.go:189: failed to query data, got error unsupported Scan, storing driver.Value type string into type *time.Time
image

I will check it...
https://github.com/go-gorm/gorm/blob/master/tests/serializer_test.go#L142
https://github.com/go-gorm/gorm/actions/runs/4676651295/jobs/8283214295#step:5:1423

@black-06
Copy link
Contributor Author

Before v1.5.0 ,

https://github.com/go-gorm/gorm/blob/e9637024d3780dba4de755e6f5879150f43e8390/tests/serializer_test.go#L25`
in SQLite type is datetime (even if "time" is specified in the tag) because we didn't distinuish betwwen schema.Time and tag time

Now it will be time in SQLite. Of course, in theory it should also work normally.
However, due to the driver, time actually returned string type, so this test failed.
See mattn/go-sqlite3#951.

I suggest specifying type as datetime.

CreatedTime    int64    `gorm:"serializer:unixtime;type:datetime"` // store time in db, use int as field type

If you want forward compatibility, don't upgrade 1.5.0 for now.

At present, the sqlite3 has been fixed but no new version has been released.
We can release again when it's released. cc @jinzhu

@johnmai-dev
Copy link

Before v1.5.0 ,

[https://github.com/go-gorm/gorm/blob/e9637024d3780dba4de755e6f5879150f43e8390/tests/serializer_test.go#L25](https://github.com/go-gorm/gorm/blob/e9637024d3780dba4de755e6f5879150f43e8390/tests/serializer_test.go#L25%60) in SQLite type is datetime` (even if "time" is specified in the tag) because we didn't distinuish betwwen schema.Time and tag time

Now it will be time in SQLite. Of course, in theory it should also work normally. However, due to the driver, time actually returned string type, so this test failed. See mattn/go-sqlite3#951.

I suggest specifying type as datetime.

CreatedTime    int64    `gorm:"serializer:unixtime;type:datetime"` // store time in db, use int as field type

If you want forward compatibility, don't upgrade 1.5.0 for now.

At present, the sqlite3 has been fixed but no new version has been released. We can release again when it's released. cc @jinzhu

I've already tried, but then another problem came up. postgres won't support datetime .

https://github.com/go-gorm/gorm/actions/runs/4676407423/jobs/8282677859#step:6:1512
image

@black-06
Copy link
Contributor Author

umm... but why is it still 1.4.4 in go.mod but your mr uses 1.5.0...
I don't see you modifying the mod. That's strange.

Or we split this test? timestamp for postgres and datetime for others

@johnmai-dev
Copy link

umm... but why is it still 1.4.4 in go.mod but your mr uses 1.5.0... I don't see you modifying the mod. That's strange.

Or we split this test? timestamp for postgres and datetime for others

Due to the utilization of the -u parameter in the tests/tests_all.sh.

I have considered spliting it, but I don't believe it to be a viable solution.

@black-06
Copy link
Contributor Author

umm... but why is it still 1.4.4 in go.mod but your mr uses 1.5.0... I don't see you modifying the mod. That's strange.

Or we split this test? timestamp for postgres and datetime for others

Due to the utilization of the -u parameter in the tests/tests_all.sh.

I have considered spliting it, but I don't believe it to be a viable solution.

But we do want to use the type given by the user.
Or do we judge whether the user type is reasonable? 
Especially time kind.

@a631807682
Copy link
Member

It is ok for me to separate this test. When the user specifies the type through the tag, it must be guaranteed to work in the corresponding database. We do not need to check whether the type is legal, because from the current use, the user not only specifies the type through the tag type, and many other attributes (like a hack but allowed because we can't support all attributes).
PR Welcome.

@black-06 black-06 mentioned this pull request Apr 19, 2023
3 tasks
JosteinLindhom added a commit to quickfeed/quickfeed that referenced this pull request Jan 22, 2024
Having type `time` causes timestamps to be returned as strings rather than the intended `time.Time` in our serializer.
go-gorm/sqlite#133 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

type标签无法指定time类型?
4 participants