Skip to content

Do not panic in certain index validations#1603

Merged
pimeys merged 2 commits intomasterfrom
datamodel/duplicate-index-panic-fix
Feb 3, 2021
Merged

Do not panic in certain index validations#1603
pimeys merged 2 commits intomasterfrom
datamodel/duplicate-index-panic-fix

Conversation

@pimeys
Copy link
Copy Markdown
Contributor

@pimeys pimeys commented Feb 3, 2021

If we have two models:

model A {
  id Int @id @derfault(autoincrement())
  b  Int

  @@index([b], name: "foo")
}

model B {
  id Int @id @derfault(autoincrement())
  a  Int

  @@unique([a], name: "foo")
}

Both models will have an index with a same name, but the data we only looked with the attribute name index, not with unique. On PostgreSQL this is a validation error, but instead we panic.

Closes: prisma/prisma#5171

If we have two models:

```prisma
model A {
  id Int @id @derfault(autoincrement())
  b  Int

  @@index([b], name: "foo")
}

model B {
  id Int @id @derfault(autoincrement())
  a  Int

  @@unique([a], name: "foo")
}
```

Both models will have an index, but the data we only looked with the
attribute name `index`, not with `unique`.
@pimeys pimeys added this to the 2.17.0 milestone Feb 3, 2021
Copy link
Copy Markdown
Contributor

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! I know you probably weighed the tradeoff already, but sure this doesn't need a separate test and adjusting the existing one won't introduce a gap?

let ast_index = ast_model
.attributes
.iter()
.find(|attribute| attribute.name.name == "index")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:(

.attributes
.iter()
.find(|attribute| attribute.name.name == "index")
.find(|attribute| attribute.is_index())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

@pimeys
Copy link
Copy Markdown
Contributor Author

pimeys commented Feb 3, 2021

I'm still not sure is this correct. What if the model have two indexes? It chooses the first for error message??

@pimeys
Copy link
Copy Markdown
Contributor Author

pimeys commented Feb 3, 2021

lgtm! I know you probably weighed the tradeoff already, but sure this doesn't need a separate test and adjusting the existing one won't introduce a gap?

f1763bb

Added a test.

@tomhoule
Copy link
Copy Markdown
Contributor

tomhoule commented Feb 3, 2021

looks good

@pimeys pimeys merged commit cd65a79 into master Feb 3, 2021
@pimeys pimeys deleted the datamodel/duplicate-index-panic-fix branch February 3, 2021 16:33
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.

Error: Error in migration engine. Reason: [libs/datamodel/core/src/transform/ast_to_dml/validate.rs:226:34] called Option::unwrap() on a None value

2 participants