sqlc-dev / sqlc

Generate type-safe code from SQL
https://sqlc.dev
MIT License
12.28k stars 778 forks source link

`timestamp with time zone` not recognized as `timestamptz` when overridden with `time.Time` #2914

Open dandee opened 10 months ago

dandee commented 10 months ago

Version

1.23.0

What happened?

A SQL type timestamp with time zone is not recognized as timestamptz when timestamptz is overridden with time.Time.

as mentioned by @andrewmbenton here: https://github.com/sqlc-dev/sqlc/issues/2630#issuecomment-1729891052

Relevant log output

///////////////////////////////////////
// models.go

// Code generated by sqlc. DO NOT EDIT.
// versions:
//   sqlc v1.23.0

package db

import (
    "time"

    "github.com/jackc/pgx/v5/pgtype"
)

type User struct {
    ID         int32
    FirstLogin *time.Time
    LastLogin  pgtype.Timestamptz // BUG: not recognized, so not overridden as *time.Time
}

///////////////////////////////////////
// query.sql.go

// Code generated by sqlc. DO NOT EDIT.
// versions:
//   sqlc v1.23.0
// source: query.sql

package db

import (
    "context"

    "github.com/jackc/pgx/v5/pgtype"
)

const updateLoginTime = `-- name: UpdateLoginTime :one
UPDATE users
  SET first_login = CASE WHEN first_login IS NULL THEN $2 END,
  last_login = $2
WHERE id = $1
RETURNING id, first_login, last_login
`

// BUG:
// should read: func (q *Queries) UpdateLoginTime(ctx context.Context, iD int32, lastLogin *time.Time) 
func (q *Queries) UpdateLoginTime(ctx context.Context, iD int32, lastLogin pgtype.Timestamptz) (User, error) {
    row := q.db.QueryRow(ctx, updateLoginTime, iD, lastLogin)
    var i User
    err := row.Scan(&i.ID, &i.FirstLogin, &i.LastLogin)
    return i, err
}

Database schema

CREATE TABLE IF NOT EXISTS users
(
    id SERIAL PRIMARY KEY,
    first_login timestamptz,
    last_login timestamp with time zone
);

SQL queries

-- name: UpdateLoginTime :one
UPDATE users
  SET first_login = CASE WHEN first_login IS NULL THEN $2 END,
  last_login = $2
WHERE id = $1
RETURNING *;

Configuration

version: "2"
sql:
- engine: "postgresql"
  queries: "."
  schema: "."
  gen:
    go:
      sql_package: "pgx/v5"
      package: "db"
      out: "db"
      emit_pointers_for_null_types: true
      query_parameter_limit: 5
      overrides:
      - db_type: "timestamptz"
        go_type:
          import: "time"
          type: "Time"
          pointer: true
        nullable: true
      # Test...
      - db_type: "timestamp with time zone"
        go_type:
          import: "time"
          type: "Time"
          pointer: true
        nullable: true

Playground URL

https://play.sqlc.dev/p/d1b3dd0e8105ed7efdbc421c5cf2c6e0896d6ed57c7a17b4eec27b0ce28ca1b1

What operating system are you using?

macOS

What database engines are you using?

PostgreSQL

What type of code are you generating?

Go

andrewmbenton commented 10 months ago

Thanks for not losing track of this. From https://www.postgresql.org/docs/current/datatype-datetime.html: "timestamptz is accepted as an abbreviation for timestamp with time zone; this is a PostgreSQL extension."

With the new database analyzer this is still an issue (note that I simplified the original query in your playground link to work around the fact that it doesn't PREPARE ...): https://play.sqlc.dev/p/d1bda5ac81677118390d371f287e5dde62ecdf30ee049352fb3597f34fa015cb

dandee commented 10 months ago

You're going to get the same error with the new database analyzer when you replace timestamp with time zone with timestamptz:

https://play.sqlc.dev/p/0ba4f8fe56f9a93c9b3298a11e68595ec3d35c084e8179e3387594b5a1d0a5f9

I tried the new analyzer yesterday and got a false positive in another query. I don't think it's ready for production use yet.

andrewmbenton commented 10 months ago

Yes the issue is not with sqlc's analysis step. I was just trying out the new analyzer to confirm.

The new analyzer will have bugs for sure. The error in your last playground link ("column "first_login" is of type timestamp with time zone but expression is of type text") comes from the fact that the query fails to PREPARE ... against a PostgreSQL database. If you are successfully running that query somewhere that would be interesting to know and I'd say it's worth opening a new issue, since it would imply that there's a problem with the assumptions the new analyzer makes about how PostgreSQL handles queries in PREPARE ... statements.

dandee commented 10 months ago

All queries in my examples are working without any issues in my project's Postgres database. I even try to run them with EXPLAIN ANALYZE and there are no issues indicated.

rpstw commented 3 months ago

We upgraded pgx from v4 without using any override. A lot of our legacy code in our repo have SomeColumn time.Time. It seems currently if sql_package is set to pgx/v5, there is no way to keep the original mapping.

https://github.com/sqlc-dev/sqlc/blob/e623dc13679d0d0d7ebdf47ec6a30e22dd7e5a08/internal/codegen/golang/postgresql_type.go#L236C8-L236C28

I'm not sure if keeping the original mapping is possible while using pgx v5. If so, please allow the override, otherwise we have to do a major refactor while upgrading to pgx v5.

owen1002 commented 4 days ago

Actually is there a reason why sqlc is generating pgtype by default?

For instance generating pgtype.Timestamp for create/update params is really not a good idea imo. Say we have a field updated_at, we’ll have to create pgtype.Timestamp ourselves just for the generated update function. Which doesn’t really make sense when we should simple pass time.Time to the update function.

To fix this, we have to apply override on pg_catalog.timestamp, so it’s using time.Time instead of pgtype.Timestamp.

Same issue for uuid, where we can’t just use the default generated code and need to apply override.