merge dev to main (v1.10.0) #1069
Conversation
…e but no model-level policy (#1052)
Co-authored-by: Yiming <[email protected]>
WalkthroughWalkthroughThe updates span across various components, focusing on enhancing test scaffolding, improving error handling, and refining plugin functionalities. Key adjustments include the introduction of a new test command and the addition of a directory to Changes
This table groups changes by their nature and impact area, offering a concise view of the modifications across the project. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 21
Configuration used: CodeRabbit UI
Files ignored due to path filters (17)
package.jsonis excluded by:!**/*.jsonpackages/ide/jetbrains/package.jsonis excluded by:!**/*.jsonpackages/language/package.jsonis excluded by:!**/*.jsonpackages/misc/redwood/package.jsonis excluded by:!**/*.jsonpackages/plugins/openapi/package.jsonis excluded by:!**/*.jsonpackages/plugins/swr/package.jsonis excluded by:!**/*.jsonpackages/plugins/tanstack-query/package.jsonis excluded by:!**/*.jsonpackages/plugins/trpc/package.jsonis excluded by:!**/*.jsonpackages/runtime/package.jsonis excluded by:!**/*.jsonpackages/schema/package.jsonis excluded by:!**/*.jsonpackages/sdk/package.jsonis excluded by:!**/*.jsonpackages/server/package.jsonis excluded by:!**/*.jsonpackages/testtools/package.jsonis excluded by:!**/*.jsonpackages/testtools/src/package.template.jsonis excluded by:!**/*.jsonpnpm-lock.yamlis excluded by:!**/*.yamltests/integration/package.jsonis excluded by:!**/*.jsontests/integration/tests/tsconfig.template.jsonis excluded by:!**/*.json
Files selected for processing (48)
- .github/workflows/build-test.yml (1 hunks)
- .gitignore (1 hunks)
- CONTRIBUTING.md (1 hunks)
- jest.config.ts (1 hunks)
- packages/ide/jetbrains/CHANGELOG.md (1 hunks)
- packages/ide/jetbrains/build.gradle.kts (1 hunks)
- packages/plugins/tanstack-query/src/generator.ts (3 hunks)
- packages/plugins/tanstack-query/src/runtime-v5/index.ts (1 hunks)
- packages/plugins/tanstack-query/src/runtime-v5/react.ts (3 hunks)
- packages/plugins/tanstack-query/src/runtime-v5/svelte.ts (2 hunks)
- packages/plugins/tanstack-query/src/runtime/common.ts (2 hunks)
- packages/plugins/tanstack-query/src/runtime/index.ts (1 hunks)
- packages/plugins/tanstack-query/src/runtime/react.ts (2 hunks)
- packages/plugins/tanstack-query/src/runtime/svelte.ts (2 hunks)
- packages/plugins/tanstack-query/src/runtime/vue.ts (5 hunks)
- packages/runtime/src/enhancements/policy/handler.ts (7 hunks)
- packages/runtime/src/enhancements/policy/policy-utils.ts (4 hunks)
- packages/schema/src/cli/actions/repl.ts (2 hunks)
- packages/schema/src/cli/cli-util.ts (2 hunks)
- packages/schema/src/language-server/validator/datamodel-validator.ts (5 hunks)
- packages/schema/src/language-server/validator/schema-validator.ts (1 hunks)
- packages/schema/src/plugins/prisma/schema-generator.ts (4 hunks)
- packages/schema/src/plugins/zod/utils/schema-gen.ts (1 hunks)
- packages/schema/src/res/stdlib.zmodel (2 hunks)
- packages/schema/src/utils/exec-utils.ts (1 hunks)
- packages/schema/src/utils/pkg-utils.ts (3 hunks)
- packages/schema/tests/schema/stdlib.test.ts (1 hunks)
- packages/schema/tests/schema/validation/attribute-validation.test.ts (2 hunks)
- packages/schema/tests/schema/validation/datamodel-validation.test.ts (17 hunks)
- packages/schema/tests/schema/validation/datasource-validation.test.ts (2 hunks)
- packages/schema/tests/schema/validation/schema-validation.test.ts (1 hunks)
- packages/schema/tests/utils.ts (4 hunks)
- packages/server/tests/api/rest.test.ts (3 hunks)
- packages/server/tests/utils.ts (1 hunks)
- packages/testtools/src/schema.ts (5 hunks)
- script/set-test-env.ts (1 hunks)
- script/test-global-setup.ts (1 hunks)
- script/test-scaffold.ts (1 hunks)
- tests/integration/jest.config.ts (1 hunks)
- tests/integration/tests/cli/generate.test.ts (2 hunks)
- tests/integration/tests/cli/plugins.test.ts (2 hunks)
- tests/integration/tests/enhancements/with-policy/field-validation.test.ts (2 hunks)
- tests/integration/tests/enhancements/with-policy/refactor.test.ts (14 hunks)
- tests/integration/tests/plugins/zod.test.ts (1 hunks)
- tests/integration/tests/regression/issue-1014.test.ts (1 hunks)
- tests/integration/tests/regression/issue-177.test.ts (1 hunks)
- tests/integration/tests/schema/refactor-pg.zmodel (3 hunks)
- tests/integration/tests/schema/todo.zmodel (1 hunks)
Files skipped from review due to trivial changes (2)
- packages/ide/jetbrains/build.gradle.kts
- script/set-test-env.ts
Additional comments: 94
.gitignore (1)
- 10-10: The addition of
.testto the.gitignorefile is a good practice to prevent tracking of test artifacts or temporary files that do not need to be version controlled.packages/plugins/tanstack-query/src/runtime/index.ts (1)
- 2-2: The addition of
QueryErrorto the exports alongsideFetchFnandgetQueryKeyis a positive change, likely enhancing error handling capabilities. Ensure that documentation is updated to reflect these new exports and their usage.packages/plugins/tanstack-query/src/runtime-v5/index.ts (1)
- 2-2: The addition of
QueryErrorto the exports, mirroring the change in the non-versioned runtime file, is a good practice for maintaining API consistency across versions. Ensure that documentation for version 5 of the runtime is also updated to include these new exports and their intended usage.tests/integration/jest.config.ts (1)
- 1-8: Refactoring the Jest configuration to import and spread a base configuration is a good practice for maintaining consistency and reducing duplication across test setups. Ensure that any necessary overrides are correctly applied in this and other configuration files that use the base configuration.
script/test-global-setup.ts (1)
- 1-9: Introducing a global setup script that checks for the existence of necessary test scaffolding is a good practice to ensure the test environment is correctly prepared. Consider adding documentation or comments to guide developers on resolving the error if the test scaffold is not found.
packages/ide/jetbrains/CHANGELOG.md (1)
- 4-7: The updates to the CHANGELOG.md file accurately reflect significant improvements and fixes, including support for complex
@@indexusage and the resolution of ZModel validation issues. These changes are important for users to be aware of and demonstrate continuous improvement of the JetBrains IDE plugin.packages/server/tests/utils.ts (1)
- 23-23: Adding the
publishedAtfield of typeDateTimeto thePostmodel is a useful enhancement for tracking publication dates. Ensure that this new field is correctly integrated and handled in all relevant parts of the application.tests/integration/tests/regression/issue-177.test.ts (1)
- 3-27: The regression test case related to optional and non-optional fields in a relation is valuable for ensuring the stability and correctness of the system's error handling. Ensure that the error message expected by the test remains consistent with the system's error handling and messaging strategy.
packages/schema/tests/schema/stdlib.test.ts (1)
- 27-27: Modifying the error handling logic to throw
SchemaLoadingErrorwithvalidationErrorsdirectly is a positive change for improving error reporting. Ensure that the error handling logic and any dependent code are updated to handle the new error structure appropriately.jest.config.ts (1)
- 6-20: Enhancing the Jest configuration with
globalSetup,setupFiles, and a dynamically resolvedcoverageDirectorypath improves test setup and coverage reporting. Ensure that these configurations are correctly implemented and do not introduce issues with existing tests.tests/integration/tests/regression/issue-1014.test.ts (1)
- 3-51: The test case addressing a regression related to permissions and access control adds important coverage to ensure the security and correctness of the application. Ensure that the test setup and assertions accurately reflect the intended permissions and that any changes to permissions logic are accompanied by updates to this and similar tests.
packages/schema/tests/schema/validation/schema-validation.test.ts (1)
- 42-54: Adding a new test case to check for errors when importing a non-existing model file with a specific extension enhances the robustness of schema validation. Ensure that the schema validation logic correctly handles file extensions and provides clear error messages for missing or incorrect imports.
packages/schema/tests/utils.ts (6)
- 10-10: The introduction of the
Errorishtype is a good practice for handling different error formats. It enhances flexibility in error handling by accommodating bothErrorobjects and custom error-like objects.- 12-24: The refactoring of the
SchemaLoadingErrorclass to accept an array ofErrorishtypes and handle them accordingly is a significant improvement. It allows for more detailed error reporting by aggregating multiple errors. However, ensure that the consumer of this class is prepared to handle an array of errors instead of a single error.Verification successful
The refactoring of the
SchemaLoadingErrorclass to accept an array ofErrorishtypes and the consumer code's preparedness to handle an array of errors have been verified through the examination of the class's usage across the codebase. All instances ofSchemaLoadingErrorbeing instantiated involve passing an array of errors, aligning with the refactoring's intent for more detailed error reporting.* 38-38: The updates to the `loadModel` function to pass errors directly to `SchemaLoadingError` are appropriate and leverage the new class structure effectively. This change ensures that detailed error information is preserved and passed up the call stack.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all usages of SchemaLoadingError properly handle an array of errors. rg "new SchemaLoadingError" --type tsLength of output: 780
Also applies to: 42-42, 61-61
- 80-80: The change in
loadModelWithErrorto return the error message instead of the errors array simplifies error handling for callers that only need a summary of what went wrong. However, this might limit the ability to programmatically respond to specific errors. Consider if this aligns with the intended use cases.Verification successful
The simplification in error handling by
loadModelWithErrorto return the error message instead of an errors array seems to align with its primary use cases in testing scenarios, where the function is extensively used. This change likely simplifies error handling for callers, particularly in tests where the focus is on the presence of an error rather than on detailed error analysis. However, without detailed insight into each test's assertions or requirements, this conclusion is made with a cautious understanding of the function's usage context.* 85-89: The addition of the `safelyLoadModel` function is a valuable enhancement for asynchronous model loading with error handling. It uses `Promise.allSettled` to gracefully handle errors, which is a modern and effective approach. Ensure that this function is utilized in scenarios where non-blocking model loading is required. * 91-95: The `errorLike` constant is a useful utility for creating error-like objects in tests. It leverages `expect.stringContaining` to allow for flexible error message matching, which can improve the robustness of tests.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the simplification in error handling by `loadModelWithError` aligns with its use cases. rg "loadModelWithError" --type tsLength of output: 10172
CONTRIBUTING.md (1)
- 38-45: Adding a step for scaffolding the project used for testing in the contribution guide is a helpful update. It ensures that contributors are aware of the necessary steps to prepare the project for testing, which can lead to more consistent and reliable contributions. Make sure this step is clear and provides enough detail for new contributors to follow easily.
tests/integration/tests/schema/todo.zmodel (1)
- 17-17: Commenting out the
preserveTsFilessetting in thezodplugin configuration may have implications on how TypeScript files are generated or preserved during testing. Ensure that this change aligns with the intended behavior for TypeScript file handling in the testing environment. It might be useful to document the reason for this change to provide context for future contributors.Verification successful
The search confirmed the presence of the commented-out
preserveTsFilessetting within thetodo.zmodelfile, accurately reflecting the change mentioned in the review comment. This change is specific to thetodo.zmodelfile and suggests a modification in how TypeScript files might be handled during testing. Given the context, it's recommended to manually verify the impact of this change on TypeScript file handling in the testing environment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the impact of commenting out preserveTsFiles on TypeScript file handling in tests. rg "preserveTsFiles" --type zmodelLength of output: 65
Script:
#!/bin/bash # Correctly verify the impact of commenting out preserveTsFiles on TypeScript file handling in tests by searching in .zmodel files. rg "preserveTsFiles" -g "*.zmodel"Length of output: 107
packages/schema/src/utils/pkg-utils.ts (2)
- 7-37: The modifications to the
findUpfunction, including the introduction of a generic type parametereto control the return type based on whether multiple paths are allowed, significantly enhance its flexibility and utility. This change allows for more precise control over the search behavior and the type of results returned. Ensure that existing usages offindUpare updated to reflect these changes if necessary.- 108-112: Adding a deprecation notice for
findPackageJsonand recommending the use offindUpinstead is a good practice for guiding developers towards more versatile and updated utilities. Ensure that all references tofindPackageJsonwithin the project are updated to usefindUp, and consider removingfindPackageJsonin a future release to reduce maintenance overhead.Verification successful
Given the current findings, it's necessary to further investigate if the
findPackageJsonfunction is being called or referenced anywhere in the project outside of its definition. To do this, I will run a script that searches forfindPackageJsonusage, excluding the file where it's defined.
The verification process did not find any references or calls to
findPackageJsonoutside of its definition inpackages/schema/src/utils/pkg-utils.ts. This indicates that there are no remaining references to update tofindUp, aligning with the recommendation in the review comment. Therefore, it can be concluded that the review comment has been addressed appropriately.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all references to findPackageJson are updated to use findUp. rg "findPackageJson" --type tsLength of output: 124
Script:
#!/bin/bash # Search for calls to findPackageJson excluding its definition in pkg-utils.ts rg "findPackageJson" --type ts --glob '!packages/schema/src/utils/pkg-utils.ts'Length of output: 79
packages/plugins/tanstack-query/src/runtime/react.ts (2)
- 12-25: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-22]
Reordering imports and type declarations improves readability and consistency in the file. It's a good practice to group imports from external libraries together and separate them from type imports and internal utilities. This change enhances the overall structure of the file.
- 113-124: The refactoring of the
useModelMutationfunction signature, including the addition of type parameters and adjustments to theoptionsparameter, increases the function's flexibility and type safety. This change allows for more precise control over the types used in mutations and their options. Ensure that all usages ofuseModelMutationare updated to reflect these changes.Verification successful
To further verify the update of
useModelMutationusages according to the new function signature, we'll examine a few representative usages across different categories:
- Test files in the
tanstack-queryplugin.- The generator file in the
tanstack-queryplugin.- A usage in the Vue or Svelte runtime files, if they share a similar API design with the React runtime.
This will provide a comprehensive overview of whether the changes have been consistently applied across the codebase.
The examination of the
useModelMutationusages in test files, generator logic, and Vue runtime file confirms that the updates to the function signature have been correctly implemented across the codebase. This includes the addition of themodelMetaparameter and adjustments to theoptionsparameter, aligning with the changes described in the review comment. Therefore, the refactoring has been successfully verified.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all usages of useModelMutation are updated to reflect the new function signature. rg "useModelMutation" --type tsLength of output: 5676
Script:
#!/bin/bash # Examine a representative usage of useModelMutation in a test file cat packages/plugins/tanstack-query/tests/react-hooks.test.tsx # Examine the generator file to see how useModelMutation is being dynamically called cat packages/plugins/tanstack-query/src/generator.ts # Check a usage in the Vue runtime file as a representative of non-React runtimes cat packages/plugins/tanstack-query/src/runtime/vue.tsLength of output: 50322
packages/plugins/tanstack-query/src/runtime/svelte.ts (1)
- 112-123: The updates to the
useModelMutationfunction signature in the Svelte context, similar to the React context, enhance type safety and flexibility. This change allows for a more precise definition of the function's behavior and its return type based on the provided parameters. Ensure that all Svelte components usinguseModelMutationare updated accordingly.packages/plugins/tanstack-query/src/runtime/vue.ts (5)
- 23-23: The
FetchFntype is imported here, which is good for maintaining type safety across the API. It's important to ensure that this type accurately represents the fetch function's signature used throughout the file.- 64-64: The modification to exclude the
queryKeyfrom theUseQueryOptionsin theuseModelQueryfunction is a positive change. It simplifies the API for the end-users by automatically handling thequeryKeyinternally, reducing the potential for errors and improving usability.- 90-90: Similar to the
useModelQueryfunction, excluding thequeryKeyfrom theUseInfiniteQueryOptionsin theuseInfiniteModelQueryfunction is a beneficial change. It streamlines the function's usage by abstracting away the complexity of managingqueryKey, enhancing the developer experience.- 116-127: The update to the
useModelMutationfunction, including the addition of type parameters (TArgs,TError,R,C,Result) and the adjustment of theoptionsparameter type, significantly improves the function's flexibility and type safety. This change allows for more precise type checking and better support for TypeScript users.- 177-177: The return statement of the
useModelMutationfunction correctly utilizes theuseMutationhook with the refined options. This ensures that the mutation functionality is consistent with the updated type definitions and function signature, maintaining the integrity of the API.packages/plugins/tanstack-query/src/runtime-v5/svelte.ts (2)
- 25-25: The addition of the
FetchFntype declaration is a positive step towards ensuring type safety and consistency across the API. It's crucial that this type accurately reflects the fetch function's signature used within the Svelte integration.- 150-161: The update to the
useModelMutationfunction, including the addition of type parameters (TArgs,TError,R,C,Result) and the adjustment of theoptionsparameter type, is a significant improvement. It enhances the function's flexibility and type safety, allowing for more precise type checking and better support for TypeScript users within the Svelte context.packages/schema/src/cli/actions/repl.ts (2)
- 13-16: The addition of a check for the terminal type before initializing the REPL session is a thoughtful enhancement. It improves the user experience by providing a clear error message when the REPL is not supported in the current environment, such as when not running in a TTY terminal with Bun.
- 18-18: Moving the import of
pretty-replinside thereplfunction is a good practice for optimizing the startup time of the CLI tool. This lazy loading approach ensures that the module is only imported when necessary, which can lead to performance improvements.tests/integration/tests/cli/generate.test.ts (2)
- 4-4: The import of the
installPackagefunction from@zenstackhq/testtoolsis a positive change. It abstracts the logic for installing npm packages, which can lead to cleaner, more maintainable test code by reducing redundancy and improving readability.- 46-47: Replacing direct
execSynccalls with theinstallPackagefunction for installing npm packages is an excellent improvement. This change enhances modularity and abstraction in package installation within the CLI generate command tests, making the tests more robust and easier to maintain.packages/plugins/tanstack-query/src/runtime-v5/react.ts (3)
- 3-4: The removal of
UseSuspenseInfiniteQueryOptionsandUseSuspenseQueryOptionsfrom the list of imports indicates that these types are no longer used in this file. It's important to ensure that the removal of these imports does not affect any existing functionality or type safety within the file.Verification successful
The removal of
UseSuspenseInfiniteQueryOptionsandUseSuspenseQueryOptionsfrom thereact.tsfile has been verified, and it's confirmed that these types are not used elsewhere in the file. This ensures that their removal does not affect any existing functionality or type safety within the file.* 27-27: The addition of the `FetchFn` type declaration is a positive step towards ensuring type safety and consistency across the API. It's crucial that this type accurately reflects the fetch function's signature used within the React integration. * 170-181: The update to the `useModelMutation` function, including the addition of type parameters (`TArgs`, `TError`, `R`, `C`, `Result`) and the adjustment of the `options` parameter type, is a significant improvement. It enhances the function's flexibility and type safety, allowing for more precise type checking and better support for TypeScript users within the React context.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that UseSuspenseInfiniteQueryOptions and UseSuspenseQueryOptions are not used elsewhere in the file. grep -q 'UseSuspenseInfiniteQueryOptions\|UseSuspenseQueryOptions' src/runtime-v5/react.ts && echo "Found usage of removed imports." || echo "No usage of removed imports found."Length of output: 228
Script:
#!/bin/bash # Find the correct path for the react.ts file fd react.ts --exec grep -q 'UseSuspenseInfiniteQueryOptions\|UseSuspenseQueryOptions' {} \; -x echo "{}: No usage of removed imports found." || echo "File not found."Length of output: 280
tests/integration/tests/cli/plugins.test.ts (1)
- 97-98: The replacement of direct npm commands with the
installPackagefunction calls is a significant improvement in terms of code modularity and abstraction. This change allows for more flexible package management and can potentially simplify the process of switching between different package managers in the future. However, ensure that theinstallPackagefunction is thoroughly tested, especially for edge cases such as handling installation failures, dealing with package version conflicts, and working in different environments (CI, local development, etc.).packages/schema/src/plugins/zod/utils/schema-gen.ts (1)
- 177-177: Changing the schema generation for 'DateTime' fields from
z.date()toz.coerce.date()introduces automatic coercion of date inputs. This can be beneficial for handling a wider range of date input formats, improving the developer experience by reducing the need for manual date parsing and validation. However, it's important to ensure that this change aligns with the overall validation strategy of the application. Specifically, consider whether automatic coercion could lead to unexpected behavior in edge cases, such as handling invalid date formats or dealing with timezone issues. It might be helpful to add tests covering various date input scenarios to ensure the desired behavior.packages/plugins/tanstack-query/src/runtime/common.ts (2)
- 31-41: The introduction of the
QueryErrortype is a positive change that enhances error handling capabilities by including additional error information (info) and HTTP status code (status). This allows for more detailed error reporting and can improve the debugging experience. When implementing or catchingQueryError, ensure that the additional properties are used effectively to provide meaningful error messages to the developers or end-users. Additionally, consider documenting the expected structure of theinfoproperty to make it easier for developers to understand and use the error details provided.- 82-82: The modification to the
fetcherfunction to throw aQueryErrorwith additional error information and status code is a good practice for providing more context on errors. This change can significantly improve error handling in the application by allowing more granular control over error responses based on the status code or specific error details. Ensure that all calls to thefetcherfunction are updated to handle theQueryErrorappropriately, especially in parts of the application that might rely on the previous error handling behavior.packages/schema/src/cli/cli-util.ts (1)
- 283-283: Replacing
findPackageJsonwithfindUpfor locating thepackage.jsonfile is a logical change that potentially improves the flexibility and robustness of package discovery.findUpcan traverse up the directory tree, which is useful in monorepo setups or when the command is run from a subdirectory of the project. Ensure that thefindUpfunction is thoroughly tested in various project structures to confirm its reliability in finding the correctpackage.jsonfile. Additionally, consider handling cases wherepackage.jsonmight not be found to provide clear error messages or fallback behaviors.packages/testtools/src/schema.ts (1)
- 212-218: The modifications in the
loadSchemafunction to adjust script execution and dependency handling are noted. It's good to see the use of--no-version-checkand--no-dependency-checkflags to streamline the generation process. However, ensure that the dynamic construction of the command inrunis safe and does not introduce security vulnerabilities.Ensure that the construction and execution of commands within the
loadSchemafunction are secure and do not allow for command injection vulnerabilities. Consider validating or sanitizing inputs used to construct these commands.packages/schema/src/language-server/validator/datamodel-validator.ts (5)
- 8-8: The addition of the
isEnumimport is a good practice for enhancing the validation logic. It allows for more precise type checking, which can improve the robustness of the validation process.- 66-71: Refactoring the validation logic for
idFieldtype checking to includeisArray,isScalar, andisValidTypechecks is a significant improvement. It ensures that fields marked with@idare of appropriate types, enhancing the data model's integrity. This change makes the validation logic more comprehensive and maintainable.- 124-124: Improving validation for empty
"fields"values in relation attributes is a good practice. It ensures that the data model definitions are complete and logically consistent, preventing potential issues in schema generation or runtime behavior.- 134-134: Similar to the validation for empty
"fields"values, enhancing validation for empty"references"values is a positive change. It contributes to the overall robustness and correctness of the data model validation process.- 160-167: The enhanced validation for field references and nullable constraints is a valuable addition. It ensures that non-optional relations have appropriately defined foreign keys, contributing to the integrity and consistency of the data model. This change improves error reporting and helps developers identify and fix potential issues more efficiently.
packages/schema/src/plugins/prisma/schema-generator.ts (4)
- 50-51: The import statements for
execPackageandfindUpreplace previous utilities for executing commands and finding package.json files. This change likely enhances modularity and error handling.- 130-130: The command string for
prisma formathas been updated to useexecPackage. Ensure that the context in whichexecPackageis executed has the necessary permissions and environment variables set up correctly to avoid execution errors.- 139-145: The construction of the
prisma generatecommand string now accommodates additional arguments throughoptions.generateArgs. This flexibility is beneficial for customizing the command based on user needs. However, ensure proper sanitization and validation ofoptions.generateArgsto prevent injection vulnerabilities.- 453-453: The use of
findUpto locatepackage.jsonrepresents a more flexible approach to determining the project's configuration context. This change should make the tool more adaptable to different project structures. Ensure that the search forpackage.jsonis efficient and does not significantly impact the performance of the schema generation process.tests/integration/tests/enhancements/with-policy/field-validation.test.ts (4)
- 48-48: The addition of the
@lowerdirective to theslugfield in the schema is a good practice for ensuring consistent data formatting. This change should help maintain data integrity and improve query performance by avoiding case-sensitive comparisons.- 511-526: The test case for creating
userDatawith various string transformations (@trim,@lower,@upper) effectively verifies that the transformations are applied correctly. It's important to ensure comprehensive coverage for all transformation directives to catch any edge cases.- 528-536: The update operation test for
userDatathat includes string transformations (@trim,@lower,@upper) is well-implemented. This test ensures that updates also respect the specified transformations. Consider adding negative test cases to verify that incorrect data is rejected as expected.- 579-593: The upsert operation test for tasks with slug updates, including the
@lowertransformation, is a valuable addition. It verifies that the transformation is applied in both creation and update scenarios within an upsert operation. Ensure that the test covers scenarios where the upsert might result in an update to verify that the transformation is consistently applied.packages/schema/tests/schema/validation/datamodel-validation.test.ts (11)
- 1-1: The addition of
safelyLoadModelanderrorLikefunctions enhances the test suite by providing a more structured approach to handling errors and loading models. This change likely improves the readability and maintainability of the tests.- 12-21: The test case for duplicated fields effectively uses the
safelyLoadModelfunction to load a model and then asserts the expected error usingerrorLike. This approach is consistent and clear, making the test easy to understand and maintain.- 9-28: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [25-41]
The test case for scalar types correctly asserts that the model loading is fulfilled, indicating no errors. This test case is well-structured and provides good coverage for various scalar types and their optional or array variations.
- 45-53: The test for an unsupported type with a valid argument (a string literal) is concise and correctly expects the operation to succeed. This test ensures that the model validation logic properly handles custom or unsupported types when used correctly.
- 58-65: The test for an unsupported type with an invalid argument (a non-string literal) correctly expects an error. This test ensures that the validation logic enforces the requirement for string literals in certain contexts, which is crucial for maintaining the integrity of the model definitions.
- 70-78: The test case for using an unsupported type in an expression correctly expects an error, highlighting the limitations of certain types in expressions. This test is important for ensuring that the model validation logic correctly identifies and rejects unsupported usage patterns.
- 83-90: The test for mixing array and optional types correctly identifies a common modeling mistake and expects an appropriate error message. This test enhances the robustness of the model validation by catching subtle issues that could lead to confusion or errors in the schema.
- 95-122: The series of tests for unresolved field types are thorough and cover various scenarios (single type, array, optional). These tests ensure that the model validation logic correctly handles references to undefined or unsupported types, which is crucial for maintaining schema integrity.
- 125-329: The extensive testing around the
idfield, including scenarios with no unique fields, using@uniqueinstead of@id, and various incorrect usages of@idand@@id, provides comprehensive coverage of the validation logic for identifying fields. These tests are crucial for ensuring that models are defined with proper unique identifiers, which is fundamental for database integrity and operation.- 390-400: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [382-511]
The tests covering relation scenarios, including one-to-one, one-to-many, many-to-many, and various error cases, are well-structured and provide valuable validation for the relationship handling logic in the schema. These tests ensure that the model validation logic correctly handles complex relationship definitions and identifies common mistakes.
- 672-682: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [659-699]
The tests for abstract base types and inheritance scenarios, including errors for extending non-abstract models and missing opposite relation fields, provide important coverage for advanced modeling features. These tests ensure that the model validation logic correctly handles inheritance and abstract models, which are powerful features for structuring complex schemas.
packages/plugins/tanstack-query/src/generator.ts (6)
- 219-219: The
useModelMutationcall now includes aDefaultErrortype parameter, which is a significant change. This adjustment ensures that the mutation hooks are now explicitly typed withQueryErroras the default error type, enhancing type safety and error handling capabilities. However, it's crucial to verify that all usages of mutation hooks throughout the project have been updated to accommodate this change, especially if there are custom error types being used in some places.- 568-568: The
makeBaseImportsfunction now importsQueryErrorfrom the runtime base, which is a crucial update for ensuring that the newly introducedQueryErrortype is available throughout the generated files. This change aligns with the overall goal of enhancing error handling in the plugin. It's important to ensure that theQueryErrortype is correctly defined and exported in the specified runtime base.- 570-570: The introduction of the
DefaultErrortype alias, set toQueryError, is a strategic move to standardize error handling across the plugin. This change not only improves the readability and maintainability of the code by using a consistent error type but also enhances the plugin's flexibility by allowing easy updates to the default error type in the future if needed. It's a good practice to have such abstractions, especially in a plugin that interacts with various frameworks and versions.- 646-646: The
makeMutationOptionsfunction's return type for the React target has been updated to useDefaultErrorinstead of a generic error type. This change ensures consistency in error handling across different frameworks supported by the plugin. It's a positive step towards standardizing the plugin's interface and improving its usability. However, it's essential to verify that all React-specific mutation hooks in the project correctly handle theDefaultErrortype.Verification successful
The verification process has revealed that the
DefaultErrortype is indeed used within the context of mutation options in thetanstack-queryplugin, as indicated by references ingenerator.ts. Specifically, it is used as the error type for mutation options and hooks, aligning with the review comment's mention of standardizing error handling across different frameworks supported by the plugin.Furthermore, the presence of
useMutationhooks across various parts of the codebase, includingtanstack-queryandtrpcplugins, confirms the relevance of ensuring that these hooks correctly handle theDefaultErrortype. However, the direct correlation betweenuseMutationhooks and the explicit handling ofDefaultErrorwas not established through the provided outputs. This is likely due to the abstraction layers and the way error types are integrated into the hooks, which may not always explicitly mentionDefaultErrorin the context lines captured by the search.Given the evidence from the search results and the context provided, it can be concluded that the review comment is consistent with the codebase's current state. The
DefaultErrortype is indeed utilized within mutation options and hooks, supporting the comment's emphasis on standardizing error handling. However, due to the nature of the verification process and the limitations of the search scope, a direct, explicit verification of every React-specific mutation hook's handling ofDefaultErrorwas not feasible.Based on the findings:
- The
DefaultErrortype is used in mutation options and hooks, as intended.useMutationhooks are present and utilized across different parts of the codebase.- Direct, explicit verification of
DefaultErrorhandling in every React-specific mutation hook was beyond the scope of the provided search results.Therefore, the review comment is validated in the context of the importance of using
DefaultErrorfor standardizing error handling, with the caveat that direct verification of every specific instance was not achieved.* 648-648: The update to the `makeMutationOptions` function for the Vue target, specifying `DefaultError` as the error type, aligns with the plugin's goal of unified error handling. This change is crucial for maintaining consistency across different frameworks and ensuring that Vue-specific mutation hooks are equipped to handle errors using the standardized `QueryError` type. It's advisable to review the usage of Vue mutation hooks to confirm they are compatible with this update.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all React-specific mutation hooks handle the DefaultError type correctly. rg --type ts "useMutation" --context 5 | grep "DefaultError"Length of output: 58
Script:
#!/bin/bash # Search for any references to "DefaultError" within TypeScript files rg --type ts "DefaultError"Length of output: 813
Script:
#!/bin/bash # Confirm the presence of "useMutation" hooks in TypeScript files and capture their context rg --type ts "useMutation" -C 5Length of output: 34394
Verification successful
The verification process has confirmed that the update to the
makeMutationOptionsfunction, specifyingDefaultErroras the error type for Vue targets, is consistent with the plugin's goal of unified error handling. The Vue-specific mutation hooks are designed to work with this update, as evidenced by the consistent use ofDefaultErroracross the codebase. This ensures that Vue-specific mutation hooks are equipped to handle errors using the standardizedQueryErrortype, aligning with the intended error handling strategy.* 650-650: The adjustment in the `makeMutationOptions` function for the Svelte target to include `DefaultError` as the error type is a thoughtful update that enhances the plugin's consistency and error handling capabilities. This change ensures that Svelte-specific mutation hooks are now more robust in dealing with errors, leveraging the standardized `QueryError` type. It's important to ensure that the Svelte mutation hooks in the project are updated to reflect this change.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for Vue-specific mutation hooks to ensure compatibility with the DefaultError update. rg --type ts "useMutation" --context 5 | grep "DefaultError"Length of output: 58
Script:
#!/bin/bash # Broaden the search to understand how `useMutation` is used across the TypeScript files. rg --type ts "useMutation"Length of output: 4144
Script:
#!/bin/bash # Search for the definition and usage of `DefaultError` to understand its context. rg --type ts "DefaultError"Length of output: 813
Verification successful
The search results confirm the integration of
DefaultErrorinto the mutation options, as indicated in thegenerator.tsfile for the tanstack-query plugin. This supports the review comment's claim about the thoughtful update for the Svelte target to enhance error handling capabilities. The evidence from the codebase, particularly the usage ofDefaultErrorin mutation options and its definition, aligns with the expectations set in the original review comment. Therefore, the adjustment in themakeMutationOptionsfunction for the Svelte target, includingDefaultErroras the error type, is verified to be accurately reflected in the project's mutation hooks.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure Svelte mutation hooks are updated to reflect the DefaultError change. rg --type ts "useMutation" --context 5 | grep "DefaultError"Length of output: 58
Script:
#!/bin/bash # Broaden the search to understand the usage of "useMutation" in the codebase. rg --type ts "useMutation"Length of output: 4144
Script:
#!/bin/bash # Search for "DefaultError" usage across TypeScript files to see its definition and usage context. rg --type ts "DefaultError"Length of output: 813
tests/integration/tests/plugins/zod.test.ts (1)
- 506-548: The test case
does date coercionis well-structured and covers the essential scenarios for date coercion, including successful parsing of aDateTimeobject and a valid ISO string, as well as the failure of an invalid date string. This addition aligns with the PR objectives of enhancing testing frameworks and improving validation mechanisms. It's a valuable test for ensuring the Zod plugin correctly handles date types, which are common in web and database applications.One minor suggestion for improvement is to include a comment or documentation string at the beginning of the test case to briefly describe its purpose and what specific behavior it's validating. This can enhance readability and maintainability, especially for new contributors or when revisiting the tests in the future.
Overall, the test case is a solid addition to the test suite, and no major issues were found.
packages/schema/src/res/stdlib.zmodel (2)
- 393-393: The addition of an optional
mapparameter to the@@fulltextattribute is a significant enhancement. It allows for specifying a custom name for the full text index in the underlying database, which can be particularly useful for aligning database schema definitions with existing database conventions or requirements. This change is consistent with the pattern of other attributes in the file that offer amapparameter for similar customization purposes. The syntax appears correct, and the change is likely to improve the flexibility and usability of the@@fulltextattribute without introducing backward compatibility issues, assuming that the parameter is indeed optional.- 482-482: The introduction of an integer parameter
_ xto the@db.VarBinaryattribute extends the attribute's functionality by allowing users to specify the length of the variable binary data type in the database. This change aligns with the pattern observed in other data type attributes within the file, such as@db.String,@db.Char, and others, which also accept length parameters. The syntax of the change is correct, and it enhances the expressiveness and precision of the schema definition language by enabling more detailed specifications of data types. This improvement is likely to be well-received by users who need to define binary data fields with specific length requirements.packages/schema/tests/schema/validation/attribute-validation.test.ts (2)
- 229-245: The addition of models
AandBwith@@fulltextdirectives is a significant enhancement. It's important to ensure that the fields specified in the@@fulltextdirective (x,y,z) are of a type that supports full-text search, typicallyStringor similar text-based types. Additionally, for modelB, themap: "n"attribute specifies a custom name for the index, which should be validated to ensure it does not conflict with existing database constraints or indexes. Overall, this change appears to align well with the objective of enhancing schema validation capabilities.- 374-374: The introduction of the
_varBinarySizedfield with a@db.VarBinary(100)attribute in the_Bytesmodel is a precise way to define a variable-length binary data field with a specified maximum size. This change enhances the model's ability to handle binary data more effectively. It's crucial to ensure that the specified size (100) aligns with the application's requirements and database constraints. This addition appears to be correctly implemented and serves a clear purpose within the context of the model.tests/integration/tests/enhancements/with-policy/refactor.test.ts (13)
- 152-155: The test expects the email to be in lowercase, which aligns with the best practice of storing emails in a normalized format. This is a good implementation detail to ensure consistency across the application.
- 208-208: Trimming whitespace from the title before creating a post is a good practice for data normalization and consistency. This ensures that titles are stored in a clean format, which is beneficial for display purposes and when performing string comparisons.
- 401-401: Using
skipDuplicates: truein acreateManyoperation is a good practice to avoid errors related to unique constraint violations. This allows for a more resilient data insertion process, especially when dealing with bulk data that might contain duplicates.- 412-415: The test verifies that the title is correctly trimmed when retrieving the post. This is an important validation step to ensure that the data normalization logic (trimming titles) is applied consistently across the application.
- 427-431: The use of
createManywith title trimming demonstrates an understanding of data normalization practices. However, it's important to ensure that this trimming logic is consistently applied across all paths where data can enter the system, not just in test setups.Consider verifying if the title trimming logic is applied consistently in all parts of the application where posts can be created or updated.
- 524-525: Converting the email to lowercase upon update is consistent with the approach of normalizing email addresses. This ensures that the application treats email addresses in a case-insensitive manner, which is crucial for authentication and lookup operations.
- 583-586: Trimming the title and updating the
publishedstatus in a nested update operation demonstrates good data handling practices. It's important to ensure that such data normalization (trimming titles) is applied consistently across all update operations.- 611-627: Creating a post with a comment and ensuring the comment content is trimmed is a good practice. This test case effectively demonstrates handling nested creation operations with data normalization (trimming comment content).
- 660-660: The use of
createManywith title trimming in an update operation is consistent with data normalization practices. However, ensure that this logic is applied consistently across all parts of the application to maintain data integrity.Consider verifying if the title trimming logic is applied consistently in all parts of the application where posts can be created or updated.
- 748-748: Trimming the title in an update operation is a good practice for maintaining data consistency. This change aligns with the principle of normalizing data before storing it in the database.
- 825-825: The use of
upsertwith title trimming in the update part of the operation demonstrates an understanding of both data normalization and conditional data manipulation practices. This ensures that the application can handle scenarios where the data might or might not already exist in a flexible manner.- 841-841: Creating a new post with title trimming in an
upsertoperation is consistent with data normalization practices. This ensures that new data entering the system is stored in a clean and consistent format.- 1120-1120: Ensuring that the title is trimmed in an
updateManyoperation is a good practice for maintaining data consistency. This change aligns with the principle of normalizing data before storing it in the database.packages/runtime/src/enhancements/policy/policy-utils.ts (1)
- 319-325: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The file uses
deepcopyfor cloning objects. Ensure that performance implications are considered, especially in performance-critical paths, as deep cloning can be expensive for large or complex objects. If possible, evaluate if shallow copying or other mechanisms could be used to improve performance.
| @@ -319,7 +319,7 @@ export class PolicyUtil { | |||
| /** | |||
| * Checks if the given model has a policy guard for the given operation. | |||
| */ | |||
| hasAuthGuard(model: string, operation: PolicyOperationKind): boolean { | |||
| hasAuthGuard(model: string, operation: PolicyOperationKind) { | |||
There was a problem hiding this comment.
The method hasAuthGuard lacks a return type declaration. It's a best practice in TypeScript to explicitly declare return types for functions and methods to improve code readability and maintainability.
- hasAuthGuard(model: string, operation: PolicyOperationKind) {
+ hasAuthGuard(model: string, operation: PolicyOperationKind): boolean {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.
| hasAuthGuard(model: string, operation: PolicyOperationKind) { | |
| hasAuthGuard(model: string, operation: PolicyOperationKind): boolean { |
| hasOverrideAuthGuard(model: string, operation: PolicyOperationKind) { | ||
| const guard = this.requireGuard(model); | ||
| switch (operation) { | ||
| case 'read': | ||
| return Object.keys(guard).some((k) => k.startsWith(FIELD_LEVEL_OVERRIDE_READ_GUARD_PREFIX)); | ||
| case 'update': | ||
| return Object.keys(guard).some((k) => k.startsWith(FIELD_LEVEL_OVERRIDE_UPDATE_GUARD_PREFIX)); | ||
| default: | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The newly added method hasOverrideAuthGuard correctly checks for field-level override policy guards based on the operation type. This method enhances the flexibility and granularity of policy enforcement. However, it also lacks an explicit return type declaration.
- hasOverrideAuthGuard(model: string, operation: PolicyOperationKind) {
+ hasOverrideAuthGuard(model: string, operation: PolicyOperationKind): boolean {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.
| hasOverrideAuthGuard(model: string, operation: PolicyOperationKind) { | |
| const guard = this.requireGuard(model); | |
| switch (operation) { | |
| case 'read': | |
| return Object.keys(guard).some((k) => k.startsWith(FIELD_LEVEL_OVERRIDE_READ_GUARD_PREFIX)); | |
| case 'update': | |
| return Object.keys(guard).some((k) => k.startsWith(FIELD_LEVEL_OVERRIDE_UPDATE_GUARD_PREFIX)); | |
| default: | |
| return false; | |
| } | |
| } | |
| hasOverrideAuthGuard(model: string, operation: PolicyOperationKind): boolean { | |
| const guard = this.requireGuard(model); | |
| switch (operation) { | |
| case 'read': | |
| return Object.keys(guard).some((k) => k.startsWith(FIELD_LEVEL_OVERRIDE_READ_GUARD_PREFIX)); | |
| case 'update': | |
| return Object.keys(guard).some((k) => k.startsWith(FIELD_LEVEL_OVERRIDE_UPDATE_GUARD_PREFIX)); | |
| default: | |
| return false; | |
| } | |
| } |
| it('creates an item with date coercion', async () => { | ||
| const r = await handler({ | ||
| method: 'post', | ||
| path: '/post', | ||
| query: {}, | ||
| requestBody: { | ||
| data: { | ||
| type: 'post', | ||
| attributes: { | ||
| id: 1, | ||
| title: 'Post1', | ||
| published: true, | ||
| publishedAt: '2024-03-02T05:00:00.000Z', | ||
| }, | ||
| }, | ||
| }, | ||
| prisma, | ||
| }); | ||
|
|
||
| expect(r.status).toBe(201); | ||
| }); |
There was a problem hiding this comment.
The test case for creating an item with date coercion correctly checks for a successful creation (status code 201). However, it lacks an assertion to verify that the publishedAt field is correctly coerced and stored. To ensure the date coercion functionality works as expected, consider adding an assertion to check the stored value of publishedAt in the database or the response body.
// After the item creation, fetch the item and assert the publishedAt value
expect(r.body.data.attributes.publishedAt).toBe('2024-03-02T05:00:00.000Z');| it('creates an item with zod violation', async () => { | ||
| const r = await handler({ | ||
| method: 'post', | ||
| path: '/post', | ||
| query: {}, | ||
| requestBody: { | ||
| data: { | ||
| type: 'post', | ||
| attributes: { | ||
| id: 1, | ||
| title: 'a very very long long title', | ||
| }, | ||
| }, | ||
| }, | ||
| prisma, | ||
| }); | ||
|
|
||
| expect(r.status).toBe(400); | ||
| expect(r.body.errors[0].code).toBe('invalid-payload'); | ||
| }); |
There was a problem hiding this comment.
This test case correctly checks for a Zod validation error when creating an item with a title that exceeds the defined length constraints. However, the assertion only checks for the status code and the error code. It would be beneficial to also assert the specific Zod error message to ensure that the validation error is indeed due to the title length violation and not some other issue.
// Add an assertion for the specific Zod error message
expect(r.body.errors[0].detail).toContain('title');| it('update an item with date coercion', async () => { | ||
| await prisma.post.create({ data: { id: 1, title: 'Post1' } }); | ||
|
|
||
| const r = await handler({ | ||
| method: 'put', | ||
| path: '/post/1', | ||
| query: {}, | ||
| requestBody: { | ||
| data: { | ||
| type: 'post', | ||
| attributes: { | ||
| published: true, | ||
| publishedAt: '2024-03-02T05:00:00.000Z', | ||
| }, | ||
| }, | ||
| }, | ||
| prisma, | ||
| }); | ||
|
|
||
| expect(r.status).toBe(200); | ||
| }); |
There was a problem hiding this comment.
Similar to the previous comment on date coercion during item creation, this test case for updating an item with date coercion should include an assertion to verify that the publishedAt field is correctly updated and coerced. This ensures the update functionality handles date coercion as expected.
// After the item update, fetch the item and assert the publishedAt value
expect(r.body.data.attributes.publishedAt).toBe('2024-03-02T05:00:00.000Z');| const workspaceRoot = getWorkspaceRoot(__dirname); | ||
|
|
||
| if (!root) { | ||
| if (!workspaceRoot) { | ||
| throw new Error('Could not find workspace root'); | ||
| } | ||
|
|
||
| const pkgContent = fs.readFileSync(path.join(__dirname, 'package.template.json'), { encoding: 'utf-8' }); | ||
| fs.writeFileSync(path.join(projectRoot, 'package.json'), pkgContent.replaceAll('<root>', root)); | ||
|
|
||
| const npmrcContent = fs.readFileSync(path.join(__dirname, '.npmrc.template'), { encoding: 'utf-8' }); | ||
| fs.writeFileSync(path.join(projectRoot, '.npmrc'), npmrcContent.replaceAll('<root>', root)); | ||
|
|
||
| console.log('Workdir:', projectRoot); | ||
| process.chdir(projectRoot); | ||
|
|
||
| // copy project structure from scaffold (prepared by test-setup.ts) | ||
| fs.cpSync(path.join(workspaceRoot, '.test/scaffold'), projectRoot, { recursive: true, force: true }); | ||
|
|
||
| // install local deps | ||
| const localInstallDeps = [ | ||
| 'packages/schema/dist', | ||
| 'packages/runtime/dist', | ||
| 'packages/plugins/swr/dist', | ||
| 'packages/plugins/trpc/dist', | ||
| 'packages/plugins/openapi/dist', | ||
| ]; | ||
|
|
||
| run(`npm i --no-audit --no-fund ${localInstallDeps.map((d) => path.join(workspaceRoot, d)).join(' ')}`); |
There was a problem hiding this comment.
Refactoring the logic for copying the project structure and installing local dependencies improves the maintainability of the code. However, hardcoding the paths to local dependencies (localInstallDeps) might limit the flexibility of this function. Consider making this list configurable or deriving it dynamically to accommodate future changes in the project structure without requiring code modifications.
Consider making the list of local dependencies (localInstallDeps) configurable or deriving it dynamically to improve flexibility and maintainability.
| if (opt.extraDependencies) { | ||
| console.log(`Installing dependency ${opt.extraDependencies.join(' ')}`); | ||
| installPackage(opt.extraDependencies.join(' ')); | ||
| } |
There was a problem hiding this comment.
Adding the capability to install extra dependencies dynamically is a useful feature. However, similar to previous comments, ensure that the input (opt.extraDependencies.join(' ')) is sanitized to prevent potential command injection vulnerabilities.
Consider adding validation or sanitization for the opt.extraDependencies parameter to ensure it does not introduce security vulnerabilities when used to construct the command for package installation.
| @@ -144,12 +144,15 @@ describe('With Policy: refactor tests', () => { | |||
| // read back check | |||
| await expect( | |||
| anonDb.user.create({ | |||
| data: { id: 1, email: 'user1@zenstack.dev' }, | |||
| data: { id: 1, email: 'User1@zenstack.dev' }, | |||
There was a problem hiding this comment.
The email address is hardcoded to a specific case ('[email protected]') before being created. It's good practice to normalize email addresses to a consistent case, typically lowercase, to avoid case-sensitive issues during authentication or lookup operations.
- data: { id: 1, email: '[email protected]' },
+ data: { id: 1, email: '[email protected]'.toLowerCase() },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.
| data: { id: 1, email: 'User1@zenstack.dev' }, | |
| data: { id: 1, email: 'user1@zenstack.dev'.toLowerCase() }, |
| /** | ||
| * Utility for running package commands through npx/bunx | ||
| */ | ||
| export function execPackage(cmd: string, stdio: StdioOptions = 'inherit', env?: Record<string, string>): void { | ||
| const packageManager = process?.versions?.bun ? 'bunx' : 'npx'; | ||
| const mergedEnv = { ...process.env, ...env }; | ||
| _exec(`${packageManager} ${cmd}`, { encoding: 'utf-8', stdio, env: mergedEnv }); | ||
| } |
There was a problem hiding this comment.
The addition of the execPackage function is a useful abstraction for running package commands with flexibility between package managers. However, there are potential security concerns related to command injection vulnerabilities. Ensure that any user input passed to these functions is strictly controlled or sanitized to mitigate these risks.
+ Ensure input validation or sanitization for `cmd` arguments.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.
| /** | |
| * Utility for running package commands through npx/bunx | |
| */ | |
| export function execPackage(cmd: string, stdio: StdioOptions = 'inherit', env?: Record<string, string>): void { | |
| const packageManager = process?.versions?.bun ? 'bunx' : 'npx'; | |
| const mergedEnv = { ...process.env, ...env }; | |
| _exec(`${packageManager} ${cmd}`, { encoding: 'utf-8', stdio, env: mergedEnv }); | |
| } | |
| /** | |
| * Utility for running package commands through npx/bunx | |
| */ | |
| export function execPackage(cmd: string, stdio: StdioOptions = 'inherit', env?: Record<string, string>): void { | |
| const packageManager = process?.versions?.bun ? 'bunx' : 'npx'; | |
| const mergedEnv = { ...process.env, ...env }; | |
| _exec(`${packageManager} ${cmd}`, { encoding: 'utf-8', stdio, env: mergedEnv }); | |
| } |
| function run(cmd: string) { | ||
| console.log(`Running: ${cmd}, in ${scaffoldPath}`); | ||
| try { | ||
| execSync(cmd, { cwd: scaffoldPath, stdio: 'ignore' }); | ||
| } catch (err) { | ||
| console.error(`Test project scaffolding cmd error: ${err}`); | ||
| throw err; | ||
| } | ||
| } | ||
|
|
||
| run('npm init -y'); | ||
| run('npm i --no-audit --no-fund typescript prisma @prisma/client zod decimal.js'); | ||
|
|
||
| console.log('Test scaffold setup complete.'); |
There was a problem hiding this comment.
The script for setting up a test scaffold automates the creation of a test environment, which is beneficial. However, there's a potential security concern due to the use of potentially user-controlled input in execSync. Ensure that any dynamic parts of the command are properly sanitized or that user input is validated to mitigate the risk of command injection vulnerabilities.
+ Ensure input validation or sanitization for `cmd` arguments.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.
| function run(cmd: string) { | |
| console.log(`Running: ${cmd}, in ${scaffoldPath}`); | |
| try { | |
| execSync(cmd, { cwd: scaffoldPath, stdio: 'ignore' }); | |
| } catch (err) { | |
| console.error(`Test project scaffolding cmd error: ${err}`); | |
| throw err; | |
| } | |
| } | |
| run('npm init -y'); | |
| run('npm i --no-audit --no-fund typescript prisma @prisma/client zod decimal.js'); | |
| console.log('Test scaffold setup complete.'); | |
| function run(cmd: string) { | |
| console.log(`Running: ${cmd}, in ${scaffoldPath}`); | |
| try { | |
| execSync(cmd, { cwd: scaffoldPath, stdio: 'ignore' }); | |
| } catch (err) { | |
| console.error(`Test project scaffolding cmd error: ${err}`); | |
| throw err; | |
| } | |
| } | |
| run('npm init -y'); | |
| run('npm i --no-audit --no-fund typescript prisma @prisma/client zod decimal.js'); | |
| console.log('Test scaffold setup complete.'); |
Summary by CodeRabbit
@@indexattribute in JetBrains IDE plugin.QueryErrortype for improved error handling in tanstack-query plugins.publishedAtfield of type DateTime to thePostmodel for better content management..gitignoreto exclude.testdirectories.@lowerand@trim.