feat: inject _time_format=sqlite by default and normalize UTC on writes by aeneasr · Pull Request #16 · ory/pop

aeneasr · 2026-03-22T10:56:13Z

Summary

Auto-inject _time_format=sqlite in finalizerSQLite as a default (like busy_timeout and foreign_keys). Ensures modernc.org/sqlite uses the same on-disk timestamp format as mattn/go-sqlite3, giving byte-for-byte compatible storage and correct lexicographic ordering for timestamp comparisons. normalizeTimesToUTC (called after every read) handles the FixedZone("", 0) artefact this introduces for UTC values.
Normalize UTC on writes — call normalizeTimesToUTC before Create, Update, and UpdateQuery so non-UTC time.Time values are converted before being bound as query parameters, not just in the in-memory struct after the fact.
Extend test coverage — write-path UTC normalisation (Create/Update), _time_format=sqlite default injection, *Inner{*time.Time} pointer-to-struct pointer-field recursion, and DB readback assertions verifying the persisted value is UTC.

Test plan

go test -run "TestSqlite|Test_ConnectionDetails_Finalize_SQLite|Test_normalizeTimesToUTC|Test_sqlitePragmaSet" ./... passes
go test -short ./... passes across all packages
Existing _time_format=sqlite in a DSN is respected and not overwritten

🤖 Generated with Claude Code

Three related improvements to the SQLite dialect: 1. Auto-inject _time_format=sqlite in finalizerSQLite (alongside the existing busy_timeout and foreign_keys defaults). This ensures modernc.org/sqlite uses the same on-disk timestamp format as mattn/go-sqlite3 ("2006-01-02 15:04:05.999999999-07:00"), giving byte-for-byte compatible storage and correct lexicographic ordering for cursor-based pagination. normalizeTimesToUTC (called after every read) already corrects the FixedZone("", 0) artefact this format introduces for UTC values. 2. Call normalizeTimesToUTC after Create and Update, mirroring the existing behaviour on SelectOne/SelectMany. This ensures non-UTC time.Time values passed by the caller are normalised to UTC in the in-memory model after every write, preventing mixed-timezone storage that would break string-order comparisons. 3. Add regression tests: write-path UTC normalisation (Create/Update), _time_format=sqlite default injection, and coverage for *Inner{*time.Time} pointer-to-struct pointer-field recursion. Also adds docs/sqlite-time-format-findings.md documenting the mattn vs modernc on-disk format differences and the cursor-pagination impact. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR standardizes SQLite timestamp behavior to improve cross-driver (mattn vs modernc) compatibility and preserve lexicographic ordering assumptions needed for cursor-style timestamp comparisons.

Changes:

Inject _time_format=sqlite by default during SQLite connection finalization.
Attempt to normalize time.Time values to UTC on write paths (Create/Update) and extend normalizeTimesToUTC recursion handling.
Add documentation and tests covering _time_format injection and time normalization scenarios.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
docs/sqlite-time-format-findings.md	Documents driver time-format differences and their impact on lexicographic comparisons.
dialect_sqlite.go	Injects `_time_format=sqlite` by default and adds post-write `normalizeTimesToUTC` calls for Create/Update.
dialect_sqlite_test.go	Updates default-option expectations and adds tests for `_time_format` defaulting + additional UTC normalization cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-22T11:02:57Z

dialect_sqlite.go

 			if err != nil {
 				return err
 			}
+			normalizeTimesToUTC(model.Value)
 			return nil


normalizeTimesToUTC(model.Value) is called after NamedExecContext, which means it only normalizes the in-memory struct and does not affect the timestamp value that was actually written to SQLite. To ensure non-UTC times are not persisted (and to keep lexicographic ordering stable), normalize the model before executing the INSERT (and ideally before logging/binding).

Fixed. Normalization now runs before NamedExecContext so the bound parameters are UTC when written.

Copilot · 2026-03-22T11:02:57Z

dialect_sqlite.go

 		if err := genericCreate(c, model, cols, m); err != nil {
 			return fmt.Errorf("sqlite create: %w", err)
 		}
+		normalizeTimesToUTC(model.Value)


In the non-int primary key path, normalizeTimesToUTC(model.Value) runs after genericCreate, but genericCreate already executed the INSERT using the pre-normalized struct values. If the goal is to store UTC consistently, normalization needs to happen before calling genericCreate so the bound parameters are UTC when written.

Suggested change

if err := genericCreate(c, model, cols, m); err != nil {

return fmt.Errorf("sqlite create: %w", err)

}

normalizeTimesToUTC(model.Value)

normalizeTimesToUTC(model.Value)

if err := genericCreate(c, model, cols, m); err != nil {

return fmt.Errorf("sqlite create: %w", err)

}

Fixed. Normalization now runs before genericCreate on both the int and non-int paths.

Copilot · 2026-03-22T11:02:58Z

dialect_sqlite.go

 		if err := genericUpdate(c, model, cols, m); err != nil {
 			return fmt.Errorf("sqlite update: %w", err)
 		}
+		normalizeTimesToUTC(model.Value)
 		return nil


normalizeTimesToUTC(model.Value) is executed after genericUpdate, so it won’t normalize the timestamps that were bound and persisted by the UPDATE statement—only the in-memory struct. If UTC-on-write is required, normalize before calling genericUpdate (and consider aligning other write paths like UpdateQuery).

Fixed. Normalization now runs before genericUpdate.

Copilot · 2026-03-22T11:02:58Z

dialect_sqlite_test.go

+func TestSqlite_Create_NormalizesTimesToUTC(t *testing.T) {
+	c := openMemorySQLite(t)
+	require.NoError(t, c.RawQuery(
+		`CREATE TABLE time_items (id INTEGER PRIMARY KEY AUTOINCREMENT, ts DATETIME NOT NULL)`,
+	).Exec())
+
+	fixedZone := time.FixedZone("offset", 3600) // UTC+1, not UTC
+	nonUTC := time.Now().Truncate(time.Second).In(fixedZone)
+
+	item := &timeItem{Ts: nonUTC}
+	require.NoError(t, c.Create(item))
+
+	require.Equal(t, time.UTC, item.Ts.Location(), "Create must normalize times to UTC")
+	require.True(t, nonUTC.Equal(item.Ts), "instant must be preserved")
+}


These tests only assert that the Go struct field is normalized to time.UTC after Create/Save, but they don't verify what was actually stored in SQLite. To protect the intended behavior (UTC on-disk + lexicographically comparable), also assert the persisted value is UTC (e.g., read back CAST(ts AS TEXT) and ensure it encodes +00:00 / matches the expected format).

Added DB readback assertions in both Create and Update tests: c.Find reads the stored row and checks stored.Ts.Location() == time.UTC and that the instant is preserved.

Copilot · 2026-03-22T11:02:58Z

docs/sqlite-time-format-findings.md

+## How pop is affected
+
+Pop passes timestamp cursor values to `WHERE` predicates as `time.Time` (via
+keyset pagination), not as pre-formatted strings. The driver then serializes the
+`time.Time` when binding the query parameter.


This section states that pop uses keyset pagination and passes timestamp cursors as time.Time. In this repo, the built-in paginator is offset-based, so this reads as misleading pop-specific behavior. Consider rephrasing to describe the general case (applications binding time.Time parameters) or explicitly note this is about external keyset pagination usage rather than pop's built-in pagination.

Suggested change

## How pop is affected

Pop passes timestamp cursor values to `WHERE` predicates as `time.Time` (via

keyset pagination), not as pre-formatted strings. The driver then serializes the

`time.Time` when binding the query parameter.

## How applications using `time.Time` parameters (including Pop) are affected

In typical usage, application code (for example, services built with Pop) often

passes timestamp cursor values to `WHERE` predicates as `time.Time` parameters

(for example, in application-level keyset/cursor pagination), not as

pre-formatted strings. The driver then serializes the `time.Time` when binding

the query parameter.

Copilot · 2026-03-22T11:02:58Z

docs/sqlite-time-format-findings.md

+All findings below were verified with cross-driver compat tests
+(`test/sqlite-compat/compat_test.go`).


The doc references test/sqlite-compat/compat_test.go as the location of the verification tests, but that path doesn't exist in this repository. Please update the reference to the actual test location(s) in this repo, or clarify that the compat tests live elsewhere.

Suggested change

All findings below were verified with cross-driver compat tests

(`test/sqlite-compat/compat_test.go`).

All findings below were verified with cross-driver SQLite compatibility tests

(maintained in a dedicated SQLite compat test suite outside this repository).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

normalizeTimesToUTC was called post-write so non-UTC times were being persisted to SQLite while only the in-memory struct was fixed. Move normalization before NamedExecContext/genericCreate/genericUpdate so UTC values are bound as query parameters. Also extend the Create/Update tests to read back the stored row and verify the persisted value is UTC, not just the in-memory struct. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

UpdateQuery binds model.Value via sqlx.Named before executing the UPDATE, so it had the same bug as Create/Update: non-UTC times were persisted without normalization. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

aeneasr requested a review from a team as a code owner March 22, 2026 10:56

Copilot AI review requested due to automatic review settings March 22, 2026 10:56

Copilot started reviewing on behalf of aeneasr March 22, 2026 10:56 View session

Copilot AI reviewed Mar 22, 2026

View reviewed changes

aeneasr and others added 3 commits March 22, 2026 12:34

chore: remove AI slop from sqlite-compat changes

b45d3eb

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix: normalize timestamps before UpdateQuery write path

6f1c4fc

UpdateQuery binds model.Value via sqlx.Named before executing the UPDATE, so it had the same bug as Create/Update: non-UTC times were persisted without normalization. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

aeneasr merged commit d0243dc into main Mar 22, 2026
8 checks passed

aeneasr deleted the sqlite-compat branch March 22, 2026 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: inject _time_format=sqlite by default and normalize UTC on writes#16

feat: inject _time_format=sqlite by default and normalize UTC on writes#16
aeneasr merged 4 commits intomainfrom
sqlite-compat

aeneasr commented Mar 22, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 22, 2026

Uh oh!

aeneasr Mar 22, 2026

Uh oh!

Copilot AI Mar 22, 2026

Uh oh!

aeneasr Mar 22, 2026

Uh oh!

Copilot AI Mar 22, 2026

Uh oh!

aeneasr Mar 22, 2026

Uh oh!

Copilot AI Mar 22, 2026

Uh oh!

aeneasr Mar 22, 2026

Uh oh!

Copilot AI Mar 22, 2026

Uh oh!

Copilot AI Mar 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		All findings below were verified with cross-driver compat tests
		(`test/sqlite-compat/compat_test.go`).

Conversation

aeneasr commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

aeneasr Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

aeneasr Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

aeneasr Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

aeneasr Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aeneasr commented Mar 22, 2026 •

edited

Loading