feature(psl): Support ignore directives on values in enums#5648
feature(psl): Support ignore directives on values in enums#5648Aijeyomah wants to merge 11 commits intoprisma:mainfrom
Conversation
|
@jacek-prisma @jkomyno, can you help review this PR? Thanks! |
CodSpeed Performance ReportMerging #5648 will not alter performanceComparing Summary
|
|
@aqrln can you help review this PR? Also, I got the authorization error when I tried pushing the integration branch after following the steps(Community PRs: create a local branch for a branch coming from a fork) |
I think this specific section is meant for the maintainers — sorry for the confusion. I have approved the CI workflow, and I pushed an integration branch: #5659. If the build succeeds, an automated integration PR will be automatically opened on Please let me know if/when the integration branch needs to be updated. I won't have time to review the PR today, I'll have a look tomorrow. Thank you for contributing so actively! |
|
@aqrln Thanks for creating the integration branch earlier 5659. After this branch was created and some of the checks were run (not all passed). I couldn't find any helpful resource on how to test the engines on Yesterday, I tried testing the changes by switching to the automated integration branch on locally on Primsa mono-repo and running |
|
@Aijeyomah hmm, it looks like something is wrong with the automation. I triggered an integration release manually now: https://github.com/prisma/prisma-engines/actions/runs/18876278132 |
|
Some tests are failing there because your branch is not up-to-date with |
@aqrln I was almost excited until just one test failed. 28405 which cancelled the release to dev/integration npm. I really appreciate your help so far, but can you help look into the test here. Thanks! |
|
@Aijeyomah that's a flaky test, ignore it. It's not related to your changes. I see there are also some network-related issues on CI in this PR, I can restart the failed jobs after the in progress ones finish. |
|
@Aijeyomah I also just restarted the cancelled release job, not sure why exactly it was cancelled. We have a huge number of CI jobs in progress right now because of the Prisma 7 work, and it looks like GitHub might be killing some of them because we might be running into some limits. |
|
@aqrln I've tested my changes and it works as expected. Thanks for helping out |
|
@aqrln @jacek-prisma I have tested this well on the Prisma monorepo, and it works as expected. Please, can you review? thanks |
Signed-off-by: Eti Ijeoma <ijayeti@gmail.com>
Signed-off-by: Eti Ijeoma <ijayeti@gmail.com>
Signed-off-by: Eti Ijeoma <ijayeti@gmail.com>
…a#5683) Remove the `url` property from `datasource` blocks in PSL, refactor the code to handle URLs separately and update the tests. Ref: https://linear.app/prisma-company/issue/TML-1545/remove-url-datasource-attribute-from-psl /prisma-branch next
Signed-off-by: Eti Ijeoma <ijayeti@gmail.com>
Signed-off-by: Eti Ijeoma <ijayeti@gmail.com>
Aijeyomah
force-pushed
the
feature/support-enum-ignore-directive
branch
from
364b22c to
9b338a2
Compare
Signed-off-by: Eti Ijeoma <ijayeti@gmail.com>
Signed-off-by: Eti Ijeoma <ijayeti@gmail.com>
|
WalkthroughThe changes add support for the Changes
Possibly related issues
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
schema-engine/datamodel-renderer/src/datamodel/enumerator.rs (1)
232-282: 🧹 Nitpick | 🔵 TrivialConsider adding a test case for
@ignorerendering.The existing
kitchen_sinktest covers@mapbut doesn't exercise the new@ignorefunctionality on enum variants.🧪 Suggested test addition
let mut variant = EnumVariant::new("Invalid".into()); variant.comment_out(); r#enum.push_variant(variant); r#enum.push_variant("Black"); + + let mut ignored_variant = EnumVariant::new("Ignored".into()); + ignored_variant.ignore(); + r#enum.push_variant(ignored_variant); r#enum.schema("city_planning");And update the expected output accordingly.
query-compiler/core/src/query_document/parser.rs (1)
209-233:⚠️ Potential issue | 🟠 MajorDefault argument values are no longer applied for omitted optional field arguments.
Line 210 says defaults should be processed, but Line 227 now returns
Nonefor all missing non-required arguments. That drops schema defaults and can change query behavior.💡 Suggested fix
- None if !input_field.is_required() => None, + None if !input_field.is_required() => input_field + .default_value + .as_ref() + .and_then(|default_value| default_value.get()) + .map(|default_value| { + self.parse_input_value( + selection_path.clone(), + argument_path, + default_value.into(), + input_field.field_types(), + query_schema, + input_field.is_parameterizable(), + ) + .map(|value| ParsedArgument { + name: input_field.name.clone().into_owned(), + value, + }) + }),
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
psl/parser-database/src/attributes.rspsl/parser-database/src/types.rspsl/parser-database/src/walkers/enum.rspsl/psl/Cargo.tomlpsl/psl/tests/attributes/ignore_positive.rspsl/psl/tests/common/asserts.rsquery-compiler/core/src/query_document/parser.rsquery-compiler/dmmf/src/ast_builders/datamodel_ast_builder.rsschema-engine/datamodel-renderer/src/datamodel/enumerator.rs
| /// @ignore on enum values. | ||
| pub(super) ignored_values: std::collections::HashSet<EnumValueId>, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using FxHashSet for consistency and performance.
The file uses rustc_hash::FxHashMap (aliased as HashMap) for other collections like mapped_values, but ignored_values uses std::collections::HashSet. For small integer keys like EnumValueId, FxHashSet provides better performance and maintains consistency with the rest of the codebase.
♻️ Suggested change
+use rustc_hash::FxHashSet;
// ... in EnumAttributes struct:
/// `@ignore` on enum values.
- pub(super) ignored_values: std::collections::HashSet<EnumValueId>,
+ pub(super) ignored_values: FxHashSet<EnumValueId>,Note: You'll need to add FxHashSet to the imports at the top of the file alongside FxHashMap.
|
|
||
| #[track_caller] | ||
| fn assert_has_enum(&'a self, name: &str) -> walkers::EnumWalker<'a> { | ||
| self.db | ||
| .walk_enums() | ||
| .find(|e| e.name() == name) | ||
| .expect("Enum {name} not found") | ||
| } |
There was a problem hiding this comment.
Fix the expect() message formatting.
The error message uses {name} as a literal string rather than interpolating the variable. This should use a format string.
🐛 Proposed fix
#[track_caller]
fn assert_has_enum(&'a self, name: &str) -> walkers::EnumWalker<'a> {
self.db
.walk_enums()
.find(|e| e.name() == name)
- .expect("Enum {name} not found")
+ .unwrap_or_else(|| panic!("Enum {name} not found"))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[track_caller] | |
| fn assert_has_enum(&'a self, name: &str) -> walkers::EnumWalker<'a> { | |
| self.db | |
| .walk_enums() | |
| .find(|e| e.name() == name) | |
| .expect("Enum {name} not found") | |
| } | |
| #[track_caller] | |
| fn assert_has_enum(&'a self, name: &str) -> walkers::EnumWalker<'a> { | |
| self.db | |
| .walk_enums() | |
| .find(|e| e.name() == name) | |
| .unwrap_or_else(|| panic!("Enum {} not found", name)) | |
| } |
| pub(crate) trait EnumAssert<'a> { | ||
| fn assert_has_value(&'a self, name: &str) -> walkers::EnumValueWalker<'a>; | ||
| } | ||
|
|
||
| impl<'a> EnumAssert<'a> for walkers::EnumWalker<'a> { | ||
| #[track_caller] | ||
| fn assert_has_value(&'a self, name: &str) -> walkers::EnumValueWalker<'a> { | ||
| self.values() | ||
| .find(|v| v.name() == name) | ||
| .expect("Enum value {name} not found") | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix the expect() message formatting.
Same issue as assert_has_enum - the error message doesn't interpolate {name}.
🐛 Proposed fix
#[track_caller]
fn assert_has_value(&'a self, name: &str) -> walkers::EnumValueWalker<'a> {
self.values()
.find(|v| v.name() == name)
- .expect("Enum value {name} not found")
+ .unwrap_or_else(|| panic!("Enum value {name} not found"))
}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@query-compiler/dmmf/src/ast_builders/datamodel_ast_builder.rs`:
- Around line 50-55: Refactor the enum value loop in enum_to_dmmf to use
iterator adapters: replace the for/if/continue pattern over en.values() with
en.values().filter(|v| !v.is_ignored()).map(enum_value_to_dmmf).collect() so
Enum.values is built declaratively; update the Enum construction in enum_to_dmmf
to assign values via that filtered map and ensure db_name and documentation are
preserved as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5fdbb47c-24fa-489a-8f82-aaeedbe7f360
📒 Files selected for processing (1)
query-compiler/dmmf/src/ast_builders/datamodel_ast_builder.rs
| for enum_value in en.values() { | ||
| if enum_value.is_ignored() { | ||
| continue; | ||
| } | ||
| enm.values.push(enum_value_to_dmmf(enum_value)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for test coverage of ignored enum values in DMMF rendering
# Look for `@ignore` on enum values in test files
echo "=== Checking DMMF test files for `@ignore` on enum values ==="
fd -e prisma . query-compiler/dmmf/test_files --exec grep -l '@ignore' {} \; 2>/dev/null || echo "No `@ignore` found in DMMF test files"
# Show any matches in detail
echo ""
echo "=== Detailed matches ==="
rg -n '@ignore' query-compiler/dmmf/test_files/ 2>/dev/null || echo "No matches"
# Also check for enum-related ignore tests in PSL tests
echo ""
echo "=== PSL ignore tests for enum values ==="
rg -n -A5 'ignore.*enum|enum.*ignore' psl/psl/tests/attributes/ --type rust 2>/dev/null | head -50Repository: prisma/prisma-engines
Length of output: 1202
Consider using .filter() for consistency with existing patterns.
The change correctly excludes ignored enum values from DMMF output. However, other filtering operations in this file use the .filter() iterator method (e.g., lines 28-29, 75, 141), whereas this uses an if/continue pattern.
♻️ Suggested refactor using `.filter()`
- for enum_value in en.values() {
- if enum_value.is_ignored() {
- continue;
- }
+ for enum_value in en.values().filter(|v| !v.is_ignored()) {
enm.values.push(enum_value_to_dmmf(enum_value));
}Alternatively, restructure the entire function to match the more declarative style used elsewhere:
fn enum_to_dmmf(en: walkers::EnumWalker<'_>) -> Enum {
Enum {
name: en.name().to_owned(),
values: en
.values()
.filter(|v| !v.is_ignored())
.map(enum_value_to_dmmf)
.collect(),
db_name: en.mapped_name().map(ToOwned::to_owned),
documentation: en.ast_enum().documentation().map(ToOwned::to_owned),
}
}Test coverage exists in PSL tests (psl/psl/tests/attributes/ignore_positive.rs::allow_ignore_on_enum_values) validating that enum values with @ignore are handled correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for enum_value in en.values() { | |
| if enum_value.is_ignored() { | |
| continue; | |
| } | |
| enm.values.push(enum_value_to_dmmf(enum_value)); | |
| } | |
| for enum_value in en.values().filter(|v| !v.is_ignored()) { | |
| enm.values.push(enum_value_to_dmmf(enum_value)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@query-compiler/dmmf/src/ast_builders/datamodel_ast_builder.rs` around lines
50 - 55, Refactor the enum value loop in enum_to_dmmf to use iterator adapters:
replace the for/if/continue pattern over en.values() with en.values().filter(|v|
!v.is_ignored()).map(enum_value_to_dmmf).collect() so Enum.values is built
declaratively; update the Enum construction in enum_to_dmmf to assign values via
that filtered map and ensure db_name and documentation are preserved as before.
3 participants
Description
@ignoredirectives on values in enums typespsl/psl/Cargo.tomlto enable allpsl-corefeatures for the test environment to resolve the unresolved import errors duringcargo test -p pslNote to reviewer
The issue raised included adding support for the
@deprecateddirective, but that work is out of scope for this pull request. I plan to open a separate PR to handle the@deprecateddirective.Closes #27911
Summary by CodeRabbit