[do not merge] 4.6.x #3380
Closed
[do not merge] 4.6.x #3380
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* 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
This regression was reported by users in prisma/prisma#16180 It was introduced on b6400f1 by the same change that caused the _other_ regression that was fixed in eb39236. Symptoms: enum migrations became non-idempotent in many cases, depending on the ordering of the migrations in the Prisma schema. In that refactoring, we started storing enum ids instead of enum names in `ColumnTypeFamily::Enum(_)`. That makes the `ColumntypeFamily` type smaller, easier to deal with with regards to ownership (`EnumId` is `Copy`), and ready for multi-schema, because the name can be ambiguous and does not carry schema information, whereas the `EnumId` does. But more relevant to this commit: it makes comparing two `ColumnTypeFamily` inaccurate _when the types come from two different schemas_, like in sql_schema_differ. That in turn changed the signature of `column_type_family_as_enum()`, leading to the differ comparing enum ids instead of enum names, which directly causes the false positives in diffing, because the same enum has a different ID in each of the two schemas. The code that caused the regression does not appear in b6400f1, it uses code that was changed there. This is also a problem with the generated implementation of PartialEq for ColumnTypeFamily. Since it would be a larger refactoring, we only deal with this where relevant (sql_schema_differ) and leave removing the dangerous PartialEq implementation to another PR. This is the issue: #3373. How could this have been caught? - Tests that check that migrations are idempotent with multiple enums, all used in model columns _not defined in alphabetical order_ (sad but true). - This chimes with the theme of us not testing with large enough Prisma schemas. - migrations-ci could have caught this. We are neglecting it at the moment.
|
Will add #3377 as soon as it is merged. |
(cherry picked from commit 379f9d3)
|
I cherry-picked #3377 from commit on main to |
|
@tom since all these commits are on main branch, we can close this PR I think |
|
I restored the branch of this PR so we have https://github.com/prisma/prisma-engines/tree/4.6.x and main...4.6.x so see what the patch release included exactly. (Note we also have earlier patch release branches around still) |
|
Yes we usually keep these branches open. |
|
We fixed the protection rule |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.