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.

Sign up for GitHub

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

Jump to bottom

timezone bug when persisting *gtime.Time to db #1714

Closed
wesleywu opened this issue Mar 27, 2022 · 5 comments
Closed

timezone bug when persisting *gtime.Time to db #1714

wesleywu opened this issue Mar 27, 2022 · 5 comments
Assignees
@gqcn
Labels
done This issue is done, which may be release in next version. enhancement

Comments

@wesleywu
Copy link
Contributor

wesleywu commented Mar 27, 2022 •
edited
Loading

1. What version of Go and system type/arch are you using?

go1.17.6 darwin/amd64

2. What version of GoFrame are you using?

v2.0.4

3. Can this issue be re-produced with the latest release?

yes

4. What did you do?

  1. database table
create table test_time (
  id int unsigned auto_increment,
  my_timestamp TIMESTAMP,
  my_time DATETIME,
  primary key(id)
);

  1. write a simple service "TestTime" to save record to db

  2. DSN
    mysql:user:pass@tcp(127.0.0.1:3306)/db?charset=utf8mb4&parseTime=true&loc=Local

  3. Use *time.Time for column type, and write a test as below:

func Test_insertDate(t *testing.T) {
	var time *time.Time
	time = &gtime.NewFromStr("2022-03-27T01:03:04Z").Time
	fmt.Println(time.String())
	service.TestTime.Add(gctx.New(), &model2.TestTimeAddReq{
		MyTimestamp: time,
		MyTime:      time,
	})

	time = &gtime.NewFromStr("2022-03-27 01:03:04").Time
	fmt.Println(time.String())
	service.TestTime.Add(gctx.New(), &model2.TestTimeAddReq{
		MyTimestamp: time,
		MyTime:      time,
	})

	time = &gtime.NewFromStr("2022-03-27T01:03:04+08:00").Time
	fmt.Println(time.String())
	service.TestTime.Add(gctx.New(), &model2.TestTimeAddReq{
		MyTimestamp: time,
		MyTime:      time,
	})

	time = &gtime.NewFromStr("2022-03-27T01:03:04+01:00").Time
	fmt.Println(time.String())
	service.TestTime.Add(gctx.New(), &model2.TestTimeAddReq{
		MyTimestamp: time,
		MyTime:      time,
	})

	time = &gtime.NewFromStr("2022-03-27T01:03:04+00:00").Time
	fmt.Println(time.String())
	service.TestTime.Add(gctx.New(), &model2.TestTimeAddReq{
		MyTimestamp: time,
		MyTime:      time,
	})
}

console output:

2022-03-27 01:03:04 +0000 UTC
2022-03-27 01:03:04 +0800 CST
2022-03-27 01:03:04 +0800 CST
2022-03-27 00:03:04 +0000 UTC
2022-03-27 01:03:04 +0000 UTC

database records:

1	2022-03-27 09:03:04	2022-03-27 09:03:04
2	2022-03-27 01:03:04	2022-03-27 01:03:04
3	2022-03-27 01:03:04	2022-03-27 01:03:04
4	2022-03-27 08:03:04	2022-03-27 08:03:04
5	2022-03-27 09:03:04	2022-03-27 09:03:04

above all are expected results.

  1. Use *gtime.Time for column type, and write another test as below:
func Test_insertDate1(t *testing.T) {
	var time *gtime.Time
	time = gtime.NewFromStr("2022-03-27T01:03:04Z")
	fmt.Println(time.String())
	service.TestTime.Add(gctx.New(), &model2.TestTimeAddReq{
		MyTimestamp: time,
		MyTime:      time,
	})

	time = gtime.NewFromStr("2022-03-27 01:03:04")
	fmt.Println(time.String())
	service.TestTime.Add(gctx.New(), &model2.TestTimeAddReq{
		MyTimestamp: time,
		MyTime:      time,
	})

	time = gtime.NewFromStr("2022-03-27T01:03:04+08:00")
	fmt.Println(time.String())
	service.TestTime.Add(gctx.New(), &model2.TestTimeAddReq{
		MyTimestamp: time,
		MyTime:      time,
	})

	time = gtime.NewFromStr("2022-03-27T01:03:04+01:00")
	fmt.Println(time.String())
	service.TestTime.Add(gctx.New(), &model2.TestTimeAddReq{
		MyTimestamp: time,
		MyTime:      time,
	})

	time = gtime.NewFromStr("2022-03-27T01:03:04+00:00")
	fmt.Println(time.String())
	service.TestTime.Add(gctx.New(), &model2.TestTimeAddReq{
		MyTimestamp: time,
		MyTime:      time,
	})
}

console output:

2022-03-27 01:03:04  // not expected, should be UTC
2022-03-27 01:03:04
2022-03-27 01:03:04
2022-03-27 00:03:04  // not expected, should be UTC
2022-03-27 01:03:04  // not expected, should be UTC

database record:

1	2022-03-27 01:03:04	2022-03-27 01:03:04  // not expected
2	2022-03-27 01:03:04	2022-03-27 01:03:04
3	2022-03-27 01:03:04	2022-03-27 01:03:04
4	2022-03-27 00:03:04	2022-03-27 00:03:04  // not expected
5	2022-03-27 01:03:04	2022-03-27 01:03:04  // not expected

I think the behavior may related to bug in gtime.Time.String().
GF might have used the string representation of a gtime.Time before persisting to db.

Another bug is:
When I use timestamp(6) or datetime(6) as sql column type, gtime.Time will truncate my milliseconds from my time field.
This may also related to the gtime.Time.String() bug.
@wesleywu
Copy link
Contributor Author

wesleywu commented Apr 3, 2022 •
edited
Loading

a more comprehensive test:

// create table test_time (
// id int unsigned auto_increment,
// my_gtime TIMESTAMP,
// my_time TIMESTAMP,
// my_time_ptr TIMESTAMP,
// primary key(id)
//);
func Test_insertDate(t *testing.T) {
	var (
		ctx              = gctx.New()
		parseRfc3339Time = func(t string) time.Time { // Rfc3339 是符合 ISO8601 定义的 Internet标准时间的实现
			res, _ := time.Parse(time.RFC3339, t)
			return res
		}
		myGtime = []*gtime.Time{
			gtime.NewFromStr("2022-03-27T01:03:04Z"),      // 2022-03-27 01:03:04 UTC 世界标准时间
			gtime.NewFromStr("2022-03-27T01:03:04+00:00"), // 2022-03-27 01:03:04 UTC 世界标准时间
			gtime.NewFromStr("2022-03-27T01:03:04+01:00"), // 2022-03-27 01:03:04 +0100 (东1区)
			gtime.NewFromStr("2022-03-27 01:03:04"),       // 2022-03-27 01:03:04 本地时间，目前是 CST +08:00
			gtime.NewFromStr("2022-03-27T01:03:04+08:00"), // 2022-03-27 01:03:04 东8区 +08:00
		}
		myTime = []time.Time{
			parseRfc3339Time("2022-03-27T01:03:04Z"),       // 2022-03-27 01:03:04 +0000 UTC (世界标准时间)
			parseRfc3339Time("2022-03-27T01:03:04+00:00"),  // 2022-03-27 01:03:04 +0000 UTC (世界标准时间)
			parseRfc3339Time("2022-03-27T01:03:04+01:00"),  // 2022-03-27 01:03:04 +0100 +0100 (东1区)
			time.Date(2022, 3, 27, 1, 3, 4, 0, time.Local), // 2022-03-27 01:03:04 +0800 CST
			parseRfc3339Time("2022-03-27T01:03:04+08:00"),  // 2022-03-27 01:03:04 +0800 CST
		}
	)

	for i := 0; i < 5; i++ {
		g.Log().Debugf(ctx, "gtime=%s time=%s", myGtime[i].String(), myTime[i].String())
		_, _ = g.DB().Insert(ctx, "test_time", g.Map{
			"my_gtime":    myGtime[i],
			"my_time":     myTime[i],
			"my_time_ptr": &myTime[i],
		})
	}

	// mysql database link: "mysql:root:root@tcp(127.0.0.1:3306)/focus"

	// mysql db timezone:
	// system_time_zone = CST
	// time_zone = +08:00

	// result:
	// 1	2022-03-27 01:03:04	2022-03-27 01:03:04	2022-03-27 01:03:04
	// 2	2022-03-27 01:03:04	2022-03-27 01:03:04	2022-03-27 01:03:04
	// 3	2022-03-27 00:03:04	2022-03-27 00:03:04	2022-03-27 00:03:04
	// 4	2022-03-27 01:03:04	2022-03-26 17:03:04	2022-03-26 17:03:04
	// 5	2022-03-27 01:03:04	2022-03-26 17:03:04	2022-03-26 17:03:04

	// expected result:
	// 1	2022-03-27 09:03:04	2022-03-27 09:03:04	2022-03-27 09:03:04
	// 2	2022-03-27 09:03:04	2022-03-27 09:03:04	2022-03-27 09:03:04
	// 3	2022-03-27 08:03:04	2022-03-27 08:03:04	2022-03-27 08:03:04
	// 4	2022-03-27 01:03:04	2022-03-27 01:03:04	2022-03-27 01:03:04
	// 5	2022-03-27 01:03:04	2022-03-27 01:03:04	2022-03-27 01:03:04

	// if I changed my link to: "mysql:root:root@tcp(127.0.0.1:3306)/focus?parseTime=true&loc=Local"

	// result:
	// 1	2022-03-27 01:03:04	2022-03-27 09:03:04	2022-03-27 09:03:04
	// 2	2022-03-27 01:03:04	2022-03-27 09:03:04	2022-03-27 09:03:04
	// 3	2022-03-27 00:03:04	2022-03-27 08:03:04	2022-03-27 08:03:04
	// 4	2022-03-27 01:03:04	2022-03-27 01:03:04	2022-03-27 01:03:04
	// 5	2022-03-27 01:03:04	2022-03-27 01:03:04	2022-03-27 01:03:04
	
	// time.Time columns have expected results, but gtime.Time columns not
}

@wesleywu
Copy link
Contributor Author

wesleywu commented Apr 3, 2022 •
edited
Loading

Mr. Guo suggested me to check the point of gtime.Time variable conversion before the variable committed to underlying driver in package gdb. Finally I got spare time to follow his suggestion today.

I traced into gdb package, Go native sql package and mysql driver package.
I believe there are no conversions happened for time.Time throughout the whole insertion process from gdb.Insert to mysql.writeExecutePacket (in mysql driver responsible to send binary bytes to mysql server using mysql protocol), until mysql driver properly converted time.Time column value to binary bytes.
WechatIMG719

And there is only conversion happened for gtime.Time at gtime_time_wrapper.go:
return t.Format("2006-01-02 15:04:05")
being called at gdb.formatWhereKeyValue(), gdb_func.go line 786.
WechatIMG720

I'm trying to open a PR to resolve this issue.

@gqcn gqcn added the wip label Apr 12, 2022
@gqcn gqcn self-assigned this Apr 12, 2022
@gqcn
Copy link
Member

gqcn commented Apr 12, 2022

@wesleywu Thanks, I will carefully review this issue and PR again these days.

@wesleywu
Copy link
Contributor Author

@gqcn Thanks for reconsider this issue.

The purpose of functions like String() or Sprintf() is to output informations (can be abbreviated or truncated information) to users. These functions should not be used at any circumstances of data persisting, because they may cause loss of information and they were designed to not be responsible for the loss.

I could accept the fact that gtime.time.String() lose timezone info and there are many workarounds like call Local() before String().
But I couldn't agree to using gtime.time.String() as default conversion method through db persisting process, giving developers no chance to work around.

gqcn added a commit that referenced this issue May 9, 2022
@gqcn
Merge pull request #1729 from WesleyWu/fix#1714
e81d6a8 
fix timezone bug when persisting *gtime.Time to db #1714
gqcn added a commit that referenced this issue May 9, 2022
@gqcn
fix issue #1714, #1727
3db97ba
@gqcn gqcn added enhancement done This issue is done, which may be release in next version. and removed wip labels May 9, 2022
@gqcn
Copy link
Member

gqcn commented May 9, 2022

@wesleywu Thanks for your PR, it is already merged into master branch.

@gqcn gqcn closed this as completed May 9, 2022
houseme pushed a commit to houseme/gf that referenced this issue May 11, 2022
@houseme
fix timezone bug when persisting *gtime.Time to db gogf#1714
489698d
houseme pushed a commit to houseme/gf that referenced this issue May 11, 2022
@gqcn @houseme
fix issue gogf#1714, gogf#1727
7b2f449
@wln32 wln32 mentioned this issue May 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gqcn gqcn

Labels
done This issue is done, which may be release in next version. enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@wesleywu @gqcn