-
-
Notifications
You must be signed in to change notification settings - Fork 94
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
fix(json): false error when using typed json in multi-file schemas #1836
Conversation
📝 WalkthroughWalkthroughThis pull request includes updates to multiple files, primarily focusing on version increments and validation logic adjustments. The version of the JetBrains plugin in Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/integration/tests/enhancements/json/validation.test.ts (1)
4-4
: Update test description to match the actual testThe test description "is only supported by postgres" is inconsistent with the actual test, which verifies that SQLite doesn't support JSON fields.
Consider updating to something like "is not supported by sqlite" or "validates datasource provider support".
packages/schema/src/plugins/prisma/schema-generator.ts (2)
85-85
: Consider adding other databases that support JSON.While PostgreSQL has excellent JSON support, other databases like MySQL and MariaDB also have native JSON capabilities. Consider expanding
PROVIDERS_SUPPORTING_TYPEDEF_FIELDS
to include these providers if they are compatible with your JSON field implementation.
852-857
: LGTM! Clear and effective validation implementation.The validation logic is well-implemented with a descriptive error message. However, since the method only uses the provider information, consider optimizing the parameter:
- private ensureSupportingTypeDefFields(zmodel: Model) { - const dsProvider = getDataSourceProvider(zmodel); + private ensureSupportingTypeDefFields(provider: string | undefined = getDataSourceProvider(this.zmodel)) { if (dsProvider && !PROVIDERS_SUPPORTING_TYPEDEF_FIELDS.includes(dsProvider)) { throw new PluginError(name, `Datasource provider "${dsProvider}" does not support "@json" fields`); } }This change would:
- Make the method more flexible by accepting a provider directly
- Default to the current behavior when no provider is passed
- Reduce the coupling with the Model type
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (13)
package.json
is excluded by!**/*.json
packages/ide/jetbrains/package.json
is excluded by!**/*.json
packages/language/package.json
is excluded by!**/*.json
packages/misc/redwood/package.json
is excluded by!**/*.json
packages/plugins/openapi/package.json
is excluded by!**/*.json
packages/plugins/swr/package.json
is excluded by!**/*.json
packages/plugins/tanstack-query/package.json
is excluded by!**/*.json
packages/plugins/trpc/package.json
is excluded by!**/*.json
packages/runtime/package.json
is excluded by!**/*.json
packages/schema/package.json
is excluded by!**/*.json
packages/sdk/package.json
is excluded by!**/*.json
packages/server/package.json
is excluded by!**/*.json
packages/testtools/package.json
is excluded by!**/*.json
📒 Files selected for processing (4)
packages/ide/jetbrains/build.gradle.kts
(1 hunks)packages/schema/src/language-server/validator/datamodel-validator.ts
(0 hunks)packages/schema/src/plugins/prisma/schema-generator.ts
(4 hunks)tests/integration/tests/enhancements/json/validation.test.ts
(2 hunks)
💤 Files with no reviewable changes (1)
- packages/schema/src/language-server/validator/datamodel-validator.ts
✅ Files skipped from review due to trivial changes (1)
- packages/ide/jetbrains/build.gradle.kts
🔇 Additional comments (5)
tests/integration/tests/enhancements/json/validation.test.ts (3)
1-1
: LGTM: Import statement updated correctly
The addition of loadSchema
import is appropriate for the new test implementation.
Line range hint 21-37
: Verify if second test case needs similar updates
While this test case remains unchanged and appears correct, let's verify if it should also use loadSchema
for consistency with the first test.
#!/bin/bash
# Check for similar validation patterns in other test files
rg -A 5 'loadModelWithError.*@json' tests/
Line range hint 6-19
: LGTM: Appropriate validation approach
The switch to loadSchema
and the updated error message correctly validate the database provider support for JSON fields.
Let's verify if this is the only test affected by this change:
✅ Verification successful
Based on the analysis of the test files, I can now provide a final response:
The JSON field validation is correctly implemented for SQLite provider
The review comment is correct and no other test files need similar updates because:
- The JSON field validation test in
validation.test.ts
specifically tests SQLite's lack of support for JSON fields - Other JSON-related tests in
typing.test.ts
andcrud.test.ts
are intentionally using PostgreSQL as the provider since they test the actual JSON functionality - The
zod.test.ts
file uses PostgreSQL for different plugin-related tests and doesn't involve JSON field validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tests that might need similar updates
rg -l 'only supported by postgres' tests/
Length of output: 44
Script:
#!/bin/bash
# Search for tests involving JSON fields and error messages
rg -A 5 '@json.*provider' tests/
rg -A 5 'provider.*@json' tests/
# Search for any test files containing JSON field tests
rg -l '@json' tests/
Length of output: 890
Script:
#!/bin/bash
# Let's check the content of these files to see if they need similar provider validation
rg -A 5 'loadSchema.*@json|loadModelWithError.*@json' tests/integration/tests/plugins/zod.test.ts tests/integration/tests/enhancements/json/typing.test.ts tests/integration/tests/enhancements/json/crud.test.ts
Length of output: 211
Script:
#!/bin/bash
# Let's check for any JSON field tests with different patterns
rg -B 2 -A 5 '@json.*{' tests/integration/tests/plugins/zod.test.ts tests/integration/tests/enhancements/json/typing.test.ts tests/integration/tests/enhancements/json/crud.test.ts
# Also check for any provider-related tests in these files
rg -B 2 -A 5 'provider|sqlite|postgres' tests/integration/tests/plugins/zod.test.ts tests/integration/tests/enhancements/json/typing.test.ts tests/integration/tests/enhancements/json/crud.test.ts
Length of output: 27895
packages/schema/src/plugins/prisma/schema-generator.ts (2)
799-802
: LGTM! Well-structured JSON field type handling.
The implementation correctly validates provider support before setting the field type to 'Json', ensuring type safety and compatibility.
852-857
: Verify type definition usage across providers.
Let's ensure there are no existing type definitions being used with unsupported providers.
✅ Verification successful
Let me try a different approach to gather the necessary information.
The previous pattern matching with ast-grep didn't yield results, possibly due to the syntax. Let's try with ripgrep to find actual usage patterns in the codebase. This will help us verify:
- Where @JSON attributes are being used
- What providers are configured
- Any type definitions that might be affected
Based on the search results, I can now make a definitive assessment. The code shows:
- The
@json
attribute is used in test files with proper validation that checks for supported providers - There's a specific test that verifies the error "Datasource provider 'sqlite' does not support '@JSON' fields"
- The code in question is part of the validation logic that ensures JSON fields are only used with supported providers
The changes are properly validated through tests and the error handling is consistent with the codebase's requirements. There's no evidence of incorrect usage of type definitions with unsupported providers in the actual codebase, only in controlled test scenarios.
The validation for JSON field support across providers is correctly implemented and tested.
- The error message is properly tested in
tests/integration/tests/enhancements/json/validation.test.ts
- Provider validation is consistently applied across the codebase
- Test cases cover both supported and unsupported provider scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all type definitions and their usage across different providers
# Find all type definitions
echo "Looking for type definition declarations..."
ast-grep --pattern 'type $name {
$$$
}'
# Find all data source declarations with their providers
echo -e "\nChecking datasource providers..."
ast-grep --pattern 'datasource $name {
provider = $provider
$$$
}'
# Find all fields using type definitions
echo -e "\nChecking type definition usage in models..."
ast-grep --pattern 'model $model {
$$$
$field: $type
$$$
}'
Length of output: 465
Script:
#!/bin/bash
# Search for @json attribute usage and type definitions in schema files
echo "Searching for @json attribute usage..."
rg "@json" -A 2 -B 2
echo -e "\nSearching for datasource provider declarations..."
rg "provider\s*=" -A 2 -B 2
echo -e "\nSearching for type definitions..."
rg "^type\s+\w+" -A 5
Length of output: 142989
fixes #1883