Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

me: fix regression w/ mapped enum names on postgres #3368

Merged
merged 2 commits into from Nov 9, 2022

Conversation

tomhoule
Copy link
Contributor

@tomhoule tomhoule commented Nov 9, 2022

When calculating a SQL schema from a Prisma schema, the Postgres
connector of the migration engine would use the name of the enum instead
of its database name (see parser-database docs for the reference on what
these mean). To picture this with an example, the following schema.

enum PersonType {
  Dog
  Cat
  @@Map("pet_preference")
}

would correspond, before the regression, and after this commit, to:

CREATE TYPE "pet_preference" AS ENUM ('Dog', 'Cat');

but the regression caused it to correspond to:

CREATE TYPE "PersonType" AS ENUM ('Dog', 'Cat');

This does not only affect newly created enums: we diff the wrong enums,
causing bad migrations for existing users of Migrate that have the enums
with the correct (mapped) names in their database.

Users reported the regression in prisma/prisma#16180
This commit should completely fix the problem.

The regression was introduced in an unrelated refactoring in
b6400f1. Enum names in column types
were replace with enum ids — a win for correctness. As part of this
refactoring, the sql_schema_describer::Enum type was made private, and
adding enums to a schema became possible only through
SqlSchema::push_enum. In the SQL migration connector

What could have caught this:

  • Testing that the name of the database enum in the migration is the
    mapped name. We can test this:
    • Through .assert_schema(), which has existed for a long time.
      It is a gap in our test coverage that we weren't testing this.
    • The new single migration tests, like the test in this commit.
      They are new, and they are less work than the previous option.
  • migrations-ci could have caught this, but it is currently neglected.

When calculating a SQL schema from a Prisma schema, the Postgres
connector of the migration engine would use the name of the enum instead
of its database name (see parser-database docs for the reference on what
these mean). To picture this with an example, the following schema.

```
enum PersonType {
  Dog
  Cat
  @@Map("pet_preference")
}
```

would correspond, before the regression, and after this commit, to:

```
CREATE TYPE "pet_preference" AS ENUM ('Dog', 'Cat');
```

but the regression caused it to correspond to:

```
CREATE TYPE "PersonType" AS ENUM ('Dog', 'Cat');
```

This does not only affect newly created enums: we diff the wrong enums,
causing bad migrations for existing users of Migrate that have the enums
with the correct (mapped) names in their database.

Users reported the regression in prisma/prisma#16180
This commit should completely fix the problem.

The regression was introduced in an unrelated refactoring in
b6400f1. Enum names in column types
were replace with enum ids — a win for correctness. As part of this
refactoring, the `sql_schema_describer::Enum` type was made private, and
adding enums to a schema became possible only through
`SqlSchema::push_enum`. In the SQL migration connector

What could have caught this:

- Testing that the name of the database enum in the migration is the
  mapped name. We can test this:
    - Through `.assert_schema()`, which has existed for a long time.
      It is a gap in our test coverage that we weren't testing this.
    - The new single migration tests, like the test in this commit.
      They are new, and they are less work than the previous option.
- migrations-ci could have caught this, but it is currently neglected.
@tomhoule
Copy link
Contributor Author

tomhoule commented Nov 9, 2022

Looking into the vitess test failures.

edit: they're almost certainly unrelated, see the PR in my next comment.

@tomhoule
Copy link
Contributor Author

tomhoule commented Nov 9, 2022

#3369

@tomhoule tomhoule merged commit eb39236 into main Nov 9, 2022
@tomhoule tomhoule deleted the me/pg-enum-mapped-name-regression branch November 9, 2022 11:15
tomhoule added a commit that referenced this pull request Nov 9, 2022
* me: fix regression w/ mapped enum names on postgres

When calculating a SQL schema from a Prisma schema, the Postgres
connector of the migration engine would use the name of the enum instead
of its database name (see parser-database docs for the reference on what
these mean). To picture this with an example, the following schema.

```
enum PersonType {
  Dog
  Cat
  @@Map("pet_preference")
}
```

would correspond, before the regression, and after this commit, to:

```
CREATE TYPE "pet_preference" AS ENUM ('Dog', 'Cat');
```

but the regression caused it to correspond to:

```
CREATE TYPE "PersonType" AS ENUM ('Dog', 'Cat');
```

This does not only affect newly created enums: we diff the wrong enums,
causing bad migrations for existing users of Migrate that have the enums
with the correct (mapped) names in their database.

Users reported the regression in prisma/prisma#16180
This commit should completely fix the problem.

The regression was introduced in an unrelated refactoring in
b6400f1. Enum names in column types
were replace with enum ids — a win for correctness. As part of this
refactoring, the `sql_schema_describer::Enum` type was made private, and
adding enums to a schema became possible only through
`SqlSchema::push_enum`. In the SQL migration connector

What could have caught this:

- Testing that the name of the database enum in the migration is the
  mapped name. We can test this:
    - Through `.assert_schema()`, which has existed for a long time.
      It is a gap in our test coverage that we weren't testing this.
    - The new single migration tests, like the test in this commit.
      They are new, and they are less work than the previous option.
- migrations-ci could have caught this, but it is currently neglected.

* me: move enum test to enums directory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants