Unflattening array fix for very large arrays #1432
Conversation
|
WalkthroughThe changes involve updates to the Changes
Poem
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 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 and nitpick comments (1)
packages/core/src/v3/utils/flattenAttributes.ts (1)
98-98: Consider adding documentation about numeric key handling.It would be helpful to add a comment explaining how the function handles different types of numeric keys (array indices vs large numeric keys) to make the behavior clear for future maintainers.
Add documentation like this:
+ // Handle key splitting with special care for: + // 1. Array indices (e.g., "[0]") - converted to numbers + // 2. Large numeric keys (e.g., "1234567890123") - preserved as strings const parts = key.split(".").reduce(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/core/src/v3/utils/flattenAttributes.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/core/src/v3/utils/flattenAttributes.ts (2)
Line range hint
128-133: LGTM! The array detection logic complements the key splitting changes.The type-based check
typeof nextPart === "number"ensures that only properly parsed array indices (from "[n]" syntax) trigger array creation, preventing large numeric keys from being misinterpreted as array indices.
98-117: LGTM! The new key splitting logic correctly handles large numeric keys.The implementation properly differentiates between array indices (e.g., "[0]") and large numeric keys (e.g., "1234567890123"), addressing the core issue described in the PR objectives.
Let's verify the fix with a test case:
@trigger.dev/build
@trigger.dev/core
trigger.dev
@trigger.dev/react-hooks
@trigger.dev/sdk
commit: |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
packages/core/src/v3/utils/flattenAttributes.ts (1)
122-124: Document edge case handling for falsy values.The condition
!part && part !== 0handles edge cases, but the behavior should be documented for maintainability.Add a comment explaining the logic:
+// Skip empty parts but preserve 0 as a valid key +// This ensures compatibility with arrays while preventing +// issues with empty string keys if (!part && part !== 0) { continue; }packages/core/test/flattenAttributes.test.ts (2)
4-4: Fix typo in test name.The test name has a typo: "correctl" should be "correctly".
- it("handles number keys correctl", () => { + it("handles number keys correctly", () => {
Line range hint
1-216: Improve test organization and documentation.Consider restructuring the tests to better separate concerns:
- Basic functionality tests (primitives, simple objects)
- Edge cases and boundary conditions (large numbers, memory efficiency)
- Integration scenarios (complex nested structures)
Also, add JSDoc comments to document test groups and their purposes, especially for the new edge cases being tested.
Example structure:
/** * Basic functionality tests for primitive types and simple objects */ describe("basic functionality", () => { // existing primitive tests }); /** * Edge cases and boundary conditions * Verifies handling of large numeric keys and memory efficiency */ describe("edge cases", () => { // large number tests // memory efficiency tests }); /** * Integration scenarios with complex nested structures */ describe("complex structures", () => { // existing nested structure tests });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/unit-tests.yml (1 hunks)
- packages/core/src/v3/utils/flattenAttributes.ts (1 hunks)
- packages/core/test/flattenAttributes.test.ts (1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/unit-tests.yml
[error] 41-41: trailing spaces
(trailing-spaces)
🪛 Biome
packages/core/src/v3/utils/flattenAttributes.ts
[error] 103-103: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
.github/workflows/unit-tests.yml (1)
42-43: LGTM! Good addition of package-specific tests.The new test step correctly targets the "@trigger.dev/*" packages, which is essential for testing the changes to the core package's flattening utilities. The step is properly positioned in the workflow and inherits the necessary environment variables.
packages/core/src/v3/utils/flattenAttributes.ts (1)
98-115: Verify handling of large numeric keys with test cases.The implementation looks solid, but we should verify it handles the PR's example cases correctly.
Also applies to: 126-130
✅ Verification successful
Let me gather more information by examining the existing test file for
flattenAttributessince it was found in the output.The existing test file might provide insights into current test coverage and help us verify if the changes handle the edge cases correctly. I'll analyze the implementation and test file contents when provided.
After analyzing the implementation and test files, I can now provide a final response since I have enough information to verify the changes.
The implementation correctly handles large numeric keys and array indices
The changes in
flattenAttributes.tsare verified to be correct for the following reasons:
- The implementation properly handles numeric keys through the regex pattern
/^\[(\d+)\]$/which can handle numbers of any length- The existing test suite already covers various edge cases including:
- Numeric keys both as strings and numbers (
"25"and25)- Array indices with proper bracket notation
- Complex nested objects with mixed types
- The new implementation maintains backward compatibility while adding more precise handling of array indices
The PR's example case of
"akey.with.a.large.number.1234567890123"will work correctly because:
- Non-bracketed numbers are treated as regular string parts
- Only bracketed numbers
[n]are parsed as array indices- The implementation correctly distinguishes between array indices and regular numeric keys
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Create a test file to verify the fix cat << 'EOF' > test.ts import { unflattenAttributes } from "./packages/core/src/v3/utils/flattenAttributes"; const testCases = [ { input: { "akey.with.a.large.number.1234567890123": "value" }, desc: "Large numeric key" }, { input: { "array.[0]": "value" }, desc: "Valid array index" } ]; console.log(JSON.stringify(testCases.map(tc => ({ description: tc.desc, result: unflattenAttributes(tc.input) })), null, 2)); EOF # Find and list all existing test files echo "Existing test files:" fd -e test.ts -e spec.tsLength of output: 602
Script:
#!/bin/bash # Check the existing test cases in flattenAttributes.test.ts cat packages/core/test/flattenAttributes.test.ts # Also let's check the actual implementation to understand the full context cat packages/core/src/v3/utils/flattenAttributes.tsLength of output: 11939
🧰 Tools
🪛 Biome
[error] 103-103: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/test/flattenAttributes.test.ts (1)
1-1: 🛠️ Refactor suggestionAdd edge case tests for memory efficiency.
To prevent regression of the large array issue, consider adding tests that verify memory efficiency when handling large numeric keys. This could include:
- Performance assertions for large numeric keys
- Memory usage verification
- Edge cases with mixed array indices and numeric keys
Let's check if there are any existing performance tests:
Add these test cases:
describe("performance and memory efficiency", () => { it("handles very large numeric keys efficiently", () => { const start = process.memoryUsage().heapUsed; const result = unflattenAttributes({ "object.999999999999999": "value", "array.[0]": "actual_array" }); const end = process.memoryUsage().heapUsed; // Verify correct structure expect(result).toEqual({ object: { "999999999999999": "value" }, array: ["actual_array"] }); // Verify memory usage is reasonable (should be minimal for simple objects) const bytesUsed = end - start; expect(bytesUsed).toBeLessThan(1000); // Adjust threshold as needed }); });
| @@ -38,7 +38,9 @@ jobs: | |||
| SESSION_SECRET: "secret" | |||
| MAGIC_LINK_SECRET: "secret" | |||
| ENCRYPTION_KEY: "secret" | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Remove trailing whitespace.
There are trailing spaces on this line that should be removed.
Apply this diff to fix the formatting:
-
+📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
🧰 Tools
🪛 yamllint
[error] 41-41: trailing spaces
(trailing-spaces)
| const parts = key.split(".").reduce( | ||
| (acc, part) => { | ||
| if (part.startsWith("[") && part.endsWith("]")) { | ||
| // Handle array indices more precisely | ||
| const match = part.match(/^\[(\d+)\]$/); | ||
| if (match && match[1]) { | ||
| acc.push(parseInt(match[1])); | ||
| } else { | ||
| // Remove brackets for non-numeric array keys | ||
| acc.push(part.slice(1, -1)); | ||
| } | ||
| } else { | ||
| acc.push(part); | ||
| } | ||
| return acc; | ||
| }, | ||
| [] as (string | number)[] | ||
| ); |
There was a problem hiding this comment.
Consider adding numeric bounds validation for array indices.
While the regex pattern correctly identifies numeric indices, it doesn't prevent the issue with very large numbers. Consider adding a size check to prevent array allocation attacks.
Apply this diff to add bounds validation:
const match = part.match(/^\[(\d+)\]$/);
if (match && match[1]) {
+ const index = parseInt(match[1]);
+ if (index > Number.MAX_SAFE_INTEGER) {
+ // Treat very large numbers as string keys
+ acc.push(part);
+ } else {
- acc.push(parseInt(match[1]));
+ acc.push(index);
+ }
} else {📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const parts = key.split(".").reduce( | |
| (acc, part) => { | |
| if (part.startsWith("[") && part.endsWith("]")) { | |
| // Handle array indices more precisely | |
| const match = part.match(/^\[(\d+)\]$/); | |
| if (match && match[1]) { | |
| acc.push(parseInt(match[1])); | |
| } else { | |
| // Remove brackets for non-numeric array keys | |
| acc.push(part.slice(1, -1)); | |
| } | |
| } else { | |
| acc.push(part); | |
| } | |
| return acc; | |
| }, | |
| [] as (string | number)[] | |
| ); | |
| const parts = key.split(".").reduce( | |
| (acc, part) => { | |
| if (part.startsWith("[") && part.endsWith("]")) { | |
| // Handle array indices more precisely | |
| const match = part.match(/^\[(\d+)\]$/); | |
| if (match && match[1]) { | |
| const index = parseInt(match[1]); | |
| if (index > Number.MAX_SAFE_INTEGER) { | |
| // Treat very large numbers as string keys | |
| acc.push(part); | |
| } else { | |
| acc.push(index); | |
| } | |
| } else { | |
| // Remove brackets for non-numeric array keys | |
| acc.push(part.slice(1, -1)); | |
| } | |
| } else { | |
| acc.push(part); | |
| } | |
| return acc; | |
| }, | |
| [] as (string | number)[] | |
| ); |
🧰 Tools
🪛 Biome
[error] 103-103: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| if (typeof nextPart === "number") { | ||
| // Ensure we create an array for numeric indices | ||
| current[part] = Array.isArray(current[part]) ? current[part] : []; | ||
| } else if (current[part] === undefined) { | ||
| // Create an object for non-numeric paths |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen type safety in array/object creation.
The current implementation could benefit from more explicit type checking and array initialization.
Consider these improvements:
-if (typeof nextPart === "number") {
+if (typeof nextPart === "number" && Number.isInteger(nextPart) && nextPart >= 0) {
// Ensure we create an array for numeric indices
- current[part] = Array.isArray(current[part]) ? current[part] : [];
+ current[part] = Array.isArray(current[part]) ? current[part] : new Array();
} else if (current[part] === undefined) {
// Create an object for non-numeric paths
- current[part] = {};
+ current[part] = Object.create(null);
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (typeof nextPart === "number") { | |
| // Ensure we create an array for numeric indices | |
| current[part] = Array.isArray(current[part]) ? current[part] : []; | |
| } else if (current[part] === undefined) { | |
| // Create an object for non-numeric paths | |
| if (typeof nextPart === "number" && Number.isInteger(nextPart) && nextPart >= 0) { | |
| // Ensure we create an array for numeric indices | |
| current[part] = Array.isArray(current[part]) ? current[part] : new Array(); | |
| } else if (current[part] === undefined) { | |
| // Create an object for non-numeric paths | |
| current[part] = Object.create(null); |
| it("handles number keys correctl", () => { | ||
| expect(flattenAttributes({ bar: { "25": "foo" } })).toEqual({ "bar.25": "foo" }); | ||
| expect(unflattenAttributes({ "bar.25": "foo" })).toEqual({ bar: { "25": "foo" } }); | ||
| expect(flattenAttributes({ bar: ["foo", "baz"] })).toEqual({ | ||
| "bar.[0]": "foo", | ||
| "bar.[1]": "baz", | ||
| }); | ||
| expect(unflattenAttributes({ "bar.[0]": "foo", "bar.[1]": "baz" })).toEqual({ | ||
| bar: ["foo", "baz"], | ||
| }); | ||
| expect(flattenAttributes({ bar: { 25: "foo" } })).toEqual({ "bar.25": "foo" }); | ||
| expect(unflattenAttributes({ "bar.25": "foo" })).toEqual({ bar: { 25: "foo" } }); | ||
| }); |
There was a problem hiding this comment.
Add test cases for large numeric keys.
While the test covers basic number keys, it doesn't verify the specific issue mentioned in the PR objectives regarding very large numeric keys. Consider adding test cases that validate the fix for keys like "akey.with.a.large.number.1234567890123".
Add these test cases:
expect(flattenAttributes({ "akey.with.a.large.number.1234567890123": "value" }))
.toEqual({ "akey.with.a.large.number.1234567890123": "value" });
expect(unflattenAttributes({ "akey.with.a.large.number.1234567890123": "value" }))
.toEqual({ "akey.with.a.large.number.1234567890123": "value" });
// Verify that array indices still work correctly
expect(flattenAttributes({ array: ["value"] }))
.toEqual({ "array.[0]": "value" });
* fix unflattening of array indices * Get tests passing --------- Co-authored-by: nicktrn <55853254+nicktrn@users.noreply.github.com> Co-authored-by: Eric Allam <eallam@icloud.com> # Conflicts: # .github/workflows/unit-tests.yml
If JSON like this existed:
{ "akey.with.a.large.number.1234567890123": "foo"When this gets unflattened it creates a massive array. An array should not be created in this situation, it should be a string key.
{ "akey.with.a.large.number.[0]": "foo"This is what an array item looks like.
Summary by CodeRabbit
New Features
Bug Fixes
Tests