hasura / hasura/graphql-engine

Missing semi-colon in drop column down migration

Open
#7,944 0 comments 0 reactions 0 assignees View on GitHub
c/cli k/bug t/product-platform
Dominant language
TypeScript
Stars
32.1k
Forks
3k
PR merge metrics
PR metrics pending

Description

### Version Information

Server Version:
CLI Version (for CLI related issue):

2.1.0

### Environment

Docker

### What is the expected behaviour?

I've revisited an older project of mine and am in the process of upgrading from Hasura 1.2.1 to 2.1.0. As part of that, I've updated the CLI client and that requires that I upgrade my project to the v2 configuration. After running the `update-project-v2` script, I was left with an invalid migration file. This is very similar to what I saw with #3543.

### Keywords

Migrations,v2-upgrade

### What is the current behaviour?

The problem is the upgrade script merged a SQL statement from my _down.yaml_ file and my _down.sql_ file without inserting a semi-colon in between, leaving me with an invalid SQL statement:

```sql
ALTER TABLE "public"."todos" DROP COLUMN "completed_at" CASCADEDROP TRIGGER trigger_todos_completed_changed on todos;
DROP FUNCTION update_todos_completed_at;
```

### How to reproduce the issue?

Recreating the issue exactly the same will likely require my entire schema. I'll share the files from the old migration and hopefully that's enough to track down the issue. If not, I can upload the whole project somewhere.

Here are my v1 migration files.

down.yaml

```yaml
- args:
role: user
table:
name: todos
schema: public
type: drop_select_permission
- args:
permission:
allow_aggregations: false
columns:
- created_at
- id
- is_completed
- profile_id
- rank
- title
computed_fields: []
filter:
profile:
user:
auth0_id:
_eq: X-Hasura-User-Id
role: user
table:
name: todos
schema: public
type: create_select_permission
- args:
sql: ALTER TABLE "public"."todos" DROP COLUMN "completed_at" CASCADE
type: run_sql
```

down.sql

```sql
DROP TRIGGER trigger_todos_completed_changed on todos;
DROP FUNCTION update_todos_completed_at;
```

up.yaml

```yaml
- args:
sql: ALTER TABLE "public"."todos" ADD COLUMN "completed_at" timestamptz
type: run_sql
- args:
sql: ALTER TABLE "public"."todos" ALTER COLUMN "completed_at" DROP NOT NULL
type: run_sql
- args:
role: user
table:
name: todos
schema: public
type: drop_select_permission
- args:
permission:
allow_aggregations: false
columns:
- completed_at
- created_at
- id
- is_completed
- profile_id
- rank
- title
computed_fields: []
filter:
profile:
user:
auth0_id:
_eq: X-Hasura-User-Id
role: user
table:
name: todos
schema: public
type: create_select_permission

```

up.sql

```sql
CREATE OR REPLACE FUNCTION update_todos_completed_at()
RETURNS TRIGGER AS $$
BEGIN
IF NEW.is_completed = TRUE THEN
NEW.completed_at = NOW();
ELSE
NEW.completed_at = NULL;
END IF;

RETURN NEW;
END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER trigger_todos_completed_changed
BEFORE UPDATE ON todos
FOR EACH ROW
EXECUTE PROCEDURE update_todos_completed_at();

```

### Please provide any traces or logs that could help here.

I don't see any traces to share.

### Any possible solutions?

I can manually fix the broken V2 migration files. It's not a large project. But, it does cause concern as to what else may have been lost in the upgrade.

### Can you identify the location in the source code where the problem exists?

I've only looked at the source, but I think the issue is in https://github.com/hasura/graphql-engine/blob/40678855d03c70204419b7b27fa9068897cc4c7c/cli/commands/scripts_update_config_v2.go#L203-L204. It looks to me like Hasura put DDL statements in the old YAML migration files that did not end with `;` and the V2 upgrade script assumes that they exist. I don't know the full history of the migration formats, but I suspect checking if `to.SQL` ends with a `;` and appending one if it doesn't would solve the problem. If a `;` were unconditionally added, I believe that'd also solve the problem with the caveat that the migration may end up with empty statements.

### If the bug is confirmed, would you be willing to submit a PR?

It's possible I could submit a PR. I don't actively write in Go, so any patch would be delayed while I grapple with the new language and project.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.