pop should save created_at and updated_at fields in UTC

[sbinet]

it seems to me as pop.Connection.Create should create time.Time fields in UTC time instead of local time.

pop.Connection.Create uses the pop.Model.touch{Created,Updated}At methods to fill these time.Time values, which themselves use time.Now().

according to time.Now documentation, the time.Time value being returned is in local time:

func Now() Time
    Now returns the current local time.

that's fine, but then, when reading back that value from the db, we read it back as if stored in UTC.

So, we should probably modify pop to always generate time.Time values in UTC:

diff --git a/model.go b/model.go
index b1f9db6..96a0802 100644
--- a/model.go
+++ b/model.go
@@ -140,14 +140,14 @@ func (m *Model) setID(i interface{}) {
 func (m *Model) touchCreatedAt() {
        fbn, err := m.fieldByName("CreatedAt")
        if err == nil {
-               fbn.Set(reflect.ValueOf(time.Now()))
+               fbn.Set(reflect.ValueOf(time.Now().UTC()))
        }
 }
 
 func (m *Model) touchUpdatedAt() {
        fbn, err := m.fieldByName("UpdatedAt")
        if err == nil {
-               fbn.Set(reflect.ValueOf(time.Now()))
+               fbn.Set(reflect.ValueOf(time.Now().UTC()))
        }
 }

[markbates]

I disagree. This could be strange behavior for people. Imagine you purposefully have your database running in EST, then you start storing times in a different zone. Traditionally you set your database to run in UTC, or the TZ you want it to be.

[glb]

Related: fizz.CREATED_COL and fizz.UPDATED_COL are of type timestamp instead of timestamp with timezone, which could cause surprises for some folks -- see this note in the postgres docs, for example.

[sbinet]

I could probably live with timestamp with timezone then :)
(or a way to configure it)

[markbates]

Open a PR. :) for either solution would be great.

[sbinet]

done: #25.
only the default part: I am not sure about the correct wiring from, say, buffalo db migrate up down to fizz.CREATED_COL...

alternatively: one could allow for users to define created_at and updated_at columns in their migrations/xxxx_create_blabla.up.fizz scripts (and take those definitions for these columns if they are of type timestamp, error out otherwise.)

[sbinet]

what about letting this configuration option be done at the user models level:

package models

type User struct {
	ID              uuid.UUID    `json:"id" db:"id"`
	CreatedAt       time.Time    `json:"created_at" db:"created_at,utc"`
	UpdatedAt       time.Time    `json:"updated_at" db:"updated_at,utc"`
	Username        string       `json:"username" db:"username"`
}

ie: allow the UTC-ness to be defined in the struct-tag ?

[sbinet]

@markbates WDYT ?

[petar-dambovaliev]

@sbinet it adds complication for no clear benefits. It should be saved as utc and it can be converted to the local timezone of the user, when/if needed.

[trevrosen]

@markbates I'd be in favor of some struct tag solution for specifying UTC on timestamp columns. @sbinet I'd be happy to collaborate on this if you're still interested in something along the lines of your proposed solution and Mark or $buffalo_person has blessed the approach.

[markbates]

What about: a tz tag so users can specify the time zone of their choice, default is current behavior.

…

----------- Mark Bates

On Jun 27, 2018, 8:37 PM -0400, Trevor Rosen ***@***.***>, wrote: @markbates I'd be in favor of some struct tag solution for specifying UTC on timestamp columns. @sbinet I'd be happy to collaborate on this if you're still interested in something along the lines of your proposed solution and Mark or $buffalo_person has blessed the approach. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[trevrosen]

@markbates well tbh my goal is to store all times in UTC and convert as needed upon retrieval, so I'd be in favor of something that toggles UTC for the timestamp columns.

[flyinprogrammer]

I am hoping that in providing this feedback that I'm the last person awake at 1am debugging why my duration calculations are working in my unit tests, and completely broken at runtime.

The Buffalo ecosystem is freaking awesome, and it's my intention that this feedback is taken as encouragement and not as a complaint.

---

I disagree. This could be strange behavior for people. Imagine you purposefully have your database running in EST, then you start storing times in a different zone. Traditionally you set your database to run in UTC, or the TZ you want it to be.

What about: a tz tag so users can specify the time zone of their choice, default is current behavior.

I think these points are certainly well intentioned, but let's take a look our vanilla buffalo application with postgres support.

Our buffalo applications are setting the CreatedAt and UpdatedAt fields to local time via time.Now(), which is typically Not-UTC (certainly when we're developing locally).

And then in fizz we're not using with time zone, which means unless the database and application have been started/created with the same timezone, we will be improperly storing this data.

Which really means that when we unmarshal CreatedAt and UpdatedAt back out of the database, the data comes out as the timezone of the database, which is typically UTC.

This then breaks our ability to do duration arithmetic on these dates and auditing as the return values are now off by a mystery amount. In fact, changing the timezone of the application now stands a chance to break any functionality that was relying on these fields.

---

If this case this still isn't clear, lets create a brand new buffalo application with postgres, create any type of resource, debug the timestamps, and here's what we'll see:

Debug code to throw in an action:

timeNow := time.Now()
timeNowUTC := timeNow.In(time.UTC)
timeNowLocal := timeNow.Local()
// replace c. with your struct.
createAt := c.CreatedAt
createAtUTC := createAt.In(time.UTC)
createAtLocal := createAt.Local()
fmt.Println("timeNow: ", timeNow)
fmt.Println("timeNowUTC: ", timeNowUTC)
fmt.Println("timeNowLocal: ", timeNowLocal)
fmt.Println("createAt: ", createAt)
fmt.Println("createAtUTC: ", createAtUTC)
fmt.Println("createdAtLocal:", createAtLocal)

Debug output:

timeNow:  2018-08-20 00:04:23.934610043 -0500 CDT m=+41.594083555
timeNowUTC:  2018-08-20 05:04:23.934610043 +0000 UTC
timeNowLocal:  2018-08-20 00:04:23.934610043 -0500 CDT
createAt:  2018-08-20 00:04:16.698797 +0000 +0000
createAtUTC:  2018-08-20 00:04:16.698797 +0000 UTC
createdAtLocal: 2018-08-19 19:04:16.698797 -0500 CDT

You'll see that timeNow and createAt have the same time, but their timezones are different.

---

There are a lot of ways to fix this behavior. I think we should enable pop/fizz/buffalo to simply enforce the usage of UTC everywhere, and allow things like struct tag overrides for those who play with chainsaws, but that's just me. As for backwards incompatibility, anyone who wasn't ensuring that their application and database run in the same timezone, is already broken, and I bet that's the majority of folks.

[trevrosen]

I agree with @flyinprogrammer's well-reasoned thoughts here and would also like to add that Rails' ActiveRecord, which Pop is heavily influenced by, defaults to UTC and allows a global override. This has been the case there for many years.

I continue to believe that the least intrusive way to get this behavior is to allow for users to set a struct tag like tz:utc on any given time field.

[ichord]

I set TZ="UTC" in buffalo .env file to solve the problem for now, for the time.Now() would output UTC time by setting TZ to UTC.
And I will take care of the time zone in app layer.

[seopei]

@flyinprogrammer how did you fix this?

[flyinprogrammer]

The same way @ichord does with starting the application with the TZ="UTC" environment variable.

[seopei]

@flyinprogrammer thanks!

[xihajuan2010]

I encountered one issue , that the time queried from BD is always using UTC+0 timezone, while my database is using UTC +8, after some search I found one solution:

If your database is not using the UTC +0 time zone, you can set the DSN by adding loc=Asia%2Shanghai same as you database timezone

url: "mysql://root:root@(localhost:3306)/buffalo_test_development?parseTime=true&multiStatements=true&readTimeout=3s&loc=Asia%2FShanghai"

[jsshapiro]

This is a specific example of a general problem, which is the ability to distinguish between zoned time (postgresql: timestamptz) and non-zoned time (postgresql: timestamp). Local time is semantically very troublesome. It is mainly useful for things that have to do with a wall clock, but it is the default in early SQL, which was a really dumb decision.

Some databases, such as mariadb, do not support timestamp with adequate precision. Mariadb also does not support datetime with a zone. Which is kind of awful, since every datetime type in every programming language used today supports zones.

Though the marshaling will vary by database, the right default is that all stored times should be stored and retrieved as UTC. The storage type will vary according to the database, because (e.g.) timestamp is just useless in mariadb. If the database doesn’t does not support zoned datetime, storing UTC requires “faking” a UTC time that looks like a local time at insert/update, and restoring the zone during select. Some SQL value parsers don’t get this quite right, so it may require a stored procedure.

For update and create time stamps, all values MUST rise monotonically for correct semantics. Correct semantics simply is not possible using local times because of time zone shifts and leap seconds. Note this means that the timestamp type in mariadb is darned close to useless because it lacks adequate precision.

Summary:

1. datetime should be stored as UTC.
2. if the ‘tz:local tag is present, the value should be stored as zoneless. There is provision for this in the ISO specification for local datetime strings. Personally, I would prefer introducing a new type LocalDateTime instead of the tag. It is more explicit.
3. For databases that do not support zoned times, use modified marshaling on both input and output in order to store UTC.
4. Accept that read-after-write may not return the same result, because in certain time zones a particular time value may have more than one representation. This is inherent in how zones work; it is not a bug.
5. Make all of this true for all datetime fields, not just createtime and updatetime fields.

Unfortunately, this would be a breaking change. The next best thing would be to introduce timestamptz and DateTimeTZ as supported field types and use them to signal the desired behavior.

The fact that timestamp and datetime mean very different things across databases is a really big problem semantically. Enough so that I think we should either store a time matching the semantics of the programming language library (which would always be zoned) or we should deprecate the ambiguous types. Personally, I prefer the first one, but it’s a breaking change for mariadb, so that requires some careful thought. It wouldn’t be hard to generate a “fixtimes” migration for databases that need it, but the version stamp for current migrations isn’t able to capture the dependency.

Since it doesn’t have agreed semantics across databases, I actually think timestamp should be deprecated as a storage type. If it gets set explicitly, fine. Caveat emptor.

I have the required [de]marshaling code working for MySQL/mariadb in a little package I wrote for node. I’d be happy to offer it up, but I’m not sure what the situation is in some of the other supported databases. It may need an assist from others.

[pdewilde]

The default behavior is clearly wrong.

Fizz generates created_at and updated_at with no timezone.

Pop sets the database values of those fields with local time rather than UTC. "2023-05-15T16:46:37.944294+4" for example

When querying and deserializing into a struct, pop then interprets them as UTC and makes a time.time with a UTC offset. Now you have a time.time which is wrong. "2023-05-15T16:46:37.944294Z"

At the very least this behavior deserves a giant red flashing warning in the official docs.

A new user looking at documents would likely do the following steps:
https://gist.github.com/pdewilde/360869733139538fe01442b550c4e363

It would be much better if they were warned about the timezone issue in the documentation when they were introduced to the created_at and updated_at fields.