go-jet / jet

Type safe SQL builder with code generation and automatic query result data mapping
Apache License 2.0
2.23k stars 110 forks source link

ErrNoRows not returned on INSERT ON CONFLICT DO UPDATE WHERE... #264

Closed darylhjd closed 4 months ago

darylhjd commented 10 months ago

Describe the bug ErrNoRows error is not returned when update is skipped during an upsert with PostgreSQL.

Code snippet Using the following query:

Classes.INSERT(
        Classes.Code,
        Classes.Year,
        Classes.Semester,
        Classes.Programme,
    ).MODELS(
        inserts,
    ).ON_CONFLICT().ON_CONSTRAINT(
        "ux_code_year_semester",
    ).DO_UPDATE(
        SET(
            Classes.Programme.SET(Classes.EXCLUDED.Programme),
        ).WHERE(
            Classes.Programme.NOT_EQ(Classes.EXCLUDED.Programme)
        ),
    ).RETURNING(
        Classes.AllColumns,
    )

where classes has the following schema:

CREATE TABLE classes
(
    id         BIGSERIAL PRIMARY KEY,
    code       TEXT        NOT NULL,
    year       INTEGER     NOT NULL,
    semester   TEXT        NOT NULL,
    programme  TEXT        NOT NULL,
    CONSTRAINT ux_code_year_semester
        UNIQUE (code, year, semester)
);

When a row with the same code, year, semester, and programme fields is going to be inserted:

  1. ON CONFLICT will attempt to DO UPDATE
  2. DO UPDATE will not set the new field because the programme field has the same value according to WHERE clause.
  3. RETURNING returns no rows (destination is nil).

However, when doing QueryContext on the query, no ErrNoRows error is returned even though there is no result.

Expected behavior Since the update is skipped and no rows are returned from the resulting query, I expect ErrNoRows to be returned but this is not the case. Not sure if this is expected or a bug.

houten11 commented 10 months ago

https://github.com/go-jet/jet/blob/33333585e9571ee4375c1a6ee99acff129c801ba/internal/jet/statement.go#L21-L24 Only if the destination is a pointer to a struct ErrNoRows is returned.
If the destination is a slice, you can check if there were any updates, by checking the slice length:

if len(dest) == 0 {
   // no updates
}
darylhjd commented 10 months ago

Thanks for the reply!

Is this a deliberate design choice for compatibility with multiple database implementations (Postgres, MySQL, etc...)? Because I personally think the behaviour of errors should be type-agnostic for more predictable behaviour.

go-jet commented 10 months ago

Yeah, it's a mistake made at the begging and has remained until now. There is a plan to deprecate the current Query and QueryContext and create two new ones, QueryScanOne and QueryScanMany, but there is always some higher priority features. Maybe we can squeeze it into the next release.