-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
merge dev to main (v1.10.1) #1099
Conversation
WalkthroughWalkthroughThis update focuses on enhancing plugin functionality, improving schema generation utilities, and refining the TypeScript expression transformer. It also includes adjustments to testing tools and the addition of new integration tests for both feature enhancements and regression scenarios. Key improvements involve handling specific validation rules more effectively and ensuring better configuration for TypeScript compiler options in tests. The changes aim to streamline development workflows and bolster the robustness of validation and testing mechanisms. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
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 (7)
- packages/ide/jetbrains/build.gradle.kts (1 hunks)
- packages/schema/src/plugins/zod/utils/schema-gen.ts (2 hunks)
- packages/schema/src/utils/typescript-expression-transformer.ts (5 hunks)
- packages/testtools/src/schema.ts (1 hunks)
- tests/integration/tests/cli/plugins.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-policy/field-validation.test.ts (1 hunks)
- tests/integration/tests/regression/issue-1078.test.ts (1 hunks)
Additional comments: 9
packages/ide/jetbrains/build.gradle.kts (1)
- 12-12: The version update from "1.10.0" to "1.10.1" in the
build.gradle.kts
file for the JetBrains IDE plugin is noted. This change indicates a minor version update, which typically involves bug fixes or minor improvements. Ensure that the version number is correctly updated in all relevant documentation and configuration files to maintain consistency across the project.tests/integration/tests/cli/plugins.test.ts (1)
- 119-119: The addition of
skipLibCheck: true
to the TypeScript compiler options in the CLI Plugins Tests is a significant change. This option skips type checking of all declaration files (*.d.ts
). While this can speed up the compilation process, it may also hide potential type errors in third-party libraries that could affect the project. Consider the trade-offs of this configuration carefully, especially for a large project where type safety is critical.
- Evaluate the impact of skipping library checks on the project's type safety and build performance.
- Consider enabling
skipLibCheck
only in specific scenarios where build performance is a priority, and the risks of skipping type checks are understood and acceptable.packages/schema/src/plugins/zod/utils/schema-gen.ts (2)
- 7-7: The import of
isDataModelFieldReference
from@zenstackhq/sdk
is correctly added to support the new validation logic. This import is used to enhance the validation mechanism by checking if a given expression is a reference to a data model field.- 209-218: The modification to the
makeValidationRefinements
function to handleisDataModelFieldReference
is a thoughtful addition. It ensures that if the expression is a simple field reference, undefined values are treated as true in the validation context, which aligns with the optional nature of fields in this context. This change improves the robustness of the validation logic by accommodating field references more gracefully.packages/testtools/src/schema.ts (1)
- 108-108: Setting
preserveTsFiles
totrue
in thezod
plugin configuration is a significant change that affects how TypeScript files are handled during schema generation. This configuration ensures that TypeScript files are retained, which can be beneficial for debugging and understanding the generated code. However, it's important to consider the implications on the build size and the potential for clutter in the generated output.
- Evaluate the impact of preserving TypeScript files on the project's build size and clarity.
- Consider providing configuration options to enable or disable this behavior based on the developer's needs or the build environment.
packages/schema/src/utils/typescript-expression-transformer.ts (3)
- 20-20: The addition of
isDataModelFieldReference
from@zenstackhq/sdk
is noted. This import is crucial for the enhancements made in the validation logic within this file. It's important to ensure that@zenstackhq/sdk
is a dependency in the project'spackage.json
to avoid any import errors.- 261-267: The modification in the
ensureBoolean
method to handleExpressionContext.ValidationRule
differently is a logical change. It treats undefined fields astrue
in a validation context, which aligns with the principle that all fields are optional in such contexts. This change enhances the robustness of the validation logic. However, it's essential to ensure that this behavior is well-documented, as it might not be immediately intuitive to other developers or users of the library.Consider adding documentation or comments explaining the rationale behind treating undefined as
true
in validation contexts to improve code readability and maintainability.
- 309-320: The update in the
unary
method to handle specific cases based on context andisDataModelFieldReference
is a thoughtful addition. It ensures that unary operations involving undefined fields in a validation context are treated as booleantrue
, which is consistent with the changes made in theensureBoolean
method. This consistency in handling undefined values across different contexts is crucial for maintaining the integrity of the validation logic.tests/integration/tests/enhancements/with-policy/field-validation.test.ts (1)
- 613-765: The addition of tests for model-level validation in the context of the With Policy feature is a significant enhancement to the project's testing capabilities. These tests cover a wide range of scenarios, including field validations, optionality, comparisons, complex expressions, and negation, which are crucial for ensuring the robustness and reliability of the validation logic.
It's commendable that the tests are comprehensive, covering various edge cases and ensuring that the validation logic behaves as expected across different scenarios. However, it's essential to ensure that these tests are well-documented, explaining the rationale behind each test case and the specific validation rules being tested. This will improve the maintainability of the test suite and make it easier for other developers to understand the purpose of each test.
Consider adding comments or documentation within the test file to explain the rationale behind each test case and the specific validation rules being tested. This will enhance the maintainability and readability of the test suite.
if (this.options.context === ExpressionContext.ValidationRule) { | ||
// in a validation context, we treat binary involving undefined as boolean true | ||
if (isDataModelFieldReference(expr.left)) { | ||
_default = `(${left} !== undefined ? (${_default}): true)`; | ||
} | ||
if (isDataModelFieldReference(expr.right)) { | ||
_default = `(${right} !== undefined ? (${_default}): true)`; | ||
} | ||
} |
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.
The adjustments in the binary
method to handle binary expressions based on ExpressionContext.ValidationRule
and isDataModelFieldReference
are well thought out. These changes ensure that binary operations involving undefined fields in a validation context are treated as boolean true
. This consistent approach to handling undefined values in validation contexts across different expression types is commendable. However, it's important to ensure comprehensive testing around these changes to validate their correctness in various scenarios.
Ensure comprehensive testing is conducted around the changes in the binary
method to validate their correctness across various scenarios, particularly focusing on edge cases involving undefined values in validation contexts.
describe('issue 1078', () => { | ||
it('regression', async () => { | ||
const { prisma, enhance } = await loadSchema( | ||
` | ||
model Counter { | ||
id String @id | ||
|
||
name String | ||
value Int | ||
|
||
@@validate(value >= 0) | ||
@@allow('all', true) | ||
} | ||
` | ||
); | ||
|
||
const db = enhance(); | ||
|
||
const counter = await db.counter.create({ | ||
data: { id: '1', name: 'It should create', value: 1 }, | ||
}); | ||
|
||
//! This query fails validation | ||
const updated = await db.counter.update({ | ||
where: { id: '1' }, | ||
data: { name: 'It should update' }, | ||
}); | ||
}); |
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.
In the test case titled 'regression', there are a few points to consider:
-
Validation Logic: The comment on line 25 indicates that the subsequent query is expected to fail validation. However, there's no assertion or expectation that verifies this behavior. It's important to explicitly test for expected failures to ensure the validation logic is working as intended.
-
Async/Await Usage: The test case makes use of
async/await
for asynchronous operations, which is good practice. However, ensure that all asynchronous calls within the test are properly awaited to avoid flaky tests. -
Resource Cleanup: The test creates a database entry but does not include any cleanup logic. Consider adding an
afterEach
block to clean up any resources created during the test to prevent side effects on subsequent tests. -
Add an assertion to verify that the update query fails as expected.
-
Ensure all asynchronous operations are awaited.
-
Implement resource cleanup to maintain test isolation.
it('read', async () => { | ||
const { prisma, enhance } = await loadSchema( | ||
` | ||
model Post { | ||
id Int @id() @default(autoincrement()) | ||
title String @allow('read', true, true) | ||
content String | ||
} | ||
`, | ||
{ logPrismaQuery: true } | ||
); | ||
|
||
const db = enhance(); | ||
|
||
const post = await prisma.post.create({ data: { title: 'Post1', content: 'Content' } }); | ||
await expect(db.post.findUnique({ where: { id: post.id } })).toResolveNull(); | ||
await expect(db.post.findUnique({ where: { id: post.id }, select: { title: true } })).resolves.toEqual({ | ||
title: 'Post1', | ||
}); | ||
}); |
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.
In the test case titled 'read', the following observations and recommendations are made:
-
Expectation Clarity: The use of
toResolveNull
on line 47 is not a standard Jest matcher and might be confusing or lead to errors if not properly implemented. Ensure that custom matchers are clearly documented and tested. -
Database Interaction: This test case interacts with the database by creating a
post
entity and then attempting to find it. It's good practice to isolate test cases from each other by cleaning up any created entities in anafterEach
block. -
Test Description: The test description 'read' is somewhat vague. Consider using a more descriptive title that clearly states the intent of the test, such as 'should only allow reading posts with proper permissions'.
-
Verify the implementation of
toResolveNull
or consider using a more standard Jest matcher for clarity. -
Add cleanup logic to remove created entities after the test.
-
Update the test description to more accurately reflect the test's purpose.
|
||
plugin zod { | ||
provider = '@core/zod' | ||
// preserveTsFiles = true | ||
preserveTsFiles = true | ||
modelOnly = ${!options.fullZod} | ||
} | ||
`; |
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.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [40-40]
The use of execSync
with command concatenation in the run
function could potentially lead to command injection vulnerabilities if user-controlled input is passed to this function. Ensure that all inputs to this function are properly sanitized or validated to prevent malicious command execution.
- Implement input validation or sanitization for the
cmd
parameter in therun
function to mitigate the risk of command injection.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [64-64]
The use of path.join
with potentially user-controlled input in functions like getWorkspaceRoot
and getWorkspaceNpmCacheFolder
could lead to path traversal vulnerabilities. It's crucial to ensure that any user input is properly sanitized or validated before being used in file path operations.
- Sanitize or validate user input before using it in
path.join
orpath.resolve
functions to prevent path traversal vulnerabilities.
Also applies to: 75-75
Summary by CodeRabbit