sqrl icon indicating copy to clipboard operation
sqrl copied to clipboard

SelectFrom args with wrong placeholder values

Open pindamonhangaba opened this issue 4 years ago • 1 comments

The query params inside a FromSelect are numbered independently from the whole query, so:

psql.Update(`mytable`).
		Set("name", "newname").
                FromSelect(psql.Select("column").From("anothertable").Where(sq.Eq{"column": 3})).
                Where(sq.Eq{"name": "oldname"})

Args will be ["newname", "oldname", 3], but query string will be:

update mytable set name = $1
select column from anothertable where column = $1
where name = $2

Correct should be:

update mytable set name = $1
select column from anothertable where column = $3
where name = $2

FromSelect should change the placeholder count to start from where the parent Update ends

pindamonhangaba avatar Feb 09 '21 13:02 pindamonhangaba

I just stumbled on this problem myself. In my case the query is something like this:

import (
	"fmt"
	sq "github.com/elgris/sqrl"
	"testing"
)

func TestBug(t *testing.T) {
	sq.StatementBuilder = sq.StatementBuilder.PlaceholderFormat(sq.Dollar)

	q := sq.Insert("table").Columns(
		"col_a",
		"col_b",
		"col_c",
	).Select(sq.Select().
		Column("tile.data").
		Column("?", "hello").
		Column("?", "world").
		FromSelect(
			sq.Select().Column(
				"ST_Tile(ST_FromGDALRaster(?, ?), ?, ?) AS data", []byte{}, 4326, 100, 100,
			), "tile",
		),
	)

	sql, args, _ := q.ToSql()
	fmt.Printf("sql=%v arg=%v", sql, args)
}

And the generated SQL is:

INSERT INTO table (col_a,col_b,col_c) SELECT tile.data, $1, $2 FROM (SELECT ST_Tile(ST_FromGDALRaster($1, $2), $3, $4) AS data) AS tile

@elgris @shaxbee I think this is a major bug. I realized something is wrong because I got an error about wrong value types. But I think that in the worst case scenario this wrong query would execute silently if the value happened to match column types.

The problem here is that when InsertBuilder calls b.iselect.ToSql(), the returned SQL already has placeholders replaced. I think this could be solved by adding a method like toSqlRaw that won't replace placeholders. Or use some other way to make sure that builders inside another builder always return ? placeholders, and then only the topmost builder would call ReplacePlaceholders.

kszafran avatar Sep 10 '21 15:09 kszafran