Omitting not-null parameters inserts zero value instead of an error

[inconshreveable]

sqlc promises type-safety in its generated code. Unfortunately it runs into the same problem that all Go programs do which is the inability to distinguish between a non-present value and a zero value.

I defined a table like so:

CREATE TABLE foo (
        id VARCHAR(27) NOT NULL,
        val INTEGER NOT NULL,
);

An insert statement like so:

-- name: CreateFoo :one
INSERT INTO foo
(id, val)
VALUES
($1, $2)
RETURNING *

And then called it like so:

query.CreateFoo(query.CreateFooParams{
    Val: 42,
})

I expect that this call will fail. In a perfect world it would fail at compile-time. It a better world it would fail at runtime when the database says "you tried to insert a row but did not specify the NOT NULL parameter id.

What I observed is that the call succeeds and that the database simply contains an empty string for the id value.

Mitigations:

There are two reasonable mitigations I can imagine, but they both have tradeoffs:

1. Make all non-null columns be pointer fields in the generated parameters structs. e.g.

type CreateFooParams struct {
        ID        *string
        Val      *int32
}

(these could be columns like sql.NullString instead of pointer values if you'd like, it makes little difference)

And then provide convenience functions to make the interface less gross. This is the approach taken by the AWS libraries. e.g.

query.CreateFoo(query.CreateFooParams{
    Val: sqlc.Int32(42),
})

or if it's generated using sql.NullInt32:

query.CreateFoo(query.CreateFooParams{
    Val: sql.NullInt32{Int32: 42, Valid: true},
})

Presumably this could be a tunable option that callers could choose on a query-by-query basis with another argument on the query (and could be set globally in sqlc.json as well):

-- name: CreateFoo :one udefcol: nil|zero
INSERT INTO foo
(id, val)
VALUES
($1, $2)
RETURNING *

2. Make all arguments to not null columns be standalone function arguments. e.g. generate code that looks like:

func (q *Queries) CreateFoo(ctx context.Context, id string, val int32) (Foo, error) {
}

This makes the call site far less readable, especially when working with many parameters but it enforces the type-safe at compile time which is nice. sqlc currently uses a heuristic that it takes that approach if there is a single argument (and generates a structure if there are 2+ arguments). It's possible that sqlc could make that a tunable argument of the query, e.g.

-- name: CreateFoo :one funcsig: struct|args
INSERT INTO foo
(id, val)
VALUES
($1, $2)
RETURNING *

If you specify struct, it generates a struct of all parameters, but if you specify args it generates a function call where each parameter is a separate argument to the function. This would allow the library user to trade off safety for readability.

[kyleconroy]

sqlc promises type-safety in its generated code.

The example code you provided is type-safe and correct. The default zero value for the ID field is the empty string, which according to your table definition is a valid value. If empty string is not valid, I'd suggest adding a column constraint.

CREATE TABLE foo (
        id VARCHAR(27) NOT NULL idchk (char_length > 0),
        val INTEGER NOT NULL,
);

Zero values are not NULL values and I think it's a mistake to try and equate the two. I've ported a few code bases to sqlc and have not run into this issue. I think the convenience of struct params outweighs the occasional forgotten parameter.

That said, I do understand this is an issue for larger code bases, so let’s look over your two suggestions in reverse oder.

I really do not like positional arguments for methods with many parameters. Beyond one argument, they become difficult to use. For your example table, it would be easy to mix up parameters when calling CreateFoo. Also, when looking at a call to the method, you have no idea which parameters map to which values.

query.CreateFoo(ctx, "", 0)

I also really dislike the “everything is a pointer” approach taken by the AWS SDK. Pointers to strings, ints, and bools are hard to create without helper methods.

I think the only option I like is the sql.Null* solution. Generated structs would have all parameters set to their corresponding null type.

type CreateFooParams struct {
        ID       sql.NullString
        Val      sql.NullInt32
}

query.CreateFoo(ctx, query.CreateFooParams{
    Val: sql.NullInt32{Int32: 42, Valid: true},
})

Constructing the CreateFooParams struct is a bit verbose, but could be improved with a helper library (outside the scope of sqlc)

query.CreateFoo(ctx, query.CreateFooParams{
    ID: null.String("some-id"),
    Val: null.Int32(42),
})

I know this solution looks basically the same as the pointer solution, but I like that it's explicit about expected zero value of fields.

The only downside to this approach is that it doesn't work well with types that do not have an associated sql.Null* value. Thoughts on what we could do there?

[talksik]

thanks for the detailed answer! I like the helper I went with: "gopkg.in/guregu/null.v4"

However, I don't want to change my schema just to remove NOT NULL, just so sqlc can generate the sql.Null*

I am frightened about the empty strings in my database already.

[andrewmbenton]

I just ran into this issue in a project using tables with <name> uuid NOT NULL; columns. It was frightening to see zero-valued UUIDs in the database and I had a similar reaction to @inconshreveable in that I expected sqlc to protect me from myself here somehow.

I think @kyleconroy has the right solution for types that map to sql.Null* types. Perhaps that's implemented now (I didn't check). If not I think it's a good path forward.

For types outside of sql.Null* I think it would be nice to have sqlc generate those and then use them in query.Create*Params structs to enforce usage. I think it should be possible to do this automatically but I may not have thought it through entirely.

[talksik]

I'm also frightened about the empty strings in my database already.

My workaround right now is do validation at the code level and erroring out before execution gets to the insert, instead of expecting insert to fail on validation. Not ideal for sure as it means we are maintaining schema validation at schema level and backend validation level.