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

Foreign key constraint is only working with Exec() method without RETURNING clause #986

Closed
glebarez opened this issue Dec 1, 2021 · 6 comments

Comments

@glebarez
Copy link

glebarez commented Dec 1, 2021

Hello, I did some research about FOREIGN KEY constraint, and It turns out that it's not enforced in following situations:

  • If Query method of DB API is used
  • If Exec method is used and query contains "RETURNING" clause

The record is not inserted in database, but NO error is produced.
I tracked this bug after noticing strange behavior in GORM.

Here I provide a reproduction code with behavior diagnosis.
I also conduct the same queries in Postgres, to show what's believed to be a correct behavior.

The diagnose output that I get:

              driver        sql.DB method       with returning diagnose
--------------------------------------------------------------------------------
              sqlite                 exec                 true RowsAffected: 0| LastInsertId: 1
              sqlite                query                 true scan value: 1
              sqlite                 exec                false Exec() error: FOREIGN KEY constraint failed
              sqlite                query                false No Rows
            postgres                 exec                 true Exec() error: ERROR: insert or update on table "child" violates foreign key constraint "fk_child_parent" (SQLSTATE 23503)
            postgres                query                 true Query() error: ERROR: insert or update on table "child" violates foreign key constraint "fk_child_parent" (SQLSTATE 23503)
            postgres                 exec                false Exec() error: ERROR: insert or update on table "child" violates foreign key constraint "fk_child_parent" (SQLSTATE 23503)
            postgres                query                false Query() error: ERROR: insert or update on table "child" violates foreign key constraint "fk_child_parent" (SQLSTATE 23503)

The code to reproduce:

package main

import (
	"fmt"
	"log"
	"strings"

	"gorm.io/driver/postgres"
	"gorm.io/driver/sqlite"
	"gorm.io/gorm"
	"gorm.io/gorm/schema"
)

var (
	fkViolationQuery = "INSERT INTO child (parent_id) VALUES (666)"
	returningClause  = "RETURNING id"

	DSNs = []struct {
		driverName string
		dsn        string
	}{
		{"sqlite", ":memory:?_foreign_keys=1"},
		{"postgres", "postgres://postgres:1234@localhost:5432/postgres"},
	}
)

// define models with foreign key and auto-generated ID
type Parent struct {
	ID uint64 `gorm:"primaryKey;autoIncrement;not null"`
}

type Child struct {
	ID       uint64 `gorm:"primaryKey;autoIncrement;not null"`
	ParentID uint64
	Parent   Parent
}

func main() {
	header()
	// loop DSNs
	for _, dsn := range DSNs {
		// run combinations of Query/Exec With/Without Returning
		for _, withReturning := range []bool{true, false} {
			// connect to database
			gormDB := connectDB(dsn.driverName, dsn.dsn)
			// run migration
			migrate(gormDB)

			// get generic interface
			db, err := gormDB.DB()
			if err != nil {
				log.Fatal()
			}

			// build query
			var query string
			if withReturning {
				query = fmt.Sprintf("%s %s", fkViolationQuery, returningClause)
			} else {
				query = fkViolationQuery
			}

			// run exec
			result, err := db.Exec(query)
			if err != nil {
				report(dsn.driverName, "exec", withReturning, fmt.Sprintf("Exec() error: %v", err))
			} else {
				if result == nil {
					report(dsn.driverName, "exec", withReturning, "result is nil")
				} else {
					var resultInfo string
					rowCount, err := result.RowsAffected()
					if err != nil {
						resultInfo += fmt.Sprintf("RowsAffected: error: %v", err)
					} else {
						resultInfo += fmt.Sprintf("RowsAffected: %v", rowCount)
					}

					lastInsertID, err := result.LastInsertId()
					if err != nil {
						resultInfo += fmt.Sprintf("| LastInsertId: error: %v", err)
					} else {
						resultInfo += fmt.Sprintf("| LastInsertId: %v", lastInsertID)
					}
					report(dsn.driverName, "exec", withReturning, resultInfo)
				}
			}

			// run query
			rows, err := db.Query(query)
			if err != nil {
				report(dsn.driverName, "query", withReturning, fmt.Sprintf("Query() error: %v", err))
			} else {
				if !rows.Next() {
					report(dsn.driverName, "query", withReturning, "No Rows")
				} else {

					var id uint64
					if err := rows.Scan(&id); err != nil {
						report(dsn.driverName, "query", withReturning, fmt.Sprintf("rows.Scan() error: %v", err))
					} else {
						report(dsn.driverName, "query", withReturning, fmt.Sprintf("scan value: %v", id))
					}

				}
			}

		}
	}
}

func report(driver, method string, returning bool, info string) {
	fmt.Printf("%20s %20s %20v %s\n", driver, method, returning, info)
}

func header() {
	fmt.Printf("%20s %20s %20s %s\n", "driver", "sql.DB method", "with returning", "diagnose")
	fmt.Println(strings.Repeat("-", 80))
}

func connectDB(driverName, dsn string) *gorm.DB {
	var (
		db  *gorm.DB
		err error
	)

	config := &gorm.Config{
		// Logger: logger.Default.LogMode(logger.Info),
		NamingStrategy: schema.NamingStrategy{
			SingularTable: true,
		},
	}

	switch driverName {
	case "postgres":
		db, err = gorm.Open(postgres.Open(dsn), config)
	case "sqlite":
		db, err = gorm.Open(sqlite.Open(dsn), config)
	default:
		log.Fatalf("unsupported driver %s", driverName)
	}

	if err != nil {
		log.Fatal(err)
	}
	return db
}

func migrate(db *gorm.DB) {
	if err := db.Migrator().DropTable(&Child{}, &Parent{}); err != nil {
		log.Fatal()
	}
	if err := db.AutoMigrate(&Parent{}, &Child{}); err != nil {
		log.Fatal()
	}
}

The docker-compose.yml to quickly spin up the postgres:

version: '2.2'
services:
    db:
        image: postgres
        ports:
            - 5432:5432
        tmpfs:
        - /var/lib/postgresql/data:rw
        environment:
            POSTGRES_PASSWORD: 1234
            PGDATA: /var/lib/postgresql/data/pgdata
        healthcheck:
            test: ["CMD-SHELL", "pg_isready -U postgres"]
            interval: 1s
            timeout: 2s
            retries: 10
@rittneje
Copy link
Collaborator

rittneje commented Dec 1, 2021

I am unable to reproduce the issue. The following test program gets back a "FOREIGN KEY constraint failed" error from the call to row.Scan.

package main

import (
	"database/sql"
	"fmt"

	_ "github.com/mattn/go-sqlite3"
)

func main() {
	c, err := sql.Open("sqlite3", "file::memory:?_foreign_keys=1")
	if err != nil {
		panic(err)
	}
	defer c.Close()

	c.SetConnMaxIdleTime(0)
	c.SetConnMaxLifetime(0)
	c.SetMaxIdleConns(1)
	c.SetMaxOpenConns(1)

	if err := c.Ping(); err != nil {
		panic(err)
	}

	if _, err := c.Exec(`CREATE TABLE Parent(id INTEGER PRIMARY KEY)`); err != nil {
		panic(err)
	}

	if _, err := c.Exec(`CREATE TABLE Child(id INTEGER PRIMARY KEY, parent_id INTEGER NOT NULL REFERENCES Parent(id))`); err != nil {
		panic(err)
	}

	row := c.QueryRow(`INSERT INTO child (parent_id) VALUES (666) RETURNING id`)
	var id uint32
	if err := row.Scan(&id); err != nil {
		panic(err)
	}

	fmt.Println(id)
}

Please share a dump of the SQLite schema that gorm is creating for you. To do so, generate your database to a file rather than in-memory, and then use the .dump command from the sqlite3 CLI.

@glebarez
Copy link
Author

glebarez commented Dec 1, 2021

@rittneje thank you for response.
I elaborated on your example, and got to the bottom of this.
Row.Scan() internally checks for errors when closing the Rows.
But if we use sql.DB.Query() and then manually scan the Rows, then we successfully scan the generated ID via Rows.Scan() without any errors, and only later do we spot an error by manually checking Rows.Err(), after Rows are closed.

It's probably "intended" for now, since SQLite console works exactly same way:

sqlite> PRAGMA foreign_keys(1);
sqlite> CREATE TABLE Parent(id INTEGER PRIMARY KEY);
sqlite> CREATE TABLE Child(id INTEGER PRIMARY KEY, parent_id INTEGER NOT NULL REFERENCES Parent(id));
sqlite> INSERT INTO child (parent_id) VALUES (666) RETURNING id;
1.         // <--- generated ID returned
Error: FOREIGN KEY constraint failed // <---- error afterwards
package main

import (
	"database/sql"
	"fmt"

	_ "github.com/mattn/go-sqlite3"
)

func main() {
	c, err := sql.Open("sqlite3", "file::memory:?_foreign_keys=1&_pragma=foreign_keys(1)")
	if err != nil {
		panic(err)
	}
	defer c.Close()

	c.SetConnMaxIdleTime(0)
	c.SetConnMaxLifetime(0)
	c.SetMaxIdleConns(1)
	c.SetMaxOpenConns(1)

	if err := c.Ping(); err != nil {
		panic(err)
	}

	if _, err := c.Exec(`CREATE TABLE Parent(id INTEGER PRIMARY KEY)`); err != nil {
		panic(err)
	}

	if _, err := c.Exec(`CREATE TABLE Child(id INTEGER PRIMARY KEY, parent_id INTEGER NOT NULL REFERENCES Parent(id))`); err != nil {
		panic(err)
	}

	rows, err := c.Query(`INSERT INTO child (parent_id) VALUES (666) RETURNING id`)
	if err != nil {
		panic((err))
	}

	var id uint32
	for rows.Next() {
		if err := rows.Scan(&id); err != nil {
			panic(err)
		}
		fmt.Printf("scanned %v\n", id) // <----- successfully scanned generated ID
	}

	if rows.Err() != nil {
		panic(rows.Err()) // <----- only at this stage can we spot foreign key violation
	}
}

@itizir
Copy link
Contributor

itizir commented Dec 1, 2021

Hi!

I won't be able to offer much insight into the details of how Postgres and SQLite prepare and check statements and when they fail, etc. to explain the initial discrepancies you pointed out, however I'd like to clarify how the golang database/sql is meant to be 'correctly' used, as there should then be no such problem:

You should use Exec for statements that do not return rows, and Query otherwise; and when you iterate through Rows returned by Query, you should always check for Rows.Err() when done with the Rows.Next() loop:

https://pkg.go.dev/database/sql#Rows.Next

Next prepares the next result row for reading with the Scan method. It returns true on success,
or false if there is no next result row or an error happened while preparing it.
Err should be consulted to distinguish between the two cases.

For statements that will return a single row, like in your examples above, you can also use QueryRow for simplicity:

err := c.QueryRow(stmt).Scan(&id)

will return the expected error.

@rittneje
Copy link
Collaborator

rittneje commented Dec 1, 2021

I did some more testing, and it seems that what is actually happening is the constraint violation isn't being detected until the underlying call to sqlite3_reset. That is why the behavior of this library is so inconsistent.

According to the SQLite docs this seems to be one of the known flaws of the original implementation, which allegedly was fixed. So maybe it'll work more reasonably in 3.37.0? (The library currently vendors version 3.36.0.)

@glebarez
Copy link
Author

glebarez commented Dec 1, 2021

I tested on 3.37 and observed same behavior in SQLite console.
I think it's not a bug of current driver but a specifics of SQLite itself.
I filed an issue to them (https://sqlite.org/forum/forumpost/793beaf322)
I think it's worth waiting for their response.

@glebarez
Copy link
Author

glebarez commented Dec 1, 2021

Sqlite admitted this as a bug and fixed in https://sqlite.org/src/info/a818ba2ed635b91e

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

3 participants