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

Fix Insert issue for default postgres type template Insert operation #224

Closed
wants to merge 1 commit into from

Conversation

mccolljr
Copy link
Contributor

@mccolljr mccolljr commented May 4, 2020

As far as I can tell, the .Scan here will always fail without the RETURNING clause in the SQL (tried with a couple different drivers, lib/pq and pgx. Rather than add RETURNING when the primary key is set manually, I opted to just use Exec rather than QueryRow.

As far as I can tell, the `.Scan` here will always fail without the `RETURNING` clause in the SQL (tried with a couple different drivers, `lib/pq` and `pgx`. Rather than add `RETURNING` when the primary key is set manually, I opted to just use `Exec` rather than `QueryRow`.
@kenshaw
Copy link
Member

kenshaw commented May 4, 2020

@mccolljr I appreciate the work here -- can you share a small table definition and version of PostgreSQL you were using? There's a good chance that the way xo needs to pull type / sequence information from PostgreSQL has changed and is the real issue of the problem. Before I merge this, I'd like to verify it from my end. Thanks!

@mccolljr
Copy link
Contributor Author

mccolljr commented May 5, 2020

@kenshaw sorry for the delay, here's a small repro:

# postgres 9.6
CREATE TABLE "team" (
  "id" UUID NOT NULL PRIMARY KEY,
  "name" VARCHAR(100) NOT NULL
)

If you generate this, the Insert method will successfully create a record, but will return sql.ErrNoRows due to the call to Scan.

@mccolljr
Copy link
Contributor Author

mccolljr commented May 5, 2020

A further example (will have to replace with your own schema)

func TestXOIssue(t *testing.T) {
	db, err := sql.Open("postgres", "postgres://(user):(password)@127.0.0.1:5432/postgres?sslmode=disable")
	if err != nil {
		log.Fatalln(err)
	}
	if err := db.Ping(); err != nil {
		log.Fatalln(err)
	}

	team := models.Team{
		ID:   uuid.NewV4(),
		Name: "test",
	}

        t.Logf("Inserting team: %v", team)

	if err := team.Insert(db); err != nil {
		t.Logf("Insert returned error: %s", err)
	}

	if found, err := models.TeamByID(db, team.ID); err != nil {
		t.Logf("TeamByID returned error: %s", err)
	} else {
		t.Logf("TeamByID found team: %v", found)
	}
}

go test -v output for this test:

=== RUN   TestXOIssue
    TestXOIssue: xo_test.go:26: Inserting team: {e830900b-f04f-4e65-869e-cec27a8de2a6 test false false}
    TestXOIssue: xo_test.go:29: Insert returned error: sql: no rows in result set
    TestXOIssue: xo_test.go:35: TeamByID found team: &{e830900b-f04f-4e65-869e-cec27a8de2a6 test true false}
--- PASS: TestXOIssue (0.01s)

@mccolljr
Copy link
Contributor Author

mccolljr commented May 5, 2020

The generated code (using default templates)

// Insert inserts the Team to the database.
func (t *Team) Insert(db XODB) error {
	var err error

	// if already exist, bail
	if t._exists {
		return errors.New("insert failed: already exists")
	}

	// sql insert query, primary key must be provided
	const sqlstr = `INSERT INTO "public"."teams" (` +
		`"id", "name"` +
		`) VALUES (` +
		`$1, $2` +
		`)`

	// run query
	XOLog(sqlstr, t.ID, t.Name)
	err = db.QueryRow(sqlstr, t.ID, t.Name).Scan(&t.ID)
	if err != nil {
		return err
	}

	// set existence
	t._exists = true

	return nil
}

@turnkey-commerce
Copy link
Contributor

It looks like this tests fine, but also had to run the tpl.sh to update the go-bindata code with the change.

@mccolljr
Copy link
Contributor Author

@turnkey-commerce thanks for making the fix. I put this PR in from the GitHub web UI, and had forgotten that codegen needed to be run. 👍

@mccolljr mccolljr closed this May 13, 2020
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

Successfully merging this pull request may close these issues.

None yet

3 participants