Closed Oliver-Fish closed 2 years ago
Thanks for the detailed report! This is definitely a shortcoming in pggen. The relevant code is https://github.com/jschaf/pggen/blob/main/internal/pginfer/pginfer.go#L110. The code currently:
pg_prepared_statements
to get the types.Postgres doesn't provide any information in pg_prepared_statements about the nullability so we can't solely rely on that. For inputs, I chose to assume not null to avoid dealing with *string
or the pgtype
variants. Outputs assume everything is nullable by default to avoid losing information about nulls.
To infer nullability automatically would be quite difficult in the general case because you can have arbitrarily complex insert statements, like INSERT INTO foo SELECT some_complex_query
. To do it for simple cases like INSERT INTO .. VALUES
isn't too difficult but would require parsing the explain plan and combining that with data about the target table.
An alternative that I prefer is to specify the go-type directly in pggen.arg
, something like, pggen.arg('name', go_type := '*string').
That avoids the complex work of the explain statement. Taking that approach means I'll need to extend the crappy SQL parser in pggen to extract pggen.arg function invocations to get the default value. I don't think the regex-base approach is up to the task.
As an immediate work-around, assuming you're okay with using an empty string to mean null
, you could do something like:
-- name: Insert :exec
INSERT INTO actor (id, tenant_id, created, type, name)
VALUES (pggen.arg('id'), pggen.arg('tenant_id'), NOW(), pggen.arg('type'), NULLIF(pggen.arg('name'), ''));
Great response, I agree that this is actually a really complex problem to solve, especially when dealing with a query that does multiple table inserts through CTE's.
I would say that your workaround solves 99% of use-cases here, but of-course in the long run I agree that some more flexibility in pggen.arg is the key to me using this project to build out all of my database communication.
The biggest problem I guess you face is allowing for dynamic queries that don't offload the complexity to the actual query and instead solve these problems at build time. I'm thinking of things like SELECT queries that contain WHERE clauses that are only needed if a value was supplied. Is this something your planning to tackle?
Is this something your planning to tackle?
Yes, I'd like to eventually add dynamic queries. As a first sketch, maybe extend pggen with:
pggen.ident
to pass identifier names: SELECT * FROM foo ORDER BY pggen.ident('order_by');
pggen.predicate
to pass expressions that evaluate to a boolean: SELECT * FROM foo WHERE pggen.predicate('pred');
. That would take an arg that matches a pggen.Predicate
Go interface which would support building up an expression AST for arbitrarily complex predicates. Then, pggen would be responsible for serializing pggen.Predicate
into a safe SQL string.A similar request popped up in https://github.com/jschaf/pggen/issues/18.
An alternative that I prefer is to specify the go-type directly in pggen.arg, something like, pggen.arg('name', go_type := '*string').
pggen.predicate to pass expressions that evaluate to a boolean: SELECT * FROM foo WHERE pggen.predicate('pred');. That would take an arg that matches a pggen.Predicate Go interface which would support building up an expression AST for arbitrarily complex predicates. Then, pggen would be responsible for serializing pggen.Predicate into a safe SQL string.
@jschaf That would be awesome! are these already in roadmap?
There's a workaround for optional args with nullif
so going to close. The request for dynamic predicate support is covered by #18.
Given the following schema
and the following insert query
The insert parms struct that is generated looks like this
I would however expect it to look like this
This is because in the schema name is nullable and thus when I am inserting if I don't pass a pointer null should be supplied to the query.
Is it possible to overcome this issue? I couldn't see anything in the docs so sorry if I missed something.