volatiletech / sqlboiler

Generate a Go ORM tailored to your database schema.
BSD 3-Clause "New" or "Revised" License
6.73k stars 544 forks source link

Sqlboiler assumes all SQL defaults are Go zero value #237

Closed Dirbaio closed 6 years ago

Dirbaio commented 6 years ago

Create a table like this (using Postgres):

CREATE TABLE foo (
    id          integer   NOT NULL PRIMARY KEY,
    allow_bar   boolean   NOT NULL DEFAULT true
);

Then try to insert something with allow_bar = false

f := Foo {
    ID: 4,
    AllowBar: false,
}
f.Insert(db) 

Then select the object, it ends up in the db with allow_bar = true

aarondl commented 6 years ago

This actually recently came up with someone else using sqlboiler. It's actually a more common use case than I thought and it sucks. Anything with a default that is the zero value is determined to "not be set".

Because we don't have dirty flagging on the struct, we don't actually know what information you want to send to the database. So we take an educated guess: https://github.com/volatiletech/sqlboiler/blob/master/strmangle/sets.go#L3 https://github.com/volatiletech/sqlboiler/blob/master/strmangle/sets.go#L15

Unfortunately it can't always be right, because it's based on the assumption that the 0-value in your struct means unset. And in the case of bools, false is the zero value.

The solution to be perfect of course is dirty flagging, if the value has been set since retrieval/object creation, flag it as dirty so it's sent to the database. Unfortunately that means all struct interactions with the above table look like:

allowBar := f.AllowBar()
f.SetAllowBar(false)

It was a goal of sqlboiler NOT to support this syntax, to keep it frictionless and make it so that the way you work with your structs is the way you would work with them if you had written them yourself.

Unfortunately this means that we need this sort of inference, which isn't always correct.

So the normal workaround/fix for this that I've suggested in the past is: For any zero value that has a default in the database that you'd like to insert, use a whitelist.

I'm happy to discuss changes to the inference, it's possible we can do something different around this. Maybe we could make bool an exception? But it feels a bit wrong. Thoughts?

Dirbaio commented 6 years ago

Why not just insert all the columns regardless of whether they have a default or not? This would ensure correct behavior at the cost of performance and not being able to use the database defaults. If users care about performance they can use a whitelist.

If you want to allow using database defaults, sqlboiler could maybe read the defaults from the db and generate a NewFoo() function that returns a new Foo instance with all the fields populated with the defaults from the DB?

aarondl commented 6 years ago

Because inserting regardless of default is the wrong thing to do most of the time. Consider primary key IDs which are typically on most tables. Those are default values (just like any other default value), and they shouldn't be inserted into because their value always comes from the database. Also it's possible to do some really complex stuff with default values or trigger evaluation and some people rely on this functionality to properly assign to fields when they create a new record.

For the same reason about complexity above it's not possible to read the default values from the database and assign them into the struct in any complete manner. For the same reasons it's not possible to compare the value in the struct to the would-be default value.

The NewFoo() is also not a great path because now to insert some values, you have to do 2 different INSERT INTO calls in the database where one would do.

The only thing I could think to do would be to drop the inference completely and never insert into default values unless whitelisted. Though that doesn't seem great because of the surprise factor (though the current behavior sure is surprising when trying to insert a 0-value over top of a default doesn't it?). The issue is obviously most visible with bools.

I'm trying to decide how we might fix this, but currently I'm at a loss except for what I suggested - which still leaves us with the exact same issue you filed here, equally as surprising until you understand what's going on, and will lead to bugs in code. Still thinking. Now's the time to find a solution though, since v3 still hasn't landed.

aarondl commented 6 years ago

Want to meditate on this one a little more. The idea of inference, blacklists, whitelists, and filters has been brought up often enough: #165 as well as another colleague had talked to me about this.

aarondl commented 6 years ago

I believe the referenced commit solves this use case fairly nicely. Now when inserting/updating you -must- provide a column list to do the operation with. To get the old rules, use boil.Infer() (same as not using a whitelist in v2). In order to deal with this annoying issue (where the inference fails on default bool trues) you can supplement the inferenced column list with your own list using boil.Greylist. Please see the documentation of boil.(Insert|Update)ColumnSet for more details on how the Columns type affects each type of query.