canonical / sqlair

Friendly type mapping for SQL databases
Apache License 2.0
17 stars 8 forks source link

Add support for input expressions in INSERT statements #69

Closed Aflynn50 closed 6 months ago

Aflynn50 commented 1 year ago

Add explicit support for input expressions in INSERT statements. For example:

The tests are updated to use this new syntax for creating test databases. The test TestJujuStore is removed as it was added as a proof of concept and is no longer needed. The test would need a significant rewrite to work with the new method for creating a test DB.

marco6 commented 1 year ago

Few thoughts we already discussed on syntax 1, just to keep track. In case the struct has a database-generated ID, this API creates a problem.

Consider the struct used as an example:

CREATE TABLE person(
    id INTEGER NOT NULL AUTO_INCREMENT,
    name TEXT NOT NULL,
    team TEXT NOT NULL
);

And the go representation:

type Person struct {
    ID  int `db:"id"`
    Name    string  `db:"name"`
    Team    string  `db:"team"`
}

Normally, you would insert into that table with something like:

func newPerson(person Person) (id int) {
    res, _ := db.Exec("INSERT INTO person(name, team) VALUES (?, ?)", person.Name, person.Team);
    return res.LastInsertedId(); // Doesn't work for postgres BTW.
}

Now, replacing with the first syntax would look like:

func newPerson(person Person) (id int) {
    res, _ := db.Exec("INSERT INTO person(*) VALUES (&Person.*)", person);
    return res.LastInsertedId();
}

Now this query seems right and innocent (and it is most likely consistend with other usages of the API). However, the query gets expanded, including the ID which is most likely 0 (or whatever client-generated - which can easily conflict in high-concurrency scenarios).

It seems this problem doesn't affect syntax 2 & 3, even if in those case you loose partially the niceness of not having to list all fields manually...

Aflynn50 commented 1 year ago

Few thoughts we already discussed on syntax 1, just to keep track. In case the struct has a database-generated ID, this API creates a problem.

Consider the struct used as an example:

CREATE TABLE person(
  id INTEGER NOT NULL AUTO_INCREMENT,
  name TEXT NOT NULL,
  team TEXT NOT NULL
);

And the go representation:

type Person struct {
  ID  int `db:"id"`
  Name    string  `db:"name"`
  Team    string  `db:"team"`
}

Normally, you would insert into that table with something like:

func newPerson(person Person) (id int) {
  res, _ := db.Exec("INSERT INTO person(name, team) VALUES (?, ?)", person.Name, person.Team);
  return res.LastInsertedId(); // Doesn't work for postgres BTW.
}

Now, replacing with the first syntax would look like:

func newPerson(person Person) (id int) {
  res, _ := db.Exec("INSERT INTO person(*) VALUES (&Person.*)", person);
  return res.LastInsertedId();
}

Now this query seems right and innocent (and it is most likely consistend with other usages of the API). However, the query gets expanded, including the ID which is most likely 0 (or whatever client-generated - which can easily conflict in high-concurrency scenarios).

It seems this problem doesn't affect syntax 2 & 3, even if in those case you loose partially the niceness of not having to list all fields manually...

I'll work on adding the omitempty tag to this PR, I knew this was an issue but on reflection it is quite a big issue that probably warrants being address straight away rather than in a subsequent PR.

A field with the omitempty tag will be omitted from asterisk input expressions meaning queries like the one above will be safe.