Timestamp incorrectly adds 'Z' when serializing from JSON to indicate GMT, fixes bug #2215

[pconstantinou]

Fixes #2215

[pconstantinou]

@s-montigny-desautels any chance you could review this?

[jackc]

Now that I'm looking at this functionality a little closer, it seems to me that both MarshalJSON and UnmarshalJSON are wrong. Why is time.RFC3339Nano being used at all? A timestamp doesn't have a time zone.

I would expect pgtype.Timestamp's JSON behavior to match PostgreSQL.

postgres@[local]:5015 pgx_test=# select to_json(now()::timestamp);
           to_json
──────────────────────────────
 "2025-01-03T18:57:40.099807"
(1 row)

[pconstantinou]

Now that I'm looking at this functionality a little closer, it seems to me that both MarshalJSON and UnmarshalJSON are wrong. Why is time.RFC3339Nano being used at all? A timestamp doesn't have a time zone.

I would expect pgtype.Timestamp's JSON behavior to match PostgreSQL.

postgres@[local]:5015 pgx_test=# select to_json(now()::timestamp);
           to_json
──────────────────────────────
 "2025-01-03T18:57:40.099807"
(1 row)

It looks like RFC3339 is endorsed by ISO 8601. - https://www.rfc-editor.org/rfc/rfc3339
The RFC allows a timezone to be present but doesn't require it

	RFC3339     = "2006-01-02T15:04:05Z07:00"
	RFC3339Nano = "2006-01-02T15:04:05.999999999Z07:00"

The issue I encountered was that the library added the "Z" even if it was there. I'd be happy if the whole change in 7803ec3 was rolled back.

Probably the other issue is that Go doesn't have a time const for a non-timezone version.

[s-montigny-desautels]

The fix I did was to match the postgres JSON implementation, by adding the missing "Z" to the returned string. As @jackc said, timestamp don't have timezone, but time.RFC3339Nano requires it. That's why I've added the missing "Z", assuming the timezone is UTC and user could change it afterwards. I didn't want to change too much code, but maybe a custom format should be used instead of the time.RFC3339Nano.

I don't think reverting 7803ec3 is an good option either, since it will break UnmarshalJSON when getting JSON timestamp from postgres.

@pconstantinou Where are you getting your timezone from? Maybe you are not using the proper type, since your timestamp has a timezone information in it.

[pconstantinou]

@pconstantinou Where are you getting your timezone from? Maybe you are not using the proper type, since your timestamp has a timezone information in it.

	var pgt pgtype.Timestamp
	if err := pgt.Scan(t.UTC()); err != nil {
		fmt.Printf("Failed to populate pgtype.Timestamp %v", err)
	}
	return pgt

See: #2215 with a succinct example here: https://goplay.tools/snippet/fO_ceIqGn7d

[s-montigny-desautels]

I see. Then, maybe, we should modify the MarshalJSON function instead? I don't think it should add the timezone information, since timestamp don't have one. What do you think?

[pconstantinou]

I see. Then, maybe, we should modify the MarshalJSON function instead? I don't think it should add the timezone information, since timestamp don't have one. What do you think?

I've made MarshalJSON write out standard ISO 8601 JSON formatted timestamp with only the date and time (no timezone). The internal representation of time.Time seems to try to standardize on UTC which could a little problematic but I'm not sure that there's a better option.
Modifying MarshalJSON should make it clear that the Timestamp object does not include a Timezone.

As a consequence, this change does modify the expected results in the unit tests. The old version of the unit tests expected a timezone, I've removed that.

When Unmarshalling, I've changed the implementation to start parsing using with RFC3339Nano and if that is unsuccessful then parse with 8601 setting the timezone to UTC. This is cleaner than just appending an Z which will do the wrong thing in several situations. This also makes the implementation in line with the JSON spec.

For my particular usecase, I've persisted JSON generated from the older version of the library, the appending Z broke backwards compatibility. Fixing that remains important to me and it seems like an absolute requirement for any fix.

[pconstantinou]

@jackc can you take another look? I'd love to get this merged or decide on a different approach.

[jackc]

I don't understand this change.

[pconstantinou]

The ! assert.Equal(...) function evaluates to the same as string(r) != tt.result but will also output a standard error message from testify showing clearly the expected and actual value.

[jackc]

So if I understand correctly, this change makes marshalling and unmarshalling match PostgreSQL? If so then this should be fine.

[pconstantinou]

So if I understand correctly, this change makes marshaling and unmarshalling match PostgreSQL? If so then this should be fine.

Yes. It will match PostgresSQL marshaling and unmarshalling, by default excluding the timezone.

Additionally, if the data was marshaled using an older version of the pgx library (which used to include the timezone) it will continue to work without error.