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

maxLookup index out of range on update #54

Open
lgonzalez-silen opened this issue Dec 30, 2016 · 3 comments
Open

maxLookup index out of range on update #54

lgonzalez-silen opened this issue Dec 30, 2016 · 3 comments

Comments

@lgonzalez-silen
Copy link
Contributor

Hi,

Upon adding a new column to a table that was already large I started experiencing index out of range at

https://github.com/mgutz/dat/blob/v1/update.go#L178

It seems the condition that triggers hand off between preallocated parameter conversions and ad hoc conversions may be off by one. If I change i < maxLookup to i < maxLookup - 1 everything works fine but not sure if that's right, as the last element in equalsPlaceholderTab will not be used. Also, is it right that equalsPlaceholderTab starts with $0?

I wrote a little snippet to test for various values of maxLookup (https://github.com/mgutz/dat/blob/v1/init.go#L19):

type TestStruct struct {
	ID     int64  `db:"id"`
	Field1 string `db:"field1"`
	Field2 string `db:"field2"`
}

func TestBreak() {
	DB.SQL(`CREATE TABLE testtable (id serial, field1 text, field2 text);`).Exec()
	DB.SQL(`INSERT INTO testtable (field1, field2, field3) VALUES ('a', 'b');`).Exec()

	record := TestStruct{}
	_, err := DB.
		Update("testtable").
		SetWhitelist(record, "*").
		Where("id = $1", 1).
		Exec()
	if err != nil {
		panic(err)
	}

	DB.SQL(`DROP TABLE testtable;`).Exec()
}

This breaks for maxLookup <= 3 but works for larger values.

Cheers,

Luis

anoopr added a commit to homelight/dat that referenced this issue Feb 13, 2017
anoopr added a commit to homelight/dat that referenced this issue Feb 13, 2017
@mgutz
Copy link
Owner

mgutz commented Mar 17, 2017

Not ignoring this. I'm in a bit of time crunch right now.

@lgonzalez-silen
Copy link
Contributor Author

np! thanks

@lgonzalez-silen
Copy link
Contributor Author

See the open PR referenced above. Since we use our large table extensively, it might still be good to update maxLookup for us (although probably very marginally), but in general the PR fixes the bug.

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

2 participants