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

feat(psl): add validation + warning on missing indexes for relationMode = "prisma" #3429

Merged
merged 36 commits into from Nov 24, 2022

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Nov 22, 2022

Context: internal Notion doc.

This new validation is disabled when one of the following holds:

  • relationMode = "foreignKeys" (or similarly, when no relationMode is specified)
  • provider = "mongodb"

Contributes to prisma/language-tools#1296.
Closes prisma/language-tools#992.

TODOs (which do not block this PR):

Warning message:
"""
With relationMode = \"prisma\", no foreign keys are used, so relation fields will not benefit from the index usually created by the relational database under the hood.
This can lead to poor performance when querying these fields. We recommend adding an index manually.
Learn more at https://pris.ly/d/relation-mode#indexes"
"""

@jkomyno jkomyno added this to the 4.7.0 milestone Nov 22, 2022
@jkomyno jkomyno changed the base branch from main to feat/psl-support-warnings-in-tests November 22, 2022 15:53
Base automatically changed from feat/psl-support-warnings-in-tests to main November 22, 2022 16:57
@jkomyno jkomyno marked this pull request as ready for review November 22, 2022 17:09
@jkomyno jkomyno requested a review from a team as a code owner November 22, 2022 17:09
Copy link
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.

Just a few things.

I also think we're missing a test for the case where fields in the relation is a prefix of an indexe's fields.

Also we're missing code that wolud consider the primary key (which is also an index), but let's make that a separate issue/PR.

.any(|index_set| is_left_wise_included(&relation_fields, index_set));

if !relation_fields_appear_in_index {
let span = relation_field.ast_field().field_type.span();
Copy link
Contributor

Choose a reason for hiding this comment

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

The highlighting is going to be on the field type, so only the OtherModel part in other OtherModel @relation(...). I think it would be more readable for users if we highlighted the whole field. So relation_field.ast_field().span.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I had a doubt about this span here. I think we mentioned elsewhere in the Notion doc that we wanted to highlight the @relation attribute.
Hence, I'd propose the following

let span = ast_field
  .span_for_attribute("relation")
  .unwrap_or_else(|| ast_field.span());

which falls back to your solution

Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

Waiting for updated warning message.

@jkomyno
Copy link
Contributor Author

jkomyno commented Nov 24, 2022

About @tomhoule's comment:

Also we're missing code that would consider the primary key (which is also an index), but let's make that a separate issue/PR.

Do you mean something like

model SomeUser {
  id            Int       @id
  successor     SomeUser? @relation("UserHistory")
  predecessorId Int?      @unique
  predecessor   SomeUser? @relation("UserHistory", fields: [id], references: [id])
}

?

Comment on lines +28 to 32
/// Pure convenience method. Forwards to Diagnostics::push_warning().
pub(super) fn push_warning(&mut self, warning: DatamodelWarning) {
self.diagnostics.push_warning(warning);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@pimeys pimeys left a comment

Choose a reason for hiding this comment

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

A few small things. Ping me when done, and I'll approve!

Copy link
Contributor

@pimeys pimeys left a comment

Choose a reason for hiding this comment

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

Uh, didn't mean to press approve yet

@Jolg42
Copy link
Member

Jolg42 commented Nov 24, 2022

Note: we can do changes in a follow-up PR, we really need to merge this asap

Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

Error msg feedback was ignored, left 2 comments.

@jkomyno jkomyno requested a review from pimeys November 24, 2022 15:18
@jkomyno
Copy link
Contributor Author

jkomyno commented Nov 24, 2022

Github Actions is green, Buildkite is all green except CockroachDB, which is in progress. I'm merging

@jkomyno jkomyno merged commit c6a19ec into main Nov 24, 2022
@jkomyno jkomyno deleted the feat/add-index-warnings branch November 24, 2022 15:42
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.

Recommend adding indices to foreign keys when referentialIntegrity = "prisma"
7 participants