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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
01f1f2c
feat(psl): add "push_warning" helper to Context
jkomyno Nov 22, 2022
32b45c0
test(psl): add validation tests for missing relation index warning
jkomyno Nov 22, 2022
a188095
feat(psl): add skeleton for missing relation indexes when relationMod…
jkomyno Nov 22, 2022
97ccc4d
feat(psl): integrated "validate_missing_relation_indexes" into "valid…
jkomyno Nov 22, 2022
2d40109
feat(psl): extracted sets of indexes from the model containing a @rel…
jkomyno Nov 22, 2022
33e0912
feat(psl): fixed missing relation index detection
jkomyno Nov 22, 2022
f391f4b
feat(psl): add pretty_print support to warnings, defaulting to yellow…
jkomyno Nov 22, 2022
109d558
feat(psl): add warning support to validation_tests suite
jkomyno Nov 22, 2022
84e891f
chore(psl): fix clippy
jkomyno Nov 22, 2022
5701214
chore(psl): removed leftover code
jkomyno Nov 22, 2022
69c20c7
chore(psl): address review comments
jkomyno Nov 22, 2022
a312eb4
chore(psl): address review comments
jkomyno Nov 22, 2022
a78867e
chore: removed redundant static declaration
jkomyno Nov 22, 2022
2d25679
Merge branch 'feat/psl-support-warnings-in-tests' into feat/add-index…
jkomyno Nov 22, 2022
34e1b44
chore: fix clippy
jkomyno Nov 22, 2022
127c11b
chore: removed noise
jkomyno Nov 22, 2022
3818edf
Merge branch 'main' into feat/add-index-warnings
jkomyno Nov 22, 2022
f1ddc0e
chore: fix clippy, add one more unit test
jkomyno Nov 22, 2022
581b5ce
feat(psl): moved connector-specific logic to the "Connector" trait
jkomyno Nov 23, 2022
ecaf05e
feat(psl): attempt to reduce allocations in "validate_missing_relatio…
jkomyno Nov 23, 2022
a5a94d3
chore(psl): applied review comments, fixed regression in "validate_mi…
jkomyno Nov 23, 2022
b747f6b
fix: changed duplicated test
jkomyno Nov 23, 2022
afc42aa
feat(psl): add + Clone to "referencing_fields"
jkomyno Nov 23, 2022
81267a3
chore: swapped at_unique tests
jkomyno Nov 23, 2022
6302434
chore: apply review comments on "validate_missing_relation_indexes" f…
jkomyno Nov 23, 2022
cf302ba
chore: removed model argument from "validate_missing_relation_indexes"
jkomyno Nov 23, 2022
eb283a1
chore: moved the "validate_missing_relation_indexes" function before …
jkomyno Nov 23, 2022
98526a7
chore: changed pris.ly link in the "new_missing_index_on_emulated_rel…
jkomyno Nov 23, 2022
48312a4
chore: updated "new_missing_index_on_emulated_relation" warning message
jkomyno Nov 24, 2022
c57c297
chore: update Cargo.lock
jkomyno Nov 24, 2022
0832a54
chore: removed deprecated "referentialIntegrity" preview feature in n…
jkomyno Nov 24, 2022
873e328
chore(psl): updated warnings snapshot
jkomyno Nov 24, 2022
4233793
Merge branch 'main' into feat/add-index-warnings
jkomyno Nov 24, 2022
258b019
chore(psl): fixed unit tests?
jkomyno Nov 24, 2022
d050fcd
chore: apply review suggestions
jkomyno Nov 24, 2022
52a204e
chore: rephase wording in warning
jkomyno Nov 24, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions psl/builtin-connectors/src/mongodb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,4 +132,9 @@ impl Connector for MongoDbDatamodelConnector {
fn allowed_relation_mode_settings(&self) -> enumflags2::BitFlags<RelationMode> {
RelationMode::Prisma.into()
}

/// Avoid checking whether the fields appearing in a `@relation` attribute are included in an index.
fn should_suggest_missing_referencing_fields_indexes(&self) -> bool {
jkomyno marked this conversation as resolved.
Show resolved Hide resolved
false
}
}
1 change: 1 addition & 0 deletions psl/diagnostics/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ edition = "2021"
[dependencies]
colored = "2"
pest = "2.1.3"
indoc = "1"
13 changes: 11 additions & 2 deletions psl/diagnostics/src/warning.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use colored::{ColoredString, Colorize};

use crate::{
pretty_print::{pretty_print, DiagnosticColorer},
Span,
};
use colored::{ColoredString, Colorize};
use indoc::indoc;

/// A non-fatal warning emitted by the schema parser.
/// For fancy printing, please use the `pretty_print_error` function.
Expand All @@ -25,6 +25,15 @@ impl DatamodelWarning {
Self::new(message, span)
}

pub fn new_missing_index_on_emulated_relation(span: Span) -> DatamodelWarning {
let message = indoc! {
r#"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"#
};
Self::new(message.to_owned(), span)
}

/// The user-facing warning message.
pub fn message(&self) -> &str {
&self.message
Expand Down
4 changes: 2 additions & 2 deletions psl/parser-database/src/walkers/relation_field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,12 +170,12 @@ impl<'db> RelationFieldWalker<'db> {
}

/// The fields in the `fields: [...]` argument in the forward relation field.
pub fn referencing_fields(self) -> Option<impl ExactSizeIterator<Item = ScalarFieldWalker<'db>>> {
pub fn referencing_fields(self) -> Option<impl ExactSizeIterator<Item = ScalarFieldWalker<'db>> + Clone> {
self.fields()
}

/// The fields in the `fields: [...]` argument in the forward relation field.
pub fn fields(self) -> Option<impl ExactSizeIterator<Item = ScalarFieldWalker<'db>>> {
pub fn fields(self) -> Option<impl ExactSizeIterator<Item = ScalarFieldWalker<'db>> + Clone> {
let model_id = self.model_id;
let attributes = self.attributes();
attributes.fields.as_ref().map(move |fields| {
Expand Down
6 changes: 6 additions & 0 deletions psl/psl-core/src/datamodel_connector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,12 @@ pub trait Connector: Send + Sync {
self.has_capability(ConnectorCapability::RelationFieldsInArbitraryOrder)
}

/// If true, the schema validator function checks whether the referencing fields in a `@relation` attribute
/// are included in an index.
fn should_suggest_missing_referencing_fields_indexes(&self) -> bool {
true
}

fn native_type_to_string(&self, instance: &NativeTypeInstance) -> String {
let (name, args) = self.native_type_to_parts(instance);
let args = if args.is_empty() {
Expand Down
7 changes: 6 additions & 1 deletion psl/psl-core/src/validate/validation_pipeline/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::{
datamodel_connector::{Connector, RelationMode},
Datasource, PreviewFeature,
};
use diagnostics::{DatamodelError, Diagnostics};
use diagnostics::{DatamodelError, DatamodelWarning, Diagnostics};
use enumflags2::BitFlags;

/// The validation context. The lifetime parameter is _not_ the AST lifetime, but the subtype of
Expand All @@ -24,4 +24,9 @@ impl Context<'_> {
pub(super) fn push_error(&mut self, error: DatamodelError) {
self.diagnostics.push_error(error);
}

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

Choose a reason for hiding this comment

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

Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ pub(super) fn validate(ctx: &mut Context<'_>) {
relation_fields::ignored_related_model(field, ctx);
relation_fields::referential_actions(field, ctx);
relation_fields::map(field, ctx);
relation_fields::validate_missing_relation_indexes(field, ctx);
}

for index in model.indexes() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use crate::{
diagnostics::DatamodelError,
validate::validation_pipeline::context::Context,
};
use diagnostics::DatamodelWarning;
use enumflags2::BitFlags;
use itertools::Itertools;
use parser_database::{
Expand Down Expand Up @@ -249,3 +250,67 @@ pub(super) fn map(field: RelationFieldWalker<'_>, ctx: &mut Context<'_>) {
validate_db_name(field.model().name(), relation_attr, field.mapped_name(), ctx, false);
}
}

pub(super) fn validate_missing_relation_indexes(relation_field: RelationFieldWalker<'_>, ctx: &mut Context<'_>) {
if !ctx.connector.should_suggest_missing_referencing_fields_indexes() || ctx.relation_mode != RelationMode::Prisma {
return;
}

if let Some(fields) = relation_field.referencing_fields() {
let model = relation_field.model();
// Considers all fields that should be part of an index in the given model, w.r.t. to left-wise inclusion.
let referencing_fields_it = fields.map(|field| field.field_id());

// Considers all groups of indexes explicitly declared in the given model.
// An index group can be:
// - a singleton (@unique or @id)
// - an ordered set (@@unique or @@index)
let index_field_groups = model.indexes();

let referencing_fields_appear_in_index = index_field_groups
.map(|index_walker| index_walker.fields().map(|index| index.field_id()))
.any(|index_fields_it| {
let fields_it = referencing_fields_it.clone();
is_leftwise_included_it(fields_it, index_fields_it)
});

if !referencing_fields_appear_in_index {
let ast_field = relation_field.ast_field();
let span = ast_field
.span_for_attribute("relation")
.unwrap_or_else(|| ast_field.span());
ctx.push_warning(DatamodelWarning::new_missing_index_on_emulated_relation(span));
}
}
}

/// An subgroup is left-wise included in a supergroup if the subgroup is contained in the supergroup, and all the entries of
/// the left-most entries of the supergroup match the order of definitions of the subgroup.
/// More formally: { x_1, x_2, ..., x_n } is left-wise included in { y_1, y_2, ..., y_m } if and only if
/// n <= m and x_i = y_i for all i in [1, n].
fn is_leftwise_included_it<T>(subgrop: impl ExactSizeIterator<Item = T>, supergroup: impl Iterator<Item = T>) -> bool
where
T: PartialEq,
{
supergroup.take(subgrop.len()).eq(subgrop)
}

#[cfg(test)]
mod tests {
use super::is_leftwise_included_it;
#[test]
fn test_is_left_wise_included() {
let item = vec![1, 2];
let group = vec![1, 2, 3, 4];
assert_eq!(is_leftwise_included_it(item.iter(), group.iter()), true);
let item = vec![1, 2, 3, 4];
let group = vec![1, 2, 3, 4];
assert_eq!(is_leftwise_included_it(item.iter(), group.iter()), true);
let item = vec![1, 2, 3, 4];
let group = vec![1, 2];
assert_eq!(is_leftwise_included_it(item.iter(), group.iter()), false);
let item = vec![2, 3];
let group = vec![1, 2, 3, 4];
assert_eq!(is_leftwise_included_it(item.iter(), group.iter()), false);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// no missing relation index validation on mongodb.

datasource db {
provider = "mongodb"
url = env("TEST_DATABASE_URL")
}

model SomeUser {
id String @id @map("_id") @default(auto()) @db.ObjectId
posts Post[]
}

model Post {
id String @id @map("_id") @default(auto()) @db.ObjectId
userId String
user SomeUser @relation(fields: [userId], references: [id])
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// no missing relation index validation on relationMode = "foreignKeys" / no relationMode at all.

datasource db {
provider = "mysql"
url = env("TEST_DATABASE_URL")
}

model SomeUser {
id Int @id
posts Post[]
}

model Post {
id Int @id
userId Int
user SomeUser @relation(fields: [userId], references: [id])
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// no relation index validation warning on relationMode = "prisma" when a referenced field is already in @@unique.

datasource db {
provider = "mysql"
url = env("TEST_DATABASE_URL")
relationMode = "prisma"
}

model SomeUser {
id Int @id
profile Profile?
}

model Profile {
id Int @id
userId Int
user SomeUser? @relation(fields: [userId], references: [id])

@@unique([userId])
}
jkomyno marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// no relation index validation warning on relationMode = "prisma" when a referenced field is already in @unique.

datasource db {
provider = "mysql"
url = env("TEST_DATABASE_URL")
relationMode = "prisma"
}

model SomeUser {
id Int @id
profile Profile?
}

model Profile {
id Int @id
userId Int @unique
user SomeUser? @relation(fields: [userId], references: [id])
}
jkomyno marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// add missing relation index validation warning on relationMode = "prisma".

datasource db {
provider = "mysql"
url = env("TEST_DATABASE_URL")
relationMode = "prisma"
}

model SomeUser {
id Int @id
posts Post[]
}

model Post {
id Int @id
userId Int
user SomeUser @relation(fields: [userId], references: [id])
}
// warning: 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
// --> schema.prisma:17
//  | 
// 16 |  userId Int
// 17 |  user SomeUser @relation(fields: [userId], references: [id])
//  | 
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// add missing relation index validation warning on relationMode = "prisma".

datasource db {
provider = "mysql"
url = env("TEST_DATABASE_URL")
relationMode = "prisma"
}

model SomeUser {
idA Int
idB Int
idC Int
posts Post[]

@@id([idA, idB, idC])
}

model Post {
id Int @id
userIdA Int
userIdB Int
userIdC Int
user SomeUser @relation(fields: [userIdA, userIdB, userIdC], references: [idA, idB, idC])

@@index([userIdA, userIdB])
}
// warning: 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
// --> schema.prisma:23
//  | 
// 22 |  userIdC Int
// 23 |  user SomeUser @relation(fields: [userIdA, userIdB, userIdC], references: [idA, idB, idC])
//  | 
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// add missing relation index validation warning on relationMode = "prisma".

datasource db {
provider = "mysql"
url = env("TEST_DATABASE_URL")
relationMode = "prisma"
}

model SomeUser {
idA Int
idB Int
idC Int
posts Post[]

@@id([idA, idB, idC])
}

model Post {
id Int @id
userIdA Int
userIdB Int
userIdC Int
user SomeUser @relation(fields: [userIdA, userIdB, userIdC], references: [idA, idB, idC])

@@index([userIdB, userIdC])
}
// warning: 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
// --> schema.prisma:23
//  | 
// 22 |  userIdC Int
// 23 |  user SomeUser @relation(fields: [userIdA, userIdB, userIdC], references: [idA, idB, idC])
//  | 
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// add missing relation index validation warning on relationMode = "prisma".

datasource db {
provider = "mysql"
url = env("TEST_DATABASE_URL")
relationMode = "prisma"
}

model SomeUser {
idA Int
idB Int
idC Int
posts Post[]

@@id([idA, idB, idC])
}

model Post {
id Int @id
userIdA Int @unique
userIdB Int
userIdC Int @unique
user SomeUser @relation(fields: [userIdA, userIdB, userIdC], references: [idA, idB, idC])

@@unique([userIdA, userIdB])
}
// warning: 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
// --> schema.prisma:23
//  | 
// 22 |  userIdC Int @unique
// 23 |  user SomeUser @relation(fields: [userIdA, userIdB, userIdC], references: [idA, idB, idC])
//  |