fix(psl): composite type validation in indices #4401
Merged
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
Druue
requested review from
miguelff and
jkomyno
and removed request for
a team
Druue
commented
Nov 1, 2023
psl/psl-core/src/validate/validation_pipeline/validations/indexes.rs
Outdated
Show resolved
Hide resolved
Druue
commented
Nov 1, 2023
3 tasks
CodSpeed Performance ReportMerging #4401 will not alter performanceComparing Summary
|
Druue
force-pushed
the
fix/composites_compound_index
branch
from
afeaee9 to
e3f1ca5
Compare
janpio
reviewed
Nov 1, 2023
janpio
reviewed
Nov 1, 2023
janpio
reviewed
Nov 1, 2023
janpio
reviewed
Nov 1, 2023
janpio
reviewed
Nov 1, 2023
janpio
reviewed
Nov 8, 2023
| @@ -123,7 +123,7 @@ pub(super) fn validate(ctx: &mut Context<'_>) { | |||
| indexes::supports_clustering_setting(index, ctx); | |||
| indexes::clustering_can_be_defined_only_once(index, ctx); | |||
| indexes::opclasses_are_not_allowed_with_other_than_normal_indices(index, ctx); | |||
| indexes::composite_types_are_not_allowed_in_index(index, ctx); | |||
| indexes::composite_type_in_compound_unique_index(index, ctx); | |||
There was a problem hiding this comment.
What about normal indexes? Are those supported with a mix of scalar and composite fields?
There was a problem hiding this comment.
They don't actually hit this error (fulltext also doesn't). Not to say they're supported but they run into different issues that are unrelated to the original crash report. Only @@unique causes a panic from what I've found
miguelff
approved these changes
Nov 8, 2023
| @@ -91,7 +91,7 @@ impl ScalarField { | |||
|
|
|||
| match scalar_field_type { | |||
| ScalarFieldType::CompositeType(_) => { | |||
| unreachable!("Cannot convert a composite type to a type identifier. This error is typically caused by mistakenly using a composite type within a composite index.",) | |||
| unreachable!("This shouldn't be reached; composite types are not supported in compound unique indices.",) | |||
| group String | ||
| } | ||
| #[test] | ||
| fn converting_composite_types_compound_unique() { |
- only check for composites in compound indexes - clarify message with respect to the above change tests - added test for nested scalar case
Add test to validate `@@index` case
Druue
force-pushed
the
fix/composites_compound_index
branch
from
7d1b385 to
a58ad02
Compare
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.
Updated the composite type validation introduced here to only throw a validation error when encountering composite types in compound indices.
(Specifically unique indices as I can't reproduce this using either normal or fulltext indices)
fixes prisma/prisma#21441
contributes prisma/prisma#21723
related discovery page: notion