fix(runtime): avoid duplicating non-plain objects by ymc9 · Pull Request #1545 · zenstackhq/zenstack

ymc9 · 2024-06-30T03:05:06Z

coderabbitai · 2024-06-30T03:05:21Z

Walkthrough

The code changes introduce a new clone function within the cross module, replacing all instances of the deepcopy function in various files for deep cloning arrays and objects. Additionally, a safeClone method has been introduced in policy-utils.ts. A regression test addressing issue #1533 was also added to validate searches for null values and attributes in JSON fields.

Changes

Files	Summary
`packages/runtime/src/cross/clone.ts`	Introduced a new `clone` function for deep cloning arrays and objects.
`packages/runtime/src/cross/index.ts`	Added export for the `clone` function.
`packages/runtime/src/cross/mutator.ts`, `packages/runtime/src/enhancements/default-auth.ts`, `packages/runtime/src/enhancements/delegate.ts`, `packages/runtime/src/enhancements/proxy.ts`	Replaced `deepcopy` with `clone` in multiple modules for consistent cloning behavior.
`packages/runtime/src/enhancements/policy/handler.ts`, `packages/runtime/src/enhancements/policy/policy-utils.ts`	Removed old `clone` method and introduced `safeClone` for cloning operations in policy enhancements.
`packages/runtime/src/enhancements/utils.ts`	Removed the old `clone` function using `deepcopy`.
`tests/regression/tests/issue-1533.test.ts`	Added regression test to validate JSON field checks for null values and attribute existence, addressing issue #1533.

Assessment against linked issues

Objective	Addressed	Explanation
Ensure deep cloning is consistent across the project by replacing `deepcopy` with `clone`	✅
Address issue #1533 by validating search operations for null JSON attributes	✅

Tip

Early access features: enabled

We are currently testing the following features in early access:

OpenAI gpt-4o model for code reviews and chat: OpenAI claims that this model is better at understanding and generating code than the previous models. We seek your feedback over the next few weeks before making it generally available.

Note:

You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.
Please join our Discord Community to provide feedback and report issues.
OSS projects are always opted into early access features.

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 18a4877 and 01f6ea7.

Files ignored due to path filters (2)

packages/runtime/package.json is excluded by !**/*.json
pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml, !**/*.yaml

Files selected for processing (11)

packages/runtime/src/cross/clone.ts (1 hunks)
packages/runtime/src/cross/index.ts (1 hunks)
packages/runtime/src/cross/mutator.ts (3 hunks)
packages/runtime/src/enhancements/default-auth.ts (2 hunks)
packages/runtime/src/enhancements/delegate.ts (17 hunks)
packages/runtime/src/enhancements/policy/handler.ts (15 hunks)
packages/runtime/src/enhancements/policy/policy-utils.ts (7 hunks)
packages/runtime/src/enhancements/proxy.ts (2 hunks)
packages/runtime/src/enhancements/query-utils.ts (3 hunks)
packages/runtime/src/enhancements/utils.ts (2 hunks)
tests/regression/tests/issue-1533.test.ts (1 hunks)

Files skipped from review due to trivial changes (1)

packages/runtime/src/enhancements/utils.ts

Additional context used

Biome

packages/runtime/src/cross/mutator.ts

[error] 148-148: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

packages/runtime/src/enhancements/query-utils.ts

[error] 54-57: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 157-157: Avoid the use of spread (...) syntax on accumulators.

Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.

(lint/performance/noAccumulatingSpread)

packages/runtime/src/enhancements/proxy.ts

[error] 219-221: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 232-234: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 273-273: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

packages/runtime/src/enhancements/delegate.ts

[error] 92-94: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 132-135: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 170-173: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 286-292: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 307-307: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[error] 330-330: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[error] 455-455: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[error] 672-703: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 736-736: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[error] 799-799: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[error] 807-807: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[error] 979-979: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

packages/runtime/src/enhancements/policy/policy-utils.ts

[error] 65-69: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 67-69: This else clause can be omitted because previous branches break early.

(lint/style/noUselessElse)

[error] 79-83: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 81-83: This else clause can be omitted because previous branches break early.

(lint/style/noUselessElse)

[error] 92-96: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 94-96: This else clause can be omitted because previous branches break early.

(lint/style/noUselessElse)

[error] 105-110: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 116-118: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 378-380: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 416-426: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 523-523: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[error] 867-869: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 1108-1108: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[error] 1248-1250: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

packages/runtime/src/enhancements/policy/handler.ts

[error] 216-225: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 230-232: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 389-391: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 426-428: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 455-463: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 517-519: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 540-545: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 704-706: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 915-919: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 989-989: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[error] 1009-1009: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[error] 1043-1054: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 1066-1081: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 1130-1130: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[error] 1222-1224: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 1373-1378: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 1383-1385: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 1434-1436: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 1481-1487: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)

[error] 1205-1205: Avoid the use of spread (...) syntax on accumulators.

Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.

(lint/performance/noAccumulatingSpread)

Additional comments not posted (44)

packages/runtime/src/cross/index.ts (1)

1-1: LGTM!

The new export for clone aligns with the rest of the export statements.

packages/runtime/src/cross/clone.ts (1)

1-25: LGTM!

The clone function correctly handles deep cloning of arrays and plain objects, returning other values as-is.

tests/regression/tests/issue-1533.test.ts (1)

1-66: LGTM!

The regression test for issue 1533 is well-implemented and covers the described scenarios. The test setup and teardown are handled properly.

packages/runtime/src/enhancements/default-auth.ts (2)

5-5: LGTM!

The import statement has been updated correctly to use clone from cross.

54-54: LGTM!

The preprocessWritePayload method correctly replaces deepcopy with clone.

packages/runtime/src/cross/mutator.ts (2)

12-12: Importing clone function

The clone function has been imported from ./clone. Ensure that this function is properly tested and handles all edge cases.

203-203: Using clone function

The clone function is used to ensure new object identity. This is a good practice to avoid unintended mutations.

packages/runtime/src/enhancements/query-utils.ts (3)

10-10: Importing clone function

The clone function has been imported from ../cross. Ensure that this function is properly tested and handles all edge cases.

152-152: Using safeClone in composeCompoundUniqueField

The safeClone method is used to clone fieldData. This ensures that the original data is not mutated during the composition of compound unique fields.

210-216: Adding safeClone method

The safeClone method clones an object and ensures it's not empty. This is a good utility function to prevent errors due to empty objects.

packages/runtime/src/enhancements/proxy.ts (2)

5-5: Importing clone function

The clone function has been imported from ../cross. Ensure that this function is properly tested and handles all edge cases.

77-77: Using clone function

The clone function is used to clone args if they are provided. This ensures that the original arguments are not mutated during the processing of the fluent call.

packages/runtime/src/enhancements/delegate.ts (16)

16-16: Importing clone function

The clone function has been imported from ../cross. Ensure that this function is properly tested and handles all edge cases.

75-75: Using clone function in doFind method

The clone function is used to clone args if they are provided. This ensures that the original arguments are not mutated during the processing of the find operation.

145-145: Using clone function in buildWhereHierarchy method

The clone function is used to clone where conditions. This ensures that the original conditions are not mutated during the hierarchy building process.

220-220: Using clone function in buildSelectIncludeHierarchy method

The clone function is used to clone args. This ensures that the original arguments are not mutated during the hierarchy building process.

411-411: Using clone function in doCreate method

The clone function is used to clone args. This ensures that the original arguments are not mutated during the create operation.

627-627: Using clone function in upsert method

The clone function is used to clone args. This ensures that the original arguments are not mutated during the upsert operation.

645-645: Using clone function in doUpdate method

The clone function is used to clone args. This ensures that the original arguments are not mutated during the update operation.

665-665: Using clone function in doUpdateMany method

The clone function is used to clone args. This ensures that the original arguments are not mutated during the updateMany operation.

675-675: Using clone function in doUpdateMany method (findArgs)

The clone function is used to clone args.where. This ensures that the original conditions are not mutated during the find operation.

686-686: Using clone function in doUpdateMany method (updatePayload)

The clone function is used to clone args.data. This ensures that the original data is not mutated during the update operation.

852-852: Using clone function in delete method

The clone function is used to clone args. This ensures that the original arguments are not mutated during the delete operation.

868-868: Using clone function in doDeleteMany method

The clone function is used to clone where conditions. This ensures that the original conditions are not mutated during the deleteMany operation.

921-921: Using clone function in aggregate method

The clone function is used to clone args. This ensures that the original arguments are not mutated during the aggregate operation.

949-949: Using clone function in count method

The clone function is used to clone args. This ensures that the original arguments are not mutated during the count operation.

989-989: Using clone function in groupBy method

The clone function is used to clone args. This ensures that the original arguments are not mutated during the groupBy operation.

1030-1030: Using clone function in extractSelectInclude method

The clone function is used to clone args. This ensures that the original arguments are not mutated during the extraction of select/include options.

packages/runtime/src/enhancements/policy/policy-utils.ts (5)

10-10: LGTM! The import statement for clone is correct.

553-553: LGTM! The use of clone to create a deep copy of the payload object is appropriate.

809-809: LGTM! The use of safeClone for cloning uniqueFilter is appropriate.

1004-1004: LGTM! The use of safeClone for cloning uniqueFilter is appropriate.

1036-1036: LGTM! The use of safeClone for cloning selectIncludeClean is appropriate.

packages/runtime/src/enhancements/policy/handler.ts (11)

28-28: LGTM!

The import statement change is consistent with the summary.

130-130: LGTM!

The findWithFluent method now uses safeClone to ensure safe cloning of arguments.

141-141: LGTM!

The doFind method now uses safeClone to ensure safe cloning of arguments.

179-179: LGTM!

The create method now uses safeClone to ensure safe cloning of arguments.

446-446: LGTM!

The createMany method now uses safeClone to ensure safe cloning of arguments.

483-483: LGTM!

The createManyAndReturn method now uses safeClone to ensure safe cloning of arguments.

689-689: LGTM!

The update method now uses safeClone to ensure safe cloning of arguments.

1152-1152: LGTM!

The calculatePostUpdateIds method now uses safeClone to ensure safe cloning of current IDs.

1242-1242: LGTM!

The updateMany method now uses safeClone to ensure safe cloning of arguments.

1352-1352: LGTM!

The upsert method now uses safeClone to ensure safe cloning of arguments.

1445-1445: LGTM!

The deleteMany method now uses safeClone to ensure safe cloning of arguments.

ymc9 added 2 commits June 29, 2024 17:34

fix(runtime): avoid duplicating non-plain objects

94f79e5

fix

01f6ea7

coderabbitai bot reviewed Jun 30, 2024

View reviewed changes

ymc9 merged commit ba2a113 into dev Jun 30, 2024

ymc9 deleted the fix/issue-1533 branch June 30, 2024 03:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(runtime): avoid duplicating non-plain objects#1545

fix(runtime): avoid duplicating non-plain objects#1545
ymc9 merged 2 commits intodevfrom
fix/issue-1533

ymc9 commented Jun 30, 2024

Uh oh!

coderabbitai bot commented Jun 30, 2024 •

edited

Loading

Walkthrough

Changes

Assessment against linked issues

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

Uh oh!

coderabbitai bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ymc9 commented Jun 30, 2024

Uh oh!

coderabbitai bot commented Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Assessment against linked issues

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai bot commented Jun 30, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)