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

Normalize UTC dates in pgxtypes v5 #1945

Closed
jalandis opened this issue Mar 15, 2024 · 4 comments
Closed

Normalize UTC dates in pgxtypes v5 #1945

jalandis opened this issue Mar 15, 2024 · 4 comments

Comments

@jalandis
Copy link

jalandis commented Mar 15, 2024

Is your feature request related to a problem? Please describe.

When decoding timestamptz fields, UTC values have an unexpected location set instead of the GO standard nil value.

Test code comparing structs with reflect.DeepEqual fails on time.Time fields.

Describe the solution you'd like
This appears to have been solved already in the separate pgtypes project but didn't make it to v5.

jackc/pgtype@7544603

Changing the logic to match how GO std time works with UTC seems like a safe choice. I would be happy to create a PR with the code already in pgtypes v4.

Describe alternatives you've considered

  • Creating a separate custom type
  • Using pgx v4

Converting all assertions in the project is a large lift. It would not be feasible for our code base.

Additional context
I am using the sql/database interfaces and not the pgx interfaces.

From GO time

        // loc specifies the Location that should be used to
	// determine the minute, hour, month, day, and year
	// that correspond to this Time.
	// The nil location means UTC.
	// All UTC times are represented with loc==nil, never loc==&utcLoc.
	loc *Location
@jalandis
Copy link
Author

jalandis commented Mar 16, 2024

I found that there was a PR in testify.Equal to use time.Equal in comparisons. This was determined to be a bug as 2 times with different representations were not considered equal.

The PR's fix/bug was reverted by 1.9.0 :-(

stretchr/testify#1536

stretchr/testify#1537

jackc added a commit that referenced this issue Mar 16, 2024
If ScanLocation is set, it will be used to convert the time to the given
location when scanning from the database.

The Codec interface is now implemented by *pgtype.TimestamptzCodec
instead of pgtype.TimestamptzCodec. This is technically a breaking
change, but it is extremely unlikely that anyone is depending on this,
and if there is downstream breakage it is trivial to fix.

#1195
#1945
jackc added a commit that referenced this issue Mar 16, 2024
If ScanLocation is set, the timestamps will be assumed to be in the
given location when scanning from the database.

The Codec interface is now implemented by *pgtype.TimestampCodec instead
of pgtype.TimestampCodec. This is technically a breaking change, but it
is extremely unlikely that anyone is depending on this, and if there is
downstream breakage it is trivial to fix.

#1195
#1945
@jackc
Copy link
Owner

jackc commented Mar 16, 2024

I think that jackc/pgtype@7544603 only affects parsing a text formatted timestamptz because the time zone will be part of the string. Binary formatted values do not contain any time zone information. The time zone is somewhat arbitrary. This led to #1195.

Hmm... thinking about this issue and reviewing #1195 gives me an idea...

...
...
...

Try PR #1948. I think that this issue could be resolved by the following code in an after connect hook.

		conn.TypeMap().RegisterType(&pgtype.Type{
			Name:  "timestamptz",
			OID:   pgtype.TimestamptzOID,
			Codec: &pgtype.TimestamptzCodec{ScanLocation: time.UTC},
		})

@jalandis
Copy link
Author

That worked beautifully! All my tests are now passing.

jackc added a commit that referenced this issue May 8, 2024
If ScanLocation is set, it will be used to convert the time to the given
location when scanning from the database.

The Codec interface is now implemented by *pgtype.TimestamptzCodec
instead of pgtype.TimestamptzCodec. This is technically a breaking
change, but it is extremely unlikely that anyone is depending on this,
and if there is downstream breakage it is trivial to fix.

#1195
#1945
jackc added a commit that referenced this issue May 8, 2024
If ScanLocation is set, the timestamps will be assumed to be in the
given location when scanning from the database.

The Codec interface is now implemented by *pgtype.TimestampCodec instead
of pgtype.TimestampCodec. This is technically a breaking change, but it
is extremely unlikely that anyone is depending on this, and if there is
downstream breakage it is trivial to fix.

#1195
#1945
@jackc
Copy link
Owner

jackc commented May 8, 2024

Merged #1948.

@jackc jackc closed this as completed May 8, 2024
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

No branches or pull requests

2 participants