Error on goose unfinished query

[kevinburke1]

I had this in a goose sql file

-- +goose Up
ALTER TABLE providers ADD COLUMN "path" TEXT NOT NULL DEFAULT '';
COMMENT ON COLUMN "providers"."path"
    IS 'Path relative to the public.meter.com root, for example "/providers/Sonic.png".'

-- +goose Down
ALTER TABLE providers DROP COLUMN "path";

When I run goose up, I get:

goose: migrating db environment 'development', current version: 20210409223401, target: 20210429193818
2021/04/29 14:59:25 WARNING: Unexpected unfinished SQL query: COMMENT ON COLUMN "providers"."path"
    IS 'Path relative to the public.meter.com root, for example "/providers/Sonic.png".'. Missing a semicolon?
OK    20210429193818_ProviderPath.sql

sqlc still generated the file though. It would be nice if sqlc would just error here instead of letting me generate a valid Go file.

[kyleconroy]

I think I know what the issue here is. When handling migration files, sqlc will drop the rest of the file after -- +goose Down. In this case, that ends up automatically adding a semicolon at the end of the file.

We should parse the migration file twice. One to verify that the SQL is valid for the entire file and then once again after we've removed the down command.

[mfridman]

Are you accepting PR's to fix this particular issue? Happy to help.

[kyleconroy]

@mfridman Yep

[carlclone]

is this issue solved ? want to work on this.

[mfridman]

I was perusing the codebase and had a thought, what if there was an option like schema_type that hinted to sqlc what migration tool is being utilized and used its sql parser to extract the statements. This does mean adding a dependency on an upstream package, such as the sqlparser in goose.

What we gain is a common way to parse the statements across migration tools and sqlc, which improves compatibility and reduces issues like this one.

After writing this, maybe not the best idea. And instead, validation of the .sql files should be left to the tools themslves.

For goose, there is now a goose validate command to validate .sql file which can be run alongside sqlc.

[kyleconroy]

Agreed that this should be left up to the migration tools themselves. The other option would be to parse the full sql file before we remove the down migrations.

Going to opt for the first solution and close this out.