Read-strict #8
Read-strict #8
Conversation
… checks, and enum validation when readStrict is false; update merge_objects utility for type safety; add tests for readStrict behavior in Model class.
…ma function for improved performance; adjust type checking and validation logic based on readStrict settings.
|
I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law. You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
""" WalkthroughThis update introduces a non-strict read mode for Dynamoose models, allowing attribute type validation and default application to be bypassed when reading from DynamoDB if Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Model
participant Item
participant DynamoDB
User->>Model: get(key, { readStrict: false })
Model->>DynamoDB: fetch item by key
DynamoDB-->>Model: item data
Model->>Item: objectFromSchema(item, { readStrict: false, type: "fromDynamo" })
Item-->>Model: skips validation, returns raw values
Model-->>User: returns item with raw attributes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/dynamoose/lib/utils/merge_objects.ts (1)
46-53: Fix truthiness check and guard numeric merging branch.Using
if (returnObject[key])skips zero values (falsy) and may mis-route numeric merges. Replace the truthiness guard with an explicit existence and type check. For example:-} else if (returnObject[key]) { - if (settings.combineMethod === MergeObjectsCombineMethod.ArrayMergeNewArray) { - returnObject[key] = [returnObject[key], arg[key]]; - } else if (typeof returnObject[key] === "number") { - (returnObject[key] as number) += (arg[key] as number); - } else { - returnObject[key] = arg[key]; - } -} else { - returnObject[key] = arg[key]; -} +} else if (Object.prototype.hasOwnProperty.call(returnObject, key)) { + if (settings.combineMethod === MergeObjectsCombineMethod.ArrayMergeNewArray) { + returnObject[key] = [returnObject[key], arg[key]]; + } else if (typeof returnObject[key] === "number" && typeof arg[key] === "number") { + returnObject[key] = (returnObject[key] as number) + (arg[key] as number); + } else { + returnObject[key] = arg[key]; + } +} else { + returnObject[key] = arg[key]; +}This ensures zeros are correctly merged and only numbers enter the numeric-add branch.
🧹 Nitpick comments (1)
packages/dynamoose/lib/utils/merge_objects.ts (1)
3-3: Useimport typefor type-only imports.Since
GeneralObjectis only used as a type, switch to a type import to avoid bringing in runtime code:-import {GeneralObject} from "js-object-utilities"; +import type {GeneralObject} from "js-object-utilities";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/dynamoose/lib/Item.ts(7 hunks)packages/dynamoose/lib/utils/merge_objects.ts(1 hunks)packages/dynamoose/test/Model.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Ensure code is clean, readable, and maintainable. - Adhere to proper naming conventions and consistent formatting. - Split functionality into smaller, reusable modules...
**/*.ts: - Ensure code is clean, readable, and maintainable.
- Adhere to proper naming conventions and consistent formatting.
- Split functionality into smaller, reusable modules.
- Keep files small by extracting related functionalities into separate modules.
- Use imports to connect modules effectively.
- Write concise, technical, and type-safe TypeScript code.
- Import types with
import typeto avoid importing implementations.- Use
@ts-expect-errorinstead of@ts-ignoreand add a comment explaining why the error is expected.- Use early return patterns to avoid deep nesting.
- Use descriptive variable names with auxiliary verbs (e.g.,
isLoading,hasError).- Use AWS SDK V3 instead of V2.
- Use Zod for data validation.
- Use the full import path for packages when possible to enable tree shaking and reduce bundle size.
packages/dynamoose/lib/utils/merge_objects.tspackages/dynamoose/lib/Item.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cursor BugBot
- GitHub Check: test (22.x)
🔇 Additional comments (15)
packages/dynamoose/test/Model.js (6)
800-817: LGTM! Well-structured test for readStrict functionality.This test case properly verifies that when
readStrictis false, raw Number values are preserved for Date attributes configured with ISO storage, instead of throwing type mismatch errors. The test setup, mock data, and assertions are all appropriate.
819-829: LGTM! Good complementary test for Date handling.This test case appropriately verifies that ISO Date strings are preserved when
readStrictis false, complementing the previous test for Number values. The test structure and assertions are correct.
831-840: LGTM! Comprehensive test for type mismatch handling.This test case effectively verifies that completely mismatched data types (string "Hello World" for Date field) are preserved when
readStrictis false. This ensures robust handling of unexpected data types from DynamoDB.
880-888: LGTM! Good coverage for Object type mismatches.This test case appropriately extends the readStrict functionality testing to Object types, verifying that raw String values are preserved instead of expected Object structures when type mismatches occur.
890-898: LGTM! Important test for nested Object handling.This test case provides crucial coverage for nested Object properties, ensuring that the readStrict functionality works correctly at all levels of data structure hierarchy. The test properly verifies that raw boolean values are preserved for nested properties.
1286-1295: LGTM! Completes basic type coverage for readStrict.This test case appropriately rounds out the readStrict functionality testing by covering Number type mismatches. The test structure is consistent with the other new tests and provides important coverage for this common data type.
packages/dynamoose/lib/Item.ts (9)
656-656: LGTM!Correctly skips expensive type path calculations when
readStrictis false for fromDynamo operations, improving performance for non-strict reads.
660-739: Well-implemented lightweight type checking for non-strict reads.The conditional logic correctly preserves essential parent tracking for arrays, sets, models, and dynamooseAny types while skipping expensive type validation when
readStrictis false. The comprehensive checks forneedsInfiniteTrackinghandle all necessary cases including self-referencing models (THIS type) and Set structures.
740-748: Good performance optimization with caching.The caching mechanism for
attributesWithSchemaprevents redundant expensive computations since this value is used multiple times throughout the function. The lazy initialization pattern ensures the cache is only populated when needed.
750-751: Correctly skips default value processing for non-strict reads.The logic appropriately bypasses default value application when
readStrictis false, as the comment correctly notes that defaults are already applied during item creation. This optimization aligns with the non-strict read mode's goal of improving performance.
773-773: Consistent use of cached attributes.Correctly uses the cached
getAttributesWithSchema()for better performance.
833-833: Consistent use of cached attributes in modifiers.Properly leverages the cached
getAttributesWithSchema()to avoid redundant computations.
849-851: Appropriately skips validation for non-strict reads.The conditional logic correctly bypasses validation when
readStrictis false, based on the valid assumption that data from DynamoDB is already validated. Also properly uses the cached attributes for consistency.
882-884: Correctly bypasses required field validation for non-strict reads.The logic appropriately skips required field checks when
readStrictis false, as data from DynamoDB should have already satisfied these constraints during item creation.
907-909: Completes the non-strict read optimization pattern.The enum validation is appropriately skipped when
readStrictis false, maintaining consistency with the overall approach of bypassing expensive validations for data read from DynamoDB. The implementation correctly assumes that stored data has already passed enum constraints.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Pass typeIndexOptionMap to schema methods in readStrict=false branch to ensure consistent behavior - Extract repeated condition check into descriptive variable skipValidationForNonStrictRead - Improves code readability and prevents potential undefined access issues 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Fix truthiness check in merge_objects.ts that skipped zero values - Use import type for type-only imports to improve tree shaking - All schema access functions already handle undefined typeIndexOptionMap gracefully - Custom types processing maintained for essential functionality (TTL conversion, etc.) - All tests passing 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove typeIndexOptionMap parameter from schema access methods in readStrict branch - These methods use different parameter signatures than the validation methods - Build and tests now pass successfully 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Restore original import statement and truthiness check - These changes were suggested by CodeRabbit but are not necessary for the core PR functionality - All tests still pass 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Replace unbounded Map caches with LRU caches to prevent memory leaks - Add deep copy protection for cached objects to prevent mutation - Improve cache key generation to avoid collisions - Add Model.clearCache() method for cache invalidation - Set reasonable cache limits: 200 for convertKeyToObject, 500 for attributeType Security improvements: - Prevent cache corruption from object mutations - Escape delimiters in cache keys to avoid collisions - Use stable JSON.stringify with sorted keys 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Analysis showed that cached objects are only read (Object.keys()) and never mutated, making deep copy unnecessary overhead. Performance improvement: Eliminates deep copy on every cache hit for convertKeyToObject operations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/dynamoose/lib/utils/LRUCache.ts (1)
1-55: Fix formatting issues and the implementation looks solid.The LRU cache implementation is correct and follows best practices. The use of Map's insertion order to track access patterns and the delete/re-insert approach for updating usage are appropriate for an LRU cache.
Apply this diff to fix the formatting issues:
/** * Simple LRU (Least Recently Used) Cache implementation * Prevents unbounded memory growth while maintaining performance benefits - * + * * Note: This cache stores references to objects. Callers should ensure * they do not mutate cached objects to prevent cache corruption. */ export class LRUCache<K, V> { // ... rest of implementation has(key: K): boolean { return this.cache.has(key); } -} +} +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/dynamoose/lib/Model/index.ts(6 hunks)packages/dynamoose/lib/utils/LRUCache.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Ensure code is clean, readable, and maintainable. - Adhere to proper naming conventions and consistent formatting. - Split functionality into smaller, reusable modules...
**/*.ts: - Ensure code is clean, readable, and maintainable.
- Adhere to proper naming conventions and consistent formatting.
- Split functionality into smaller, reusable modules.
- Keep files small by extracting related functionalities into separate modules.
- Use imports to connect modules effectively.
- Write concise, technical, and type-safe TypeScript code.
- Import types with
import typeto avoid importing implementations.- Use
@ts-expect-errorinstead of@ts-ignoreand add a comment explaining why the error is expected.- Use early return patterns to avoid deep nesting.
- Use descriptive variable names with auxiliary verbs (e.g.,
isLoading,hasError).- Use AWS SDK V3 instead of V2.
- Use Zod for data validation.
- Use the full import path for packages when possible to enable tree shaking and reduce bundle size.
packages/dynamoose/lib/utils/LRUCache.tspackages/dynamoose/lib/Model/index.ts
🧬 Code Graph Analysis (1)
packages/dynamoose/lib/Model/index.ts (1)
packages/dynamoose/lib/utils/LRUCache.ts (1)
LRUCache(8-55)
🪛 GitHub Check: lint
packages/dynamoose/lib/utils/LRUCache.ts
[failure] 55-55:
Newline required at end of file but not found
[failure] 4-4:
Trailing spaces not allowed
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Cursor BugBot
- GitHub Check: test (23.x)
- GitHub Check: test (22.x)
- GitHub Check: test (20.x)
- GitHub Check: test (20.x)
- GitHub Check: test (22.x)
- GitHub Check: test (23.x)
🔇 Additional comments (9)
packages/dynamoose/lib/Model/index.ts (9)
21-21: LGTM!The import follows TypeScript best practices by importing the specific class needed.
141-142: LGTM!Converting from unbounded Maps to LRU caches prevents memory leaks while maintaining performance benefits. The type annotations are correct.
755-756: Excellent cache key generation improvement.Sorting object keys before JSON.stringify ensures consistent cache keys regardless of property insertion order, preventing cache misses for semantically identical objects.
761-762: Appropriate LRU cache initialization.The 200-entry limit for
convertKeyToObjectCacheis reasonable for key conversion operations and prevents unbounded memory growth.
790-791: Robust cache key generation with collision prevention.Escaping colons in attribute names prevents cache key collisions when attribute names contain the delimiter character. This is a solid defensive programming practice.
799-800: Appropriate LRU cache initialization.The 500-entry limit for
attributeTypeCacheis reasonable for schema attribute type caching and prevents unbounded memory growth.
964-965: Consistent cache key generation pattern.This maintains the same robust cache key generation logic used elsewhere, ensuring consistent collision prevention across all attribute type caching.
973-974: Consistent LRU cache initialization.Maintains the same 500-entry limit for attribute type caching consistency across the codebase.
1188-1200: Well-designed cache invalidation method.The
clearCache()method properly clears all performance caches including the new LRU caches. The JSDoc clearly explains when this should be used, and the implementation correctly handles both Set/Map caches and LRU caches.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Tests all LRUCache methods: get, set, has, delete, clear, size - Validates LRU eviction policy and ordering behavior - Covers edge cases: maxSize 0/1, large caches, null/undefined values - Tests generic type support with different key/value types - Verifies object reference storage behavior - Includes performance tests with thousands of operations - Achieves 100% code coverage for LRUCache implementation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/dynamoose/test/utils/LRUCache.js (1)
4-19: Consider validating maxSize behavior indirectly.The constructor tests verify initial size but don't validate that the maxSize is actually respected. Consider adding assertions that fill the cache to capacity to ensure the maxSize parameter is working correctly.
For example, add a test that fills the cache to verify capacity:
+ it("Should respect custom maxSize limit", () => { + const cache = new LRUCache(2); + cache.set("key1", "value1"); + cache.set("key2", "value2"); + cache.set("key3", "value3"); // Should evict key1 + + expect(cache.size).toEqual(2); + expect(cache.has("key1")).toEqual(false); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/dynamoose/test/utils/LRUCache.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/dynamoose/test/utils/LRUCache.js (1)
packages/dynamoose/lib/utils/LRUCache.ts (1)
LRUCache(8-55)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Cursor BugBot
- GitHub Check: test (20.x)
- GitHub Check: test (22.x)
- GitHub Check: test (23.x)
- GitHub Check: test-types
- GitHub Check: test (23.x)
- GitHub Check: test (20.x)
- GitHub Check: test (22.x)
🔇 Additional comments (6)
packages/dynamoose/test/utils/LRUCache.js (6)
1-3: LGTM! Proper test setup and imports.The import structure correctly references the compiled distribution and follows Node.js testing conventions.
196-277: Excellent LRU policy test coverage.The tests thoroughly validate the LRU eviction behavior, including edge cases where items are accessed or updated to change their position in the LRU order.
293-301: Review the maxSize=0 behavior and implementation.The test acknowledges a "known limitation" where the cache still stores items when maxSize=0. This might indicate an implementation issue that should be addressed.
Consider whether this behavior is intentional or if the implementation should be updated to handle maxSize=0 correctly by immediately evicting items or preventing insertion.
343-373: Proper validation of reference behavior.The tests correctly verify that the cache stores object and array references rather than deep copies, which is the expected behavior for performance reasons.
375-412: Comprehensive generic type validation.The tests properly validate support for different key types and ensure that the cache correctly distinguishes between different types (e.g., string "1" vs number 1).
414-462: Well-designed performance validation tests.The performance tests effectively validate that the cache maintains correct LRU behavior and size limits under load conditions with reasonable iteration counts.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
69-72: Add fallbackAWS_DEFAULT_REGIONand consider movingenvto job-level
The AWS SDK can also leverageAWS_DEFAULT_REGION; including it ensures broader tool compatibility. You might also DRY out this block by declaring these vars at thetestjob’s top-levelenv(or even workflow-levelenv) instead of per-step.- env: - AWS_REGION: us-east-1 - AWS_ACCESS_KEY_ID: test - AWS_SECRET_ACCESS_KEY: test + env: + AWS_REGION: us-east-1 + AWS_DEFAULT_REGION: us-east-1 + AWS_ACCESS_KEY_ID: test + AWS_SECRET_ACCESS_KEY: test
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cursor BugBot
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/dynamoose/lib/ModelStore.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Ensure code is clean, readable, and maintainable. - Adhere to proper naming conventions and consistent formatting. - Split functionality into smaller, reusable modules...
**/*.ts: - Ensure code is clean, readable, and maintainable.
- Adhere to proper naming conventions and consistent formatting.
- Split functionality into smaller, reusable modules.
- Keep files small by extracting related functionalities into separate modules.
- Use imports to connect modules effectively.
- Write concise, technical, and type-safe TypeScript code.
- Import types with
import typeto avoid importing implementations.- Use
@ts-expect-errorinstead of@ts-ignoreand add a comment explaining why the error is expected.- Use early return patterns to avoid deep nesting.
- Use descriptive variable names with auxiliary verbs (e.g.,
isLoading,hasError).- Use AWS SDK V3 instead of V2.
- Use Zod for data validation.
- Use the full import path for packages when possible to enable tree shaking and reduce bundle size.
packages/dynamoose/lib/ModelStore.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Cursor BugBot
- GitHub Check: test (23.x)
- GitHub Check: test (22.x)
- GitHub Check: test (20.x)
- GitHub Check: test-types
- GitHub Check: lint
- GitHub Check: test (23.x)
- GitHub Check: test (22.x)
- GitHub Check: test (20.x)
🔇 Additional comments (1)
packages/dynamoose/lib/ModelStore.ts (1)
22-22: ```shell
#!/bin/bashSearch for any definitions or references of clearCache across the TypeScript codebase
echo "🔍 Searching for clearCache references..."
rg -n "clearCache" -g ".ts" -g ".d.ts"echo
echo "🔍 Locating the Model interface definition..."
rg -n "export interface Model" -g "*.ts"echo
echo "🔍 Locating the Model class implementation..."
rg -n "class Model" -g "*.ts"</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Added 'mitm' package for HTTP request interception during tests - Refactored Item.objectFromSchema to improve performance by directly calling attributesWithSchema instead of using a cached function - Updated package-lock.json and package.json to reflect the new dependency
Pull Request Test Coverage Report for Build 15826386951Details
💛 - Coveralls |
There was a problem hiding this comment.
Bug: MITM Handler Completes Requests Prematurely
The mitm request handler in _setup.js calls res.end() before throwing an error. This allows HTTP requests to complete successfully (with an empty response) instead of being blocked. Consequently, the error thrown may not properly propagate, failing to stop tests that attempt disallowed HTTP requests.
packages/dynamoose/test/_setup.js#L7-L18
dynamoose/packages/dynamoose/test/_setup.js
Lines 7 to 18 in 358d06d
Was this report helpful? Give feedback by reacting with 👍 or 👎
Summary by CodeRabbit