Implement readStrict option for Table configuration #6
Conversation
…ithub.com:dynamoose/dynamoose
…l, and Table components
|
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. |
|
Warning Rate limit exceeded@jimmyn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 47 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Model
participant Table
participant Item
participant DynamoDB
User->>Model: get/query/scan(...)
Model->>Table: Retrieve table options (including readStrict)
Model->>DynamoDB: Fetch item(s)
DynamoDB-->>Model: Return item(s) (may include unknown properties)
Model->>Item: conformToSchema({ type: "fromDynamo", readStrict })
Item-->>Model: Return item(s) with properties filtered (or not) per readStrict
Model-->>User: Return processed item(s)
sequenceDiagram
participant User
participant Table
participant Model
participant Item
User->>Table: new Table({ readStrict: false })
User->>Model: query/scan/get(...)
Model->>Item: conformToSchema({ type: "fromDynamo", readStrict: false })
Item-->>Model: Return item(s) with all properties (including unknown)
Model-->>User: Return item(s) with unknown properties preserved
sequenceDiagram
participant User
participant Table
participant Model
participant Item
User->>Table: new Table({ readStrict: true })
User->>Model: query/scan/get(...)
Model->>Item: conformToSchema({ type: "fromDynamo", readStrict: true })
Item-->>Model: Return item(s) with only schema-defined properties
Model-->>User: Return item(s) with unknown properties omitted
✨ 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: 4
♻️ Duplicate comments (2)
packages/dynamoose/lib/Model/index.ts (2)
900-902: SamereadStrict/itemifyduplication as aboveSee earlier comment — extracting a helper will simplify maintenance.
1030-1032: SamereadStrict/conformToSchemapattern repeatedCovered by earlier remarks.
🧹 Nitpick comments (18)
packages/dynamoose/lib/Table/index.ts (1)
564-578: ```shell
#!/bin/bashDisplay the JSDoc table and default JSON block above TableOptions
sed -n '1,150p' packages/dynamoose/lib/Table/index.ts
</blockquote></details> <details> <summary>.github/workflows/publish.yml (1)</summary><blockquote> `19-19`: **Strip trailing whitespace to satisfy YAML linters** `yamllint` flags these blank lines for `trailing-spaces`. They are harmless at runtime but can fail CI lint jobs—delete the extra spaces. ```diff - +Also applies to: 38-38
packages/dynamoose/test/Query.js (3)
125-128: Strip trailing whitespace to satisfy linter
eslint --fix(ruleno-trailing-spaces) is currently failing on line 128 – it’s just a blank line that contains spaces.
Remove the padding to unblock CI.- +
133-137: Second trailing-space offenderLine 137 carries the same issue. De-indent the empty line or delete it.
- +
124-140: Consider deduplicating identical “readStrict” testsThe two new cases for
readStrict: true/falseare reproduced verbatim inScan.js.
Extract a small helper (e.g.expectReadStrictBehaviour(modelFactory, execFn)) in a shared test util to avoid copy-paste and make future tweaks safer.packages/dynamoose/test/Scan.js (3)
125-128: Lint failure – trailing whitespaceCI flags line 128 for trailing spaces. Drop the padding.
- +
133-137: Repeat trailing-space issueSame on line 137 – clean it up.
- +
124-140: Reuse test logic instead of duplicatingThe
readStricttrue/false scenarios here are identical to those inQuery.js.
Factor them into a shared helper to reduce noise and ensure consistent assertions across query/scan paths.packages/dynamoose/README.md (2)
2-39: Replace hard tabs with spaces for consistent Markdown formatting.The file uses hard tabs for indentation across multiple lines, triggering MD010 errors and reducing readability. Convert these tabs to spaces (e.g., 2 or 4 spaces) throughout the file.
Example diff for the first few lines:
…and apply the same replacement for all indented lines.
74-81: Add trailing pipes to table rows for proper Markdown table style.The branch strategy table uses leading-only pipes (MD055). Add a trailing pipe at the end of each row to satisfy markdownlint.
-| [`main`](https://github.com/dynamoose/dynamoose/tree/main) | 4.x.x | | - [Documentation](https://dynamoose.pages.dev) | +| [`main`](https://github.com/dynamoose/dynamoose/tree/main) | 4.x.x | | - [Documentation](https://dynamoose.pages.dev) | ...packages/dynamoose/test/Item.js (2)
124-127: Consider isolating the birthday fixture to avoid repetitionA magic value (
new Date(10000)) is repeated in multiple tests. Defining aconst BIRTHDAY = new Date(10_000)at the top of thedescribe("item.save")block (or in a shared helper) would:
- keep the test data DRY
- make it obvious that the same value is expected everywhere
- avoid accidental divergence if one expectation is updated in-place later
151-153: Template literal inside expectation is brittleThe assertions interpolate the timestamp each time with
{"birthday": {"N": `${new Date(10000).getTime()}`}}This recreates a
Dateobject on every evaluation. While harmless here, it is noisy and slightly obscures intent. Using the same constant suggested above (or simply the literal string"10000") would make the expectation clearer and cheaper to evaluate.Example:
- "birthday": {"N": `${new Date(10000).getTime()}`} + "birthday": {"N": "10000"}Also applies to: 168-170, 212-214, 1725-1727
packages/dynamoose/lib/Item.ts (4)
443-448: Consider eliminating the extra Item instantiationInside
save()we create a newItemonly to immediately callconformToSchemaand then copy its enumerable props back ontothis.const savedItem = await new (this.getInternalProperties(internalProperties).model).Item(item as any) .conformToSchema(conformToSchemaSettings); Object.keys(savedItem).forEach((key) => this[key] = savedItem[key]);This duplicates work:
- A second constructor allocation.
- Two separate
InternalPropertiesClassobjects.We can avoid both by calling
conformToSchemadirectly on the current instance after the marshal round-trip:- const savedItem = await new (this.getInternalProperties(internalProperties).model) - .Item(item as any).conformToSchema(conformToSchemaSettings); - Object.keys(savedItem).forEach((key) => this[key] = savedItem[key]); + await this.conformToSchema(conformToSchemaSettings); + const savedItem = this;This keeps behaviour identical while shaving some allocations and internal bookkeeping.
592-599: Interface doc missing forreadStrict
readStrict?is now part ofItemObjectFromSchemaSettingsbut there’s no JSDoc comment explaining its semantics. Since this struct is frequently referenced by users, adding a short description will improve discoverability.
722-731: Minor performance tweak & optional chainingEven when
typeCheckhas been disabled (e.g. viareadStrict:false), we still call
getValueTypeCheckResultfor every attribute solely to drive custom-type transforms.
That partially defeats the perf benefit.You can gate the call:
- const typeDetails = utils.dynamoose.getValueTypeCheckResult(schema, value, key, settings, {typeIndexOptionMap}).matchedTypeDetails as DynamoDBTypeResult; + const typeDetails = settings.typeCheck === false + ? undefined + : utils.dynamoose + .getValueTypeCheckResult(schema, value, key, settings, {typeIndexOptionMap}) + .matchedTypeDetails as DynamoDBTypeResult;Additionally, the linter hint can be satisfied by optional-chaining:
- const {"type": typeInfo} = typeDetails.isOfType(value as ValueType); + const {"type": typeInfo} = typeDetails?.isOfType(value as ValueType) ?? {};Not critical, but helps fulfil the performance promise of non-strict reads and removes the lint warning.
740-742: Fallback still incurs a second expensive lookupThe try/catch pathway re-computes
findTypeForValuewhen the first lookup fails, which can be costly for large payloads.
Caching the firstvalueTypeCheckResult(even if undefined) would avoid the duplicate work.packages/dynamoose/lib/Model/index.ts (2)
74-76: Use PascalCase for interface namesTypeScript convention is to start exported type/interface names with an uppercase letter.
Proposed patch:-export interface schemaCorrectnessScoresSettings { - considerDefaults?: boolean -} +export interface SchemaCorrectnessScoresSettings { + considerDefaults?: boolean; +}
474-476: Avoid duplicatingreadStrictplumbing & double-checksaveUnknowninterplay
The same 3-line block retrieving
readStrictand buildingitemifyis repeated inbatchGet,update, andget. Consider a small helper (e.g.,this._itemifyFromDynamo(...)) or an internal getter to DRY this up.With
readStrict === trueyou still passsaveUnknown: true. Please confirm thatItem.conformToSchemaignoressaveUnknownwhenreadStrictis enabled; otherwise unknown attributes might leak back in and defeat the strict read.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
.github/workflows/publish.yml(2 hunks).vscode/settings.json(1 hunks)package.json(2 hunks)packages/dynamoose/README.md(1 hunks)packages/dynamoose/lib/General.ts(1 hunks)packages/dynamoose/lib/Item.ts(6 hunks)packages/dynamoose/lib/ItemRetriever.ts(1 hunks)packages/dynamoose/lib/Model/index.ts(5 hunks)packages/dynamoose/lib/Schema.ts(4 hunks)packages/dynamoose/lib/Table/defaults.ts(1 hunks)packages/dynamoose/lib/Table/index.ts(1 hunks)packages/dynamoose/lib/Table/utilities.ts(1 hunks)packages/dynamoose/lib/Transaction.ts(1 hunks)packages/dynamoose/package.json(2 hunks)packages/dynamoose/test/Item.js(5 hunks)packages/dynamoose/test/Model.js(2 hunks)packages/dynamoose/test/Query.js(1 hunks)packages/dynamoose/test/Scan.js(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/Transaction.tspackages/dynamoose/lib/ItemRetriever.tspackages/dynamoose/lib/Table/index.tspackages/dynamoose/lib/General.tspackages/dynamoose/lib/Table/utilities.tspackages/dynamoose/lib/Table/defaults.tspackages/dynamoose/lib/Item.tspackages/dynamoose/lib/Schema.tspackages/dynamoose/lib/Model/index.ts
🧬 Code Graph Analysis (2)
packages/dynamoose/lib/General.ts (1)
packages/dynamoose/lib/Item.ts (1)
Item(41-503)
packages/dynamoose/lib/Schema.ts (2)
packages/dynamoose/lib/General.ts (1)
ModelType(35-35)packages/dynamoose/lib/Item.ts (1)
Item(41-503)
🪛 YAMLlint (1.37.1)
.github/workflows/publish.yml
[error] 19-19: trailing spaces
(trailing-spaces)
[error] 38-38: trailing spaces
(trailing-spaces)
🪛 GitHub Check: lint
packages/dynamoose/test/Query.js
[failure] 137-137:
Trailing spaces not allowed
[failure] 128-128:
Trailing spaces not allowed
packages/dynamoose/test/Scan.js
[failure] 137-137:
Trailing spaces not allowed
[failure] 128-128:
Trailing spaces not allowed
🪛 GitHub Actions: CI
packages/dynamoose/test/Query.js
[error] 128-128: ESLint: Trailing spaces not allowed (no-trailing-spaces)
packages/dynamoose/test/Model.js
[error] 779-783: Test failure: Promise resolved instead of rejected for 'Should throw error if returning Number for ISO Date'. Expected rejection with CustomError.TypeMismatch for type number instead of date.
[error] 781-789: Test failure: Promise resolved instead of rejected for 'Should throw error if returning ISO Date for String'. Expected rejection with CustomError.TypeMismatch for type string instead of date.
[error] 789-797: Test failure: Promise resolved instead of rejected for 'Should throw type mismatch error if passing in wrong type with custom type'. Expected rejection with CustomError.TypeMismatch for type string instead of date.
[error] 817-821: Test failure: Deep equality mismatch in 'Should return object with correct values with object property with elements that don't exist in schema'. Unexpected property 'zip' found in result.
[error] 819-829: Test failure: Promise resolved instead of rejected for 'Should throw type mismatch error if passing in wrong type with custom type for object'. Expected rejection with CustomError.TypeMismatch for 'address' type string instead of object.
[error] 827-837: Test failure: Promise resolved instead of rejected for 'Should throw type mismatch error if passing in wrong type for nested object attribute'. Expected rejection with CustomError.TypeMismatch for 'address.country' type boolean instead of string.
[error] 835-839: Test failure: Deep equality mismatch in 'Should return object with correct values if Dynamo object consists properties that don't exist in schema'. Unexpected property 'hello' found in result.
[error] 1188-1191: Test failure: Promise resolved instead of rejected for 'Should throw error if Dynamo object contains properties that have type mismatch with schema'. Expected rejection with CustomError.TypeMismatch for 'age' type string instead of number.
[error] 1221-1224: Test failure: Promise resolved instead of rejected for 'Should throw error if returning Number for ISO Date' (Callback). Expected rejection with CustomError.TypeMismatch for type number instead of date.
[error] 781-791: Test failure: Promise resolved instead of rejected for 'Should throw error if returning ISO Date for String' (Callback). Expected rejection with CustomError.TypeMismatch for type string instead of date.
[error] 789-799: Test failure: Promise resolved instead of rejected for 'Should throw type mismatch error if passing in wrong type with custom type' (Callback). Expected rejection with CustomError.TypeMismatch for type string instead of date.
[error] 819-829: Test failure: Promise resolved instead of rejected for 'Should throw type mismatch error if passing in wrong type with custom type for object' (Callback). Expected rejection with CustomError.TypeMismatch for 'address' type string instead of object.
[error] 827-837: Test failure: Promise resolved instead of rejected for 'Should throw type mismatch error if passing in wrong type for nested object attribute' (Callback). Expected rejection with CustomError.TypeMismatch for 'address.country' type boolean instead of string.
[error] 835-839: Test failure: Deep equality mismatch in 'Should return object with correct values if Dynamo object consists properties that don't exist in schema' (Callback). Unexpected property 'hello' found in result.
[error] 1188-1191: Test failure: Promise resolved instead of rejected for 'Should throw error if Dynamo object contains properties that have type mismatch with schema' (Callback). Expected rejection with CustomError.TypeMismatch for 'age' type string instead of number.
[warning] Console warning: Dynamoose Warning: Passing callback function into transaction method not allowed. Removing callback function from list of arguments.
packages/dynamoose/lib/Model/index.ts
[warning] 371-371: Dynamoose Warning: Passing callback function into transaction method not allowed. Removing callback function from list of arguments.
🪛 markdownlint-cli2 (0.17.2)
packages/dynamoose/README.md
2-2: Hard tabs
Column: 1
(MD010, no-hard-tabs)
6-6: Hard tabs
Column: 1
(MD010, no-hard-tabs)
7-7: Hard tabs
Column: 1
(MD010, no-hard-tabs)
8-8: Hard tabs
Column: 1
(MD010, no-hard-tabs)
9-9: Hard tabs
Column: 1
(MD010, no-hard-tabs)
10-10: Hard tabs
Column: 1
(MD010, no-hard-tabs)
11-11: Hard tabs
Column: 1
(MD010, no-hard-tabs)
12-12: Hard tabs
Column: 1
(MD010, no-hard-tabs)
13-13: Hard tabs
Column: 1
(MD010, no-hard-tabs)
14-14: Hard tabs
Column: 1
(MD010, no-hard-tabs)
15-15: Hard tabs
Column: 1
(MD010, no-hard-tabs)
16-16: Hard tabs
Column: 1
(MD010, no-hard-tabs)
17-17: Hard tabs
Column: 1
(MD010, no-hard-tabs)
18-18: Hard tabs
Column: 1
(MD010, no-hard-tabs)
19-19: Hard tabs
Column: 1
(MD010, no-hard-tabs)
20-20: Hard tabs
Column: 1
(MD010, no-hard-tabs)
21-21: Hard tabs
Column: 1
(MD010, no-hard-tabs)
22-22: Hard tabs
Column: 1
(MD010, no-hard-tabs)
23-23: Hard tabs
Column: 1
(MD010, no-hard-tabs)
24-24: Hard tabs
Column: 1
(MD010, no-hard-tabs)
25-25: Hard tabs
Column: 1
(MD010, no-hard-tabs)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
27-27: Hard tabs
Column: 1
(MD010, no-hard-tabs)
28-28: Hard tabs
Column: 1
(MD010, no-hard-tabs)
29-29: Hard tabs
Column: 1
(MD010, no-hard-tabs)
30-30: Hard tabs
Column: 1
(MD010, no-hard-tabs)
31-31: Hard tabs
Column: 1
(MD010, no-hard-tabs)
32-32: Hard tabs
Column: 1
(MD010, no-hard-tabs)
33-33: Hard tabs
Column: 1
(MD010, no-hard-tabs)
34-34: Hard tabs
Column: 1
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1
(MD010, no-hard-tabs)
36-36: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
38-38: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
🪛 Biome (1.9.4)
packages/dynamoose/lib/Item.ts
[error] 727-727: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (13)
.vscode/settings.json (1)
8-12: Disable automatic code actions on save
This new block intentionally turns off ESLint fixes, TypeScript fixes, and import organization on save. It ensures the team’s editor doesn’t auto-modify files during development, preserving the expected workflow.packages/dynamoose/package.json (2)
49-53: publishConfig looks correct – ensure CI respects it
"access": "public"together with an explicit registry is standard; CI/CD pipelines that already setnpm publish --access publicwill work unchanged.
2-3: ```shell
#!/bin/bashList all git tags to check for prior published versions
git tag --list
</details> <details> <summary>packages/dynamoose/lib/General.ts (1)</summary> `17-34`: **Exporting `ModelItemConstructor` is safe – no concerns** Making the constructor interface public improves typing for downstream consumers; there are no breaking-change risks because it only widens visibility. </details> <details> <summary>packages/dynamoose/lib/Table/defaults.ts (1)</summary> `25-25`: **Defaulting `readStrict` to false may introduce breaking runtime behaviour** Previous releases always stripped unknown attributes; setting the default to `false` now preserves them for every table that doesn’t explicitly opt-in. Confirm that this is an intentional breaking change and that: 1. CHANGELOG/docs call it out clearly. 2. A major version bump (or a deprecation pathway) is scheduled. 3. Internal code paths that assumed strict filtering are audited for unintended side-effects. Consider keeping the default `true` and letting users disable it if backwards compatibility is required. </details> <details> <summary>packages/dynamoose/lib/Table/utilities.ts (1)</summary> `78-84`: **Optional-chaining guard looks good** Replacing the nested property check with `streamOptions?.enabled` removes a potential `undefined` access without altering logic. </details> <details> <summary>package.json (1)</summary> `2-2`: **Verify ecosystem updates after scoped package rename** Renaming to `@monei-js/dynamoose` and adding a `publishConfig` block affects: • Import paths in source & documentation. • Lerna/workspace publish settings. • Consumers depending on the unscoped name. Ensure all references, badges, and workflow scripts are updated and that the npm token allows publishing under this scope. Also applies to: 97-100 </details> <details> <summary>packages/dynamoose/test/Model.js (3)</summary> `2001-2003`: **Approve timestamp constructor assertions** Confirming that both `createdAt` and `updatedAt` are verified as `Date` instances is correct and improves type clarity in tests. --- `2013-2013`: **Approve capturing `date3` as upper bound** Introducing `const date3 = Date.now();` before range assertions on updated timestamps ensures an accurate upper bound for `updatedAt`, making the test deterministic. --- `2019-2022`: **Approve post-save timestamp range checks** Validating that `createdAt` and `updatedAt` remain within expected ranges after mutation and save strengthens test coverage of timestamp behavior. </details> <details> <summary>packages/dynamoose/README.md (1)</summary> `1-4`: **Verify banner image path and inclusion in the package.** Ensure that `internal/banner/Banner.png` is packaged and that the relative path in the README resolves correctly post-publish. A missing asset will lead to a broken banner on npm. ```shell #!/bin/bash # Check for Banner.png in the published package directory if [ ! -f packages/dynamoose/internal/banner/Banner.png ]; then echo "ERROR: Banner.png not found in packages/dynamoose/internal/banner/" exit 1 fi echo "Banner.png is present."packages/dynamoose/lib/Schema.ts (1)
276-332: Public-export looks goodThe new
exportmodifiers surface several formerly-internal types that other packages may now import. Nothing here alters runtime behaviour, and the unions still line up with the values accepted elsewhere in the file, so I have no concerns.packages/dynamoose/lib/Model/index.ts (1)
23-28:UpdatePartialtype change looks goodIntersection with optional
$SET/$ADD/$REMOVE/$DELETEprops aligns with existingupdateimplementation — especially the array support for$REMOVE. No concerns here.
(cherry picked from commit e6faec9)
…ption in User, Query, and Scan models
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/dynamoose/lib/Item.ts (1)
899-910:readStrictstill overrides explicit caller settings
We flagged this in the previous round, but the code still hard-resets
validate,required,enum,defaults, andforceDefaulttofalse,
even when the caller explicitly set them totrue.-settings = { - ...settings, - "validate": false, - "required": false, - "enum": false, - "defaults": false, - "forceDefault": false -}; +settings = { + ...settings, + validate: settings.validate ?? false, + required: settings.required ?? false, + enum: settings.enum ?? false, + defaults: settings.defaults ?? false, + forceDefault: settings.forceDefault ?? false +};This preserves the ergonomic default while respecting deliberate overrides.
🧹 Nitpick comments (4)
packages/dynamoose/lib/Item.ts (4)
443-448:readStrictflag is not forwarded when creatingconformToSchemaSettings
conformToSchemaSettingsis hard-coded and silently drops any table-levelreadStrictpreference.
If the table was created withreadStrict: false, saving an item will still invoke the strict validation path, causing an avoidable performance hit.-const conformToSchemaSettings : ItemObjectFromSchemaSettings = {"combine": true, "customTypesDynamo": true, "saveUnknown": true, "type": "fromDynamo", "modifiers": ["get"], "mapAttributes": true}; +const conformToSchemaSettings: ItemObjectFromSchemaSettings = { + "combine": true, + "customTypesDynamo": true, + "saveUnknown": true, + "type": "fromDynamo", + "modifiers": ["get"], + "mapAttributes": true, + // Preserve table-level option + "readStrict": table.getInternalProperties(internalProperties) + .options.readStrict +};
592-595: Interface extended correctly, but lacks JSDoc
readStrictwas added toItemObjectFromSchemaSettings, but no description is given.
Please add a short JSDoc entry so IDEs surface the new option.-readStrict?: boolean; +/** + * When `false`, skip expensive validation steps during `fromDynamo` + * deserialisation, while still applying transformations. + * Defaults to `true`. + */ +readStrict?: boolean;
743-751: Guard against missingcustomTypefunctions
customType.functions[settings.type]is accessed without verifying that
customType.functionsexists. While most custom types do define the property,
the check is trivial and prevents a run-timeTypeError:- if (customType && customType.functions[settings.type] && !isCorrectTypeAlready) { + if (customType?.functions?.[settings.type] && !isCorrectTypeAlready) {
760-762: Optional-chaining simplifies nullish handling
The static-analysis hint is valid –matchedTypeDetailsmay beundefined. Using optional chaining improves readability and eliminates thetry…catchthat’s only guarding against that access:- typeDetails = result ? result.matchedTypeDetails : undefined; + typeDetails = result?.matchedTypeDetails;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/dynamoose/lib/Item.ts(7 hunks)packages/dynamoose/test/Model.js(6 hunks)packages/dynamoose/test/Query.js(1 hunks)packages/dynamoose/test/Scan.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/dynamoose/test/Scan.js
- packages/dynamoose/test/Query.js
- packages/dynamoose/test/Model.js
🧰 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/Item.ts
🪛 Biome (1.9.4)
packages/dynamoose/lib/Item.ts
[error] 747-747: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: test (23.x)
- GitHub Check: test (21.x)
- GitHub Check: test (18.x)
- GitHub Check: test (19.x)
- GitHub Check: test (16.x)
- GitHub Check: test (17.x)
- GitHub Check: test (17.x)
- GitHub Check: test (21.x)
- GitHub Check: test (20.x)
- GitHub Check: test (19.x)
- GitHub Check: test (16.x)
- GitHub Check: test (18.x)
🔇 Additional comments (1)
packages/dynamoose/lib/Item.ts (1)
489-492: Good callback / promise harmonisationThe callback now receives the fully conformed
savedItem, and the promise path mirrors that behaviour – nice clean-up.
Pull Request Test Coverage Report for Build 15733095012Details
💛 - Coveralls |
…and objects without expensive checks
…ets, Models, and self-referencing types
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/dynamoose/lib/Item.ts (1)
924-935: Unconditional flag override still ignores explicit user settings
Same point raised in the previous review.The block resets
validate,required,enum,defaults, andforceDefaulttofalsewheneverreadStrict === false.
If a caller explicitly passesvalidate: truetogether withreadStrict: false, that intent is lost.Consider only overriding when the property is undefined:
settings = { ...settings, - "validate": false, - "required": false, - "enum": false, - "defaults": false, - "forceDefault": false + "validate": settings.validate ?? false, + "required": settings.required ?? false, + "enum": settings.enum ?? false, + "defaults": settings.defaults ?? false, + "forceDefault": settings.forceDefault ?? false };This retains the convenience default while respecting explicit opts.
🧹 Nitpick comments (4)
packages/dynamoose/lib/Item.ts (4)
443-448: Consider forwardingreadStrict(and other table options) when normalising the just-marshalled item
conformToSchemaSettingsis hard-coded here.
If the parent table was created withreadStrict: true, this internal post-save normalisation will silently ignore that preference, potentially leaving unknown attributes on the in-memory item while they would be stripped on a subsequentget().-const conformToSchemaSettings : ItemObjectFromSchemaSettings = {"combine": true, "customTypesDynamo": true, "saveUnknown": true, "type": "fromDynamo", "modifiers": ["get"], "mapAttributes": true}; +const conformToSchemaSettings: ItemObjectFromSchemaSettings = { + "combine": true, + "customTypesDynamo": true, + "saveUnknown": true, + "type": "fromDynamo", + "modifiers": ["get"], + "mapAttributes": true, + // bubble up the table option so behaviour is consistent everywhere + "readStrict": table.getInternalProperties(internalProperties).options.readStrict +};This keeps behaviour deterministic no matter which code path produced the item.
680-681: Strip trailing whitespace – linter is failingLine 680 contains nothing but spaces.
Please delete the superfluous whitespace to unblock CI (no-trailing-spaces).
705-706: Same whitespace issue as aboveAnother blank line with trailing spaces (line 705). Remove it for a clean lint run.
769-773: Use optional chaining to avoid undefined-access & satisfy the linter
customTypemay beundefined. The linter already points this out:-if (customType && customType.functions[settings.type] && !isCorrectTypeAlready) { - const customValue = customType.functions[settings.type](value); +if (customType?.functions?.[settings.type] && !isCorrectTypeAlready) { + const customValue = customType.functions[settings.type](value); }This silences the
useOptionalChainwarning and guards against a runtimeTypeError.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/dynamoose/lib/Item.ts(7 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/Item.ts
🪛 GitHub Check: lint
packages/dynamoose/lib/Item.ts
[failure] 705-705:
Trailing spaces not allowed
[failure] 680-680:
Trailing spaces not allowed
🪛 Biome (1.9.4)
packages/dynamoose/lib/Item.ts
[error] 772-772: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: CI
packages/dynamoose/lib/Item.ts
[error] 680-680: ESLint: Trailing spaces not allowed (no-trailing-spaces)
This PR implements the readStrict option for Table configuration to control how strictly items are validated when reading from DynamoDB.
Key Changes:
Behavior:
This allows for better performance when reading from DynamoDB by skipping unnecessary validation while still applying essential transformations like getters and custom type conversions.
Background:
This feature was previously implemented and tested. This PR allows for internal review and testing before potentially submitting to upstream.
Summary by CodeRabbit
New Features
readStrict, allowing users to control whether unknown properties from DynamoDB responses are included or excluded in query and scan results.Bug Fixes
Documentation
Refactor
readStrictoption during item retrieval and saving.Tests
readStrictoption in query and scan operations.Chores