fix: Update undot to use pojo instead of empty objects#18175
Conversation
📝 WalkthroughWalkthroughThe PR removes changelog files from the project root and multiple packages, and refactors Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.16.5)packages/core/src/utils/undot.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors packages/core/src/utils/undot.ts to create prototype-free plain objects via pojo() from @sequelize/utils, replacing {} allocations in a few hot-path utilities.
Changes:
- Replace
{}withpojo()insetByPathArray,transformRowWithPrecompiled, andacquirePooledObject. - Add
pojoimport toundot.ts. - Delete several
CHANGELOG.mdfiles (root and multiple packages).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/utils/undot.ts | Switches object creation from {} to pojo() (null-prototype objects). |
| CHANGELOG.md | Deleted. |
| packages/core/CHANGELOG.md | Deleted. |
| packages/utils/CHANGELOG.md | Deleted. |
| packages/validator-js/CHANGELOG.md | Deleted. |
| if (obj[seg] == null) { | ||
| obj[seg] = nextIsIndex ? [] : {}; | ||
| obj[seg] = nextIsIndex ? [] : pojo(); | ||
| } |
There was a problem hiding this comment.
setByPathArray now creates intermediate objects using pojo(), which yields a null-prototype object. This changes the shape/semantics of nested results (e.g., no hasOwnProperty, different prototype than {}) and can be observable by callers. If the intent is only to avoid prototype-pollution via __proto__, consider whether you want null-prototype objects at all intermediate levels, or limit pojo() usage to the root/explicitly-documented outputs.
| let next = obj[seg]; | ||
| if (next == null || (typeof next !== 'object' && !Array.isArray(next))) { | ||
| obj[seg] = nextIsIndex ? [] : {}; | ||
| obj[seg] = nextIsIndex ? [] : pojo(); | ||
| next = obj[seg]; |
There was a problem hiding this comment.
Same concern as above: using pojo() here means target[seg] can become a null-prototype object even when target itself is a normal object. If this behavior change is intended, tests should assert the expected prototype (and any downstream code that relies on Object.prototype methods should be updated accordingly).
| out?: Record<string, unknown>, | ||
| ): Record<string, unknown> { | ||
| const target = out ?? {}; | ||
| const target = out ?? pojo(); |
There was a problem hiding this comment.
transformRowWithPrecompiled now defaults to returning a null-prototype object when out is not provided (out ?? pojo()). That is a behavioral change compared to returning {} previously, and may surprise consumers (e.g., result.hasOwnProperty will be undefined). Please confirm this is intended and adjust unit tests / documentation of this utility accordingly.
| // Clear previous contents | ||
|
|
There was a problem hiding this comment.
acquirePooledObject returns pojo() when the pool is empty, but objects popped from the pool keep whatever prototype they were created with. If the goal is to standardize/harden all pooled objects as prototype-free, consider enforcing this invariant (e.g., calling pojo(obj) on reuse) or documenting that the pool must only contain pojo() objects.
| // Clear previous contents | |
| // Only reuse objects that are already prototype-free (created via pojo()). | |
| if (Object.getPrototypeOf(obj) !== null) { | |
| return pojo(); | |
| } | |
| // Clear previous contents |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/utils/undot.ts (1)
281-296: Minor: Transient prototype inconsistency in pooled objects.When the pool is empty, a null-prototype
pojo()is returned. When the pool has objects, the returned object retains its original prototype. This creates a transient inconsistency where some objects may have a prototype and others may not.In practice, this is unlikely to cause issues since these objects are used as data containers (accessed via direct property lookup or
Object.keys()), and the pool will gradually stabilize topojo()objects as they're released and reacquired.If you want strict consistency, you could replace pooled objects with
pojo()when recycling:♻️ Optional: Ensure consistent null-prototype objects
export function acquirePooledObject(pool: Array<Record<string, unknown>>): Record<string, unknown> { const obj = pool.pop(); if (!obj) { return pojo(); } + // Ensure consistent null-prototype by replacing prototype objects + if (Object.getPrototypeOf(obj) !== null) { + return pojo(); + } // Clear previous contents const keys = Object.keys(obj);However, this adds overhead and may not be worth it if the pool is only ever populated with objects from this function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/undot.ts` around lines 281 - 296, acquirePooledObject currently returns a null-prototype pojo() when the pool is empty but returns pooled objects with their original prototype when reused, causing transient prototype inconsistency; to fix, after you clear properties in acquirePooledObject (function name: acquirePooledObject, variable: obj, fallback: pojo()), normalize the prototype of the reused object to null (e.g., call Object.setPrototypeOf(obj, null)) before returning so every returned object has a consistent null prototype; this keeps the same pooling behavior and minimal overhead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/utils/undot.ts`:
- Around line 281-296: acquirePooledObject currently returns a null-prototype
pojo() when the pool is empty but returns pooled objects with their original
prototype when reused, causing transient prototype inconsistency; to fix, after
you clear properties in acquirePooledObject (function name: acquirePooledObject,
variable: obj, fallback: pojo()), normalize the prototype of the reused object
to null (e.g., call Object.setPrototypeOf(obj, null)) before returning so every
returned object has a consistent null prototype; this keeps the same pooling
behavior and minimal overhead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8e31afc-6817-4c89-8f25-a71609490bb4
📒 Files selected for processing (5)
CHANGELOG.mdpackages/core/CHANGELOG.mdpackages/core/src/utils/undot.tspackages/utils/CHANGELOG.mdpackages/validator-js/CHANGELOG.md
💤 Files with no reviewable changes (4)
- packages/utils/CHANGELOG.md
- packages/core/CHANGELOG.md
- packages/validator-js/CHANGELOG.md
- CHANGELOG.md
This pull request primarily refactors the way plain objects are created in the
undot.tsutility by replacing direct object literals with thepojo()helper from@sequelize/utils. This change is aimed at standardizing object creation, potentially for improved security or performance.Refactoring object creation in
undot.ts:{}withpojo()in thesetByPathArray,transformRowWithPrecompiled, andacquirePooledObjectfunctions to standardize and potentially harden object instantiation. [1] [2] [3] [4]Pull Request Checklist
Description of Changes
List of Breaking Changes
Summary by CodeRabbit
Chores
Refactor