volatiletech / sqlboiler

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

Insert fails with SIGSEGV when a table has numeric column with not null constraint #843

Open mtsmfm opened 3 years ago

mtsmfm commented 3 years ago

The following code fails with SIGSEGV

    user := models.User{}
    user.Insert(ctx, db, boil.Infer())
$ DATABASE_URL=postgres://postgres:password@postgres/postgres?sslmode=disable go run utils/foo.go          
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x60 pc=0x68e9dc]

goroutine 1 [running]:
github.com/ericlagergren/decimal.(*Big).IsNaN(...)
        /home/app/go/pkg/mod/github.com/ericlagergren/decimal@v0.0.0-20181231230500-73749d4874d5/big.go:853
github.com/volatiletech/sqlboiler/v4/types.decimalValue(0x0, 0x8fe300, 0x0, 0x0, 0x7fa1a03ebca0, 0x8a9601)
        /home/app/go/pkg/mod/github.com/volatiletech/sqlboiler/v4@v4.2.0/types/decimal.go:140 +0x3c
github.com/volatiletech/sqlboiler/v4/types.Decimal.Value(0x0, 0xc0001a15f0, 0x40d1e5, 0x6c4760, 0x7007c0)
        /home/app/go/pkg/mod/github.com/volatiletech/sqlboiler/v4@v4.2.0/types/decimal.go:58 +0x30

schema

CREATE TABLE IF NOT EXISTS users (
  id INTEGER PRIMARY KEY,
  dollars NUMERIC NOT NULL
);

https://gist.github.com/mtsmfm/738f16a88a0104d4f40692d68f30d68e

Is this intentional?

aarondl commented 3 years ago

The decimal type is a bit weird. Because you have not null there, it expects you to have a value for it. Semi-intentional.

The decimal type is defined like this:

type Decimal struct {
  *big.Decimal
}

so if the big.Decimal is nil it'll panic like this.

I don't know if it's fixable, but I'd accept a PR to do a check and return an error if it is.

stephenafamo commented 2 years ago

This was fixed by #1044. And released in v4.8.4

The code above will now throw an error for violating the not-null constraint.

But this raises a new question:
Should the driver.Value() of a new types.Decimal{} be 0.0? Currently, it would return nil which the driver would report as NULL.

We have explicit types.Decimal and types.NullDecimal and since we know that types.Decimal is not meant to be NULL should we prevent this by default?

stephenafamo commented 2 years ago

PR: #1073