Replaced mongodb.ObjectId() with the function mongo_utils.mongoObjectId() #9216
Conversation
WalkthroughReplaces direct MongoDB ObjectId usage across source, tests, tools, SDK typings, and clients with a centralized Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as App / Store / Test
participant MU as mongo_utils
participant DB as DB layer
Caller->>MU: ObjectId() or ObjectId(str) / is_object_id(id, generate?)
MU-->>Caller: ObjectId-like instance or hex string / validation result
Caller->>DB: perform insert/find/update using _id (mongo_utils.ObjectId / string)
DB-->>Caller: return documents (with _id handled as provided)
note right of MU: Centralized generation & validation wrapper (compat class)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/test/integration_tests/internal/test_agent_blocks_verifier.js (1)
76-81: Bug: toHexString() on chunk_id now fails (IDs are strings)chunk_id is a string; calling .toHexString() will throw.
Apply this fix:
- const chunk_id = _.get(block, 'chunk'); - const chunk = new ChunkDB(chunks_idmap[chunk_id.toHexString()]); + const chunk_id = _.get(block, 'chunk'); + const chunk = new ChunkDB(chunks_idmap[String(chunk_id)]);src/server/object_services/md_store.js (3)
1436-1438: Remove leftover toHexString() – IDs are plain strings nowAccessing toHexString() on strings will throw. Replace with direct string keys.
- chunk.parts = parts_by_chunk[chunk._id.toHexString()] || []; - chunk.objects = _.uniq(_.compact(_.map(chunk.parts, part => objects_by_id[part.obj.toHexString()]))); + const chunk_id = String(chunk._id); + chunk.parts = parts_by_chunk[chunk_id] || []; + chunk.objects = _.uniq(_.compact(_.map(chunk.parts, part => objects_by_id[String(part.obj)])));
1881-1885: Remove leftover toHexString() in load_blocks_for_chunksSame issue here; use string IDs as map keys.
- for (const chunk of chunks) { - const blocks_by_frag = _.groupBy(blocks_by_chunk[chunk._id.toHexString()], 'frag'); - for (const frag of chunk.frags) { - const frag_blocks = blocks_by_frag[frag._id.toHexString()] || []; - frag.blocks = sorter ? frag_blocks.sort(sorter) : frag_blocks; - } - } + for (const chunk of chunks) { + const chunk_blocks = blocks_by_chunk[String(chunk._id)] || []; + const blocks_by_frag = _.groupBy(chunk_blocks, 'frag'); + for (const frag of chunk.frags) { + const frag_blocks = blocks_by_frag[String(frag._id)] || []; + frag.blocks = sorter ? frag_blocks.sort(sorter) : frag_blocks; + } + }
83-86: IDs are now strings — callers using ObjectId/Buffer methods will breakmake_md_id now returns String(id_str) (MDStore.make_md_id proxies it). Codebase contains places that call ObjectId/Buffer methods on _id values; these are breakpoints that must be fixed or the ID implementation reverted.
- High-risk locations (examples found):
- src/server/object_services/md_store.js — chunk._id.toHexString / frag._id.toHexString (lines ~1436, ~1881-1883).
- src/server/object_services/object_server.js — obj._id.getTimestamp and several md._id.toHexString / getTimestamp uses (lines ~449, ~1484, ~1494, ~1497, ~1698, ~2063).
- src/test/integration_tests/db/test_md_aggregator_unit.js — block_id*.getTimestamp() after make_md_id_from_time (lines ~305–307, ~355–356).
- src/server/bg_services/scrubber.js — this.marker.getTimestamp() (line ~47).
- src/util/mongo_client.js — instanceof mongodb.ObjectId / is_object_id checks (lines ~262–264, ~323).
Actionable options: restore ObjectId-like values from make_md_id, or update the above call sites to treat IDs as plain strings (avoid .toHexString/.getTimestamp/.equals/instanceof; use string-safe comparisons or explicit parsing of timestamp hex when needed).
🧹 Nitpick comments (12)
src/test/unit_tests/db/test_schema_keywords.js (2)
76-82: Add explicit assertions on generator format.
Small reinforcement that mongo_utils.mongoObjectId() yields exactly 24 lower-hex.mocha.it('Test keyword objectid', async function() { const validator = ajv.getSchema('test_schema_keywords#/methods/params'); - const should_pass = { key3: mongo_utils.mongoObjectId() }; + const should_pass = { key3: mongo_utils.mongoObjectId() }; assert.strictEqual(validator(should_pass), true); + assert.ok(/^[0-9a-f]{24}$/.test(should_pass.key3), 'generated id must be 24 lower-hex'); const should_fail = { key3: 'not_an_objectid' }; assert.strictEqual(validator(should_fail), false); });
113-118: Fix minor typos in test descriptions.
“then” → “than”.- //Testing a buffer larger then the stated size of buffer + //Testing a buffer larger than the stated size of buffer const should_fail1 = { key5: Buffer.from('larger') }; assert.strictEqual(validator(should_fail1), false); - //Testing a buffer smaller then the stated size of buffer + //Testing a buffer smaller than the stated size of buffer const should_fail2 = { key5: Buffer.from('tiny') };src/server/analytic_services/activity_log_store.js (1)
27-29: Validate provided id_str to fail fast on bad IDs.
If callers supply an _id, validate it early for clearer errors.make_activity_log_id(id_str) { - return String(id_str || mongo_utils.mongoObjectId()); + if (id_str != null) { + const s = String(id_str).toLowerCase(); + if (!mongo_utils.is_object_id(s)) throw new Error('invalid activity_log _id (expected 24-hex string)'); + return s; + } + return mongo_utils.mongoObjectId(); }src/server/notifications/alerts_log_store.js (1)
96-102: Normalize ID strings to lowercase for $in / $lt / $gtMixed-case 24-hex IDs break lexicographic vs numeric ordering — normalize with .toLowerCase(). Verified by script: mixedcase_equal: False, mixedcase_lowered_equal: True (example mismatch: 00D8674A365B0E3597B6F9C7 vs 00c0c6b97429a6c5b6274806).
File: src/server/notifications/alerts_log_store.js (lines 96-102)
- const obj_ids = ids.map(id => String(id)); + const obj_ids = ids.map(id => String(id).toLowerCase()); ... - _id = { $lt: String(till) }; + _id = { $lt: String(till).toLowerCase() }; ... - _id = { $gt: String(since) }; + _id = { $gt: String(since).toLowerCase() };src/util/db_client.js (1)
38-40: Harden parse_object_id to avoid empty/invalid stringsGuard against
''slipping through; optionally validate 24-hex before returning.- new_object_id() { return mongo_utils.mongoObjectId(); } - parse_object_id(id_str) { return String(id_str || mongo_utils.mongoObjectId()); } + new_object_id() { return mongo_utils.mongoObjectId(); } + parse_object_id(id_str) { + const s = (id_str != null) ? String(id_str) : ''; + return s ? s : mongo_utils.mongoObjectId(); + } + // Optionally (if available): + // parse_object_id(id_str) { + // const s = (id_str != null) ? String(id_str) : ''; + // return mongo_utils.is_object_id && !mongo_utils.is_object_id(s) ? mongo_utils.mongoObjectId() : (s || mongo_utils.mongoObjectId()); + // }src/sdk/nb.d.ts (2)
49-51: API change: ID/DBBuffer types simplified
ID→stringandDBBuffer→Bufferis a breaking type change for SDK consumers.
- Add a CHANGELOG entry and bump semver appropriately.
- Confirm no published consumers rely on
mongodb.ObjectIDin their typings.Optionally strengthen typing with a branded hex type to preserve intent:
- type ID = string; + type ObjectIdHex = string & { __brand: 'ObjectIdHex' }; + type ID = ObjectIdHex;
722-724: new_object_id/parse_object_id return types updated — consider brandingReturning plain
stringloses the "24-hex" contract. Consider branded type to help downstream validation.- new_object_id(): string; - parse_object_id(id_str: string): string; + new_object_id(): ObjectIdHex; + parse_object_id(id_str: string): ObjectIdHex;src/test/unit_tests/internal/test_mapper.js (2)
656-666: _id_str is redundant when IDs are already stringsmongo_utils.mongoObjectId() returns a string; storing both _id and _id_str adds no value in tests.
Apply this diff to simplify:
- const _id = mongo_utils.mongoObjectId(); - const _id_str = _id.toString(); + const _id = mongo_utils.mongoObjectId();
397-418: Unnecessary String() wrappersSince _id is already a string, String(first_pools[0]._id) and similar conversions are redundant.
Example:
- pool_id: String(first_pools[0]._id) + pool_id: first_pools[0]._idAlso applies to: 505-519
src/util/postgres_client.js (2)
102-104: Encode guard is fine; consider explicit error on invalid objectid valuesCurrently non-hex values pass through silently. If schema relies on 24-hex, you may want to throw early.
Example:
- if (schema.objectid === true && typeof val === 'string' && (/^[0-9a-f]{24}$/i).test(val)) { - return val.toString(); - } + if (schema.objectid === true) { + if (typeof val === 'string' && (/^[0-9a-f]{24}$/i).test(val)) return val; + throw new Error(`Invalid objectid value: ${util.inspect(val)}`); + }
1221-1224: Avoid accidental “null” string in groupBy _idString(null) => "null". Use a nullish check to fall back.
- new_row._id = String(row[key]) || mongo_utils.mongoObjectId(); + new_row._id = row[key] != null ? String(row[key]) : mongo_utils.mongoObjectId();src/server/object_services/md_store.js (1)
1647-1651: Optional: make_md_id_from_time callers pass a truthy string 'zero_suffix'This works because the callee checks truthiness. Consider standardizing the parameter to boolean at call sites for clarity.
Also applies to: 1964-1966
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/agent/block_store_speed.js(2 hunks)src/sdk/nb.d.ts(3 hunks)src/server/analytic_services/activity_log_store.js(2 hunks)src/server/analytic_services/history_data_store.js(2 hunks)src/server/node_services/nodes_store.js(2 hunks)src/server/notifications/alerts_log_store.js(3 hunks)src/server/object_services/md_store.js(3 hunks)src/server/system_services/config_file_store.js(2 hunks)src/server/system_services/replication_store.js(2 hunks)src/test/integration_tests/api/s3/test_lifecycle.js(5 hunks)src/test/integration_tests/internal/test_agent_blocks_reclaimer.js(5 hunks)src/test/integration_tests/internal/test_agent_blocks_verifier.js(9 hunks)src/test/integration_tests/internal/test_map_reader.js(2 hunks)src/test/unit_tests/db/test_schema_keywords.js(2 hunks)src/test/unit_tests/internal/test_mapper.js(3 hunks)src/tools/md_blow_lifecycle.js(2 hunks)src/util/db_client.js(2 hunks)src/util/mongo_utils.js(2 hunks)src/util/postgres_client.js(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/unit_tests/db/test_schema_keywords.jssrc/test/integration_tests/api/s3/test_lifecycle.jssrc/test/integration_tests/internal/test_agent_blocks_reclaimer.jssrc/test/integration_tests/internal/test_map_reader.jssrc/test/integration_tests/internal/test_agent_blocks_verifier.jssrc/test/unit_tests/internal/test_mapper.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.318Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/server/analytic_services/activity_log_store.jssrc/server/analytic_services/history_data_store.jssrc/server/system_services/config_file_store.jssrc/test/integration_tests/internal/test_agent_blocks_reclaimer.jssrc/util/db_client.js
📚 Learning: 2025-08-08T13:10:36.141Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.141Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/server/system_services/config_file_store.js
🧬 Code graph analysis (13)
src/server/system_services/replication_store.js (1)
src/util/db_client.js (1)
mongo_utils(9-9)
src/util/mongo_utils.js (1)
src/test/integration_tests/internal/test_agent_blocks_reclaimer.js (1)
id(140-140)
src/server/notifications/alerts_log_store.js (9)
src/agent/block_store_speed.js (3)
mongo_utils(7-7)require(13-13)require(14-14)src/server/analytic_services/activity_log_store.js (1)
mongo_utils(4-4)src/server/analytic_services/history_data_store.js (1)
mongo_utils(4-4)src/server/node_services/nodes_store.js (1)
mongo_utils(5-5)src/server/object_services/md_store.js (1)
mongo_utils(10-10)src/server/system_services/config_file_store.js (1)
mongo_utils(7-7)src/server/system_services/replication_store.js (1)
mongo_utils(5-5)src/util/db_client.js (2)
mongo_utils(9-9)require(5-5)src/util/postgres_client.js (4)
mongo_utils(22-22)require(13-13)require(14-14)require(27-27)
src/test/integration_tests/api/s3/test_lifecycle.js (3)
src/server/object_services/md_store.js (1)
mongo_utils(10-10)src/util/db_client.js (2)
mongo_utils(9-9)require(5-5)src/server/object_services/object_server.js (13)
require(17-17)require(22-22)require(32-32)require(33-33)require(34-34)require(35-35)require(38-38)id(1697-1697)obj_id(47-47)obj_id(64-64)obj_id(1572-1572)obj_id(1607-1607)obj_id(1621-1621)
src/server/analytic_services/history_data_store.js (4)
src/agent/block_store_speed.js (3)
mongo_utils(7-7)require(13-13)require(14-14)src/server/analytic_services/activity_log_store.js (1)
mongo_utils(4-4)src/util/db_client.js (2)
mongo_utils(9-9)require(5-5)src/util/postgres_client.js (4)
mongo_utils(22-22)require(13-13)require(14-14)require(27-27)
src/server/system_services/config_file_store.js (5)
src/agent/block_store_speed.js (3)
mongo_utils(7-7)require(13-13)require(14-14)src/server/analytic_services/activity_log_store.js (1)
mongo_utils(4-4)src/server/node_services/nodes_store.js (1)
mongo_utils(5-5)src/server/system_services/replication_store.js (1)
mongo_utils(5-5)src/util/db_client.js (2)
mongo_utils(9-9)require(5-5)
src/test/integration_tests/internal/test_agent_blocks_reclaimer.js (2)
src/test/integration_tests/internal/test_agent_blocks_verifier.js (5)
mongo_utils(16-16)require(18-18)blocks(139-139)blocks(161-161)blocks(185-185)src/util/db_client.js (2)
mongo_utils(9-9)require(5-5)
src/test/integration_tests/internal/test_map_reader.js (3)
src/test/integration_tests/internal/test_agent_blocks_reclaimer.js (1)
mongo_utils(12-12)src/test/integration_tests/internal/test_agent_blocks_verifier.js (2)
mongo_utils(16-16)require(18-18)src/util/db_client.js (2)
mongo_utils(9-9)require(5-5)
src/test/integration_tests/internal/test_agent_blocks_verifier.js (2)
src/test/integration_tests/internal/test_agent_blocks_reclaimer.js (10)
mongo_utils(12-12)nodes(248-253)nodes(273-277)nodes(298-302)nodes(323-327)blocks(143-143)blocks(254-258)blocks(278-283)blocks(303-308)blocks(328-337)src/util/db_client.js (2)
mongo_utils(9-9)require(5-5)
src/util/postgres_client.js (1)
src/util/db_client.js (1)
mongo_utils(9-9)
src/util/db_client.js (1)
src/util/postgres_client.js (4)
mongo_utils(22-22)require(13-13)require(14-14)require(27-27)
src/server/object_services/md_store.js (2)
src/util/db_client.js (1)
mongo_utils(9-9)src/server/object_services/object_server.js (7)
require(17-17)require(22-22)require(32-32)require(33-33)require(34-34)require(35-35)require(38-38)
src/tools/md_blow_lifecycle.js (3)
src/test/integration_tests/api/s3/test_lifecycle.js (2)
obj_id(304-304)MDStore(21-21)src/server/object_services/object_server.js (5)
obj_id(47-47)obj_id(64-64)obj_id(1572-1572)obj_id(1607-1607)obj_id(1621-1621)src/tools/md_blow.js (1)
complete_params(73-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (33)
src/server/system_services/replication_store.js (2)
5-5: Import switch to centralized ID util looks good.
Aligns this store with repo-wide ID generation.
28-33: parse_object_id & replication schema — verify string/ObjectId compatibility
- Findings: src/util/db_client.js:parse_object_id returns String(id_str || mongo_utils.mongoObjectId()) (line ~39); src/util/postgres_client.js:parse_object_id returns String(...) (lines ~1879-1881); src/util/mongo_client.js:parse_object_id(new mongodb.ObjectId(...)) and throws if no id (lines ~308-311); schema: src/server/system_services/schemas/replication_configuration_schema.js uses objectid: true for _id and destination_bucket (lines ~6-24).
- Action: Confirm the schema's objectid validator accepts 24‑hex string IDs—or update the schema to accept strings. Ensure callers either accept string IDs (db_client/postgres) or convert to mongodb.ObjectId before validation when using the Mongo client.
src/util/mongo_utils.js (2)
161-163: Regex-based objectid check is correct and case-insensitive.
Matches exactly 24 hex chars; aligned with tests.
165-172: Guarantee 24-hex length and stronger uniqueness in mongoObjectId().
Pad the timestamp to 8 hex chars and use crypto.randomBytes(8) instead of Math.random to reduce collision risk and avoid clock-related length drift.
File: src/util/mongo_utils.js Lines: 165-172Apply:
+const crypto = require('crypto'); @@ function mongoObjectId() { - // eslint-disable-next-line no-bitwise - const timestamp = (new Date().getTime() / 1000 | 0).toString(16); - return timestamp + 'xxxxxxxxxxxxxxxx'.replace(/[x]/g, function() { - // eslint-disable-next-line no-bitwise - return (Math.random() * 16 | 0).toString(16); - }).toLowerCase(); + const timeHex = Math.floor(Date.now() / 1000).toString(16).padStart(8, '0'); + const randHex = crypto.randomBytes(8).toString('hex'); // 16 hex chars + return (timeHex + randHex); }src/test/unit_tests/db/test_schema_keywords.js (1)
8-8: Use of mongo_utils in tests is aligned with the migration.src/test/integration_tests/internal/test_map_reader.js (3)
12-12: Import switch to mongo_utils is correct.
49-54: Same here for admin path; looks consistent.
42-47: String _id OK — map_reader has no ObjectId-specific assumptions.map_reader.read_object_mapping / read_object_mapping_admin pass obj._id through to MDStore and do not call .toHexString(); mongo_utils.mongoObjectId() returns a hex string, so the test's string _id is fine.
Locations: src/server/object_services/map_reader.js (read_object_mapping / read_object_mapping_admin), src/util/mongo_utils.js (mongoObjectId).
src/server/system_services/config_file_store.js (2)
7-7: Centralized ID util import looks good.
29-30: Default string _id on insert: OK — schema already allows ObjectId. src/server/system_services/schemas/config_file_schema.js declares _id withobjectid: true(required), so 24‑hex string IDs are permitted and there’s no upsert behavior change.src/server/analytic_services/history_data_store.js (2)
4-4: Importing mongo_utils for ID creation: good.
33-37: String _id generation in history records: good.src/agent/block_store_speed.js (2)
7-7: Import switch to mongo_utils is correct.
64-71: Confirm server accepts string block_id (no ObjectId/toHexString assumptions).
Sandbox couldn't access server sources to verify; run these locally and confirm the server's write_block handler treats block_md.id as a string.# run from repo root rg -n --hidden --no-ignore -S 'write_block' -C3 || true rg -n --hidden --no-ignore -S 'block_md' -C3 || true rg -n --hidden --no-ignore -S '\.toHexString\(' -C3 || true rg -n --hidden --no-ignore -S 'ObjectId\(' -C3 || true rg -n --hidden --no-ignore -S 'new ObjectId' -C3 || truesrc/agent/block_store_speed.js (lines 64–71).
src/server/analytic_services/activity_log_store.js (1)
4-4: Switch to mongo_utils import is consistent with the migration.src/server/node_services/nodes_store.js (1)
5-5: Switch to mongo_utils import — OKGood step toward centralizing ID handling.
src/server/notifications/alerts_log_store.js (2)
4-4: mongo_utils import — OKAligns with the PR objective to remove direct MongoDB ObjectId usage.
25-25: make_alert_log_id fallback — OK
String(id_str || mongo_utils.mongoObjectId())safely avoids the"undefined"pitfall seen elsewhere.src/test/integration_tests/internal/test_agent_blocks_reclaimer.js (2)
12-12: Tests now use mongo_utils for IDs — OKKeeps tests aligned with the new ID strategy.
249-256: Fixture IDs migrated to mongo_utils.mongoObjectId — OKAll mock node/block IDs are now generated via the helper. Nice consistency.
Also applies to: 274-281, 299-307, 324-337, 329-345
src/test/unit_tests/internal/test_mapper.js (1)
34-41: Good migration to string IDs via mongo_utilsReplacing ObjectId constructors with mongo_utils.mongoObjectId() in test fixtures is consistent with the PR’s direction and should keep mapper tests aligned.
src/util/postgres_client.js (2)
1662-1663: LGTM: centralized ID generation returns stringsgenerate_id() now delegates to mongo_utils; consistent with schema and table definition (_id char(24)).
1832-1840: LGTM: object-id resolution now uses 24-hex string checksThe recursion correctly replaces references using string IDs and idmap lookups.
src/tools/md_blow_lifecycle.js (2)
124-131: LGTM: push string IDs for version agingStoring obj_id as strings matches MDStore expectations post-migration.
200-203: LGTM: multipart aging uses create_time and string multipart_idUsing update_multipart_by_id(String(...), { create_time }) is the right approach now that ObjectId timestamps aren’t available.
src/test/integration_tests/api/s3/test_lifecycle.js (3)
156-161: LGTM: update_object_by_id with string idConverting obj_id to string before MDStore update aligns with new ID format.
611-614: LGTM: version aging uses string IDsSwitching obj_upload_ids to string aligns with MDStore.update_objects_by_ids().
388-404: Do not switch the test to use multipart.create_time — lifecycle uses object.upload_started (ObjectId timestamp)md_store.js computes age from data->>'upload_started' via convert_mongoid_to_timestamp_sql, so updating create_time on multiparts won’t trigger the existing lifecycle logic; set upload_started to a valid ObjectId with the intended timestamp instead. (See src/server/object_services/md_store.js around the EXTRACT/convert_mongoid_to_timestamp_sql query.)
Likely an incorrect or invalid review comment.
src/test/integration_tests/internal/test_agent_blocks_verifier.js (2)
16-16: LGTM: centralized ID helper importedUsing mongo_utils is consistent across tests.
124-134: LGTM: test scaffolding generates string IDsAll fixtures (tiers, chunks, frags, nodes, pools) now use mongo_utils.mongoObjectId(); consistent with the new contract.
Also applies to: 153-158, 177-186, 204-214, 221-225, 233-249, 259-279, 289-295
src/server/object_services/md_store.js (3)
10-10: LGTM: using centralized mongo_utils importGood switch to the shared helper for ObjectId concerns.
95-103: make_md_id_from_time(): behavior OK, but ensure suffix source is 24-hexUsing mongo_utils.mongoObjectId().slice(8, 24) preserves the 4-byte time prefix + 8-byte suffix pattern. No change requested.
106-107: Validation routed through mongo_utils.is_object_id() — basic checks pass, but confirm semantic parity
- Sanity check: mongo_utils.is_object_id('zzzzzzzzzzzzzzzzzzzzzzzz') -> false; mongo_utils.is_object_id('507f1f77bcf86cd799439011') -> true.
- Repo still contains instanceof mongodb.ObjectId (src/util/mongo_client.js:323) and many .toHexString() usages (md_store.js, mapper.js, map_server.js, etc.), indicating mixed ObjectId vs string handling.
- Action: decide the contract — either make mongo_utils.is_object_id match mongodb.ObjectId.isValid (accept ObjectId instances and 12-byte inputs) or normalize callers to pass a single representation; update implementation or callers accordingly.
aayushchouhan09
force-pushed
the
mongo-object-id-fix
branch
3 times, most recently
from
979a056 to
63dca37
Compare
Signed-off-by: Aayush Chouhan <[email protected]>
aayushchouhan09
force-pushed
the
mongo-object-id-fix
branch
from
63dca37 to
71963d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/test/unit_tests/util_functions_tests/signature_test_suite/awscli/awscli_iwgdisgt.sreq (1)
109-123: Avoid instanceof; resolve by hex string and drop driver typesRely on string IDs to align with the PR intent and avoid TDZs with wrappers.
Apply:
-function resolve_object_ids_recursive(idmap, item) { - _.each(item, (val, key) => { - if (val instanceof mongo_utils.ObjectId) { - if (key !== '_id') { - const obj = idmap[val]; - if (obj) { - item[key] = obj; - } - } - } else if (_.isObject(val) && !_.isString(val)) { - resolve_object_ids_recursive(idmap, val); - } - }); - return item; -} +function resolve_object_ids_recursive(idmap, item) { + _.each(item, (val, key) => { + if (mongo_utils.is_object_id(val)) { + if (key !== '_id') { + const k = typeof val === 'string' ? val : String(val); + const obj = idmap[k]; + if (obj) item[key] = obj; + } + } else if (_.isObject(val) && !_.isString(val)) { + resolve_object_ids_recursive(idmap, val); + } + }); + return item; +}src/util/mongo_client.js (2)
263-275: Fix resolution to work with string IDs and avoid toHexStringUsing instanceof + toHexString on a wrapper is brittle and will break if values are plain strings.
- _.each(item, (val, key) => { - if (val instanceof mongo_utils.ObjectId) { - if (key !== '_id') { - const obj = idmap[val.toHexString()]; - if (obj) { - item[key] = obj; - } - } - } else if (_.isObject(val) && !_.isString(val)) { - this.resolve_object_ids_recursive(idmap, val); - } - }); + _.each(item, (val, key) => { + if (mongo_utils.is_object_id(val)) { + if (key !== '_id') { + const k = typeof val === 'string' ? val : String(val); + const obj = idmap[k]; + if (obj) item[key] = obj; + } + } else if (_.isObject(val) && !_.isString(val)) { + this.resolve_object_ids_recursive(idmap, val); + } + });
301-313: Return/generate string IDs (not wrapper instances)Prevents BSON encoding surprises with the Mongo driver.
- new_object_id() { - return new mongo_utils.ObjectId(); - } + new_object_id() { + return mongo_utils.mongoObjectId(); + } @@ - parse_object_id(id_str) { - if (!id_str) throw new TypeError('parse_object_id: arg required ' + id_str); - return new mongo_utils.ObjectId(id_str); - } + parse_object_id(id_str) { + if (!id_str) throw new TypeError('parse_object_id: arg required ' + id_str); + return mongo_utils.is_object_id(id_str, true); + }src/util/postgres_client.js (1)
1839-1851: Align resolution with string IDs; avoid toHexStringMirror the mongo_client change.
- _.each(item, (val, key) => { - if (val instanceof mongo_utils.ObjectId) { - if (key !== '_id') { - const obj = idmap[val.toHexString()]; - if (obj) { - item[key] = obj; - } - } - } else if (_.isObject(val) && !_.isString(val)) { - this.resolve_object_ids_recursive(idmap, val); - } - }); + _.each(item, (val, key) => { + if (mongo_utils.is_object_id(val)) { + if (key !== '_id') { + const k = typeof val === 'string' ? val : String(val); + const obj = idmap[k]; + if (obj) item[key] = obj; + } + } else if (_.isObject(val) && !_.isString(val)) { + this.resolve_object_ids_recursive(idmap, val); + } + });
🧹 Nitpick comments (7)
src/test/unit_tests/util_functions_tests/signature_test_suite/awscli/awscli_iwgdisgt.sreq (2)
146-148: Return plain string IDs from helpersKeep test fixtures consistent with string IDs.
-function make_object_id(id_str) { - return new mongo_utils.ObjectId(id_str); -} +function make_object_id(id_str) { + return mongo_utils.is_object_id(id_str, true); +}
164-166: Drop driver-based instanceof checksUse centralized validation; removes tight coupling to mongoose/mongodb classes.
-function is_object_id(id) { - return (id instanceof mongo_utils.ObjectId) || - (id instanceof mongoose.Types.ObjectId); -} +function is_object_id(id) { + return mongo_utils.is_object_id(id); +}src/util/mongo_client.js (1)
318-321: Normalize _id to string on fix_id_typeAvoid storing wrapper instances in docs.
- } else if (doc && doc._id) { - doc._id = new mongo_utils.ObjectId(doc._id); - } + } else if (doc && doc._id) { + doc._id = mongo_utils.is_object_id(doc._id, true); + }src/util/mongo_utils.js (1)
161-181: Minor: clarify is_object_id return typeCurrent behavior is fine; just ensure callers expect boolean when generate=false and string when generate=true. Consider a brief JSDoc to avoid misuse.
src/server/object_services/md_store.js (2)
1436-1439: Stop using toHexString; treat IDs as stringsAvoid wrapper-only APIs in hot paths.
- chunk.parts = parts_by_chunk[chunk._id.toHexString()] || []; - chunk.objects = _.uniq(_.compact(_.map(chunk.parts, part => objects_by_id[part.obj.toHexString()]))); + const cid = String(chunk._id); + chunk.parts = parts_by_chunk[cid] || []; + chunk.objects = _.uniq(_.compact(_.map(chunk.parts, part => objects_by_id[String(part.obj)])));
1881-1886: Stop using toHexString when indexing groupBy resultsUse String() for both chunk and frag IDs.
- const blocks_by_frag = _.groupBy(blocks_by_chunk[chunk._id.toHexString()], 'frag'); + const blocks_by_frag = _.groupBy(blocks_by_chunk[String(chunk._id)], 'frag'); for (const frag of chunk.frags) { - const frag_blocks = blocks_by_frag[frag._id.toHexString()] || []; + const frag_blocks = blocks_by_frag[String(frag._id)] || []; frag.blocks = sorter ? frag_blocks.sort(sorter) : frag_blocks; }src/util/postgres_client.js (1)
1893-1894: Normalize _id to string on fix_id_typeAvoid leaking wrappers into JSON serialization.
- } else if (doc && doc._id) { - doc._id = new mongo_utils.ObjectId(doc._id); - } + } else if (doc && doc._id) { + doc._id = mongo_utils.is_object_id(doc._id, true); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/agent/block_store_speed.js(2 hunks)src/sdk/nb.d.ts(3 hunks)src/server/analytic_services/activity_log_store.js(2 hunks)src/server/analytic_services/history_data_store.js(2 hunks)src/server/bg_services/mirror_writer.js(1 hunks)src/server/node_services/nodes_store.js(2 hunks)src/server/notifications/alerts_log_store.js(3 hunks)src/server/object_services/md_store.js(3 hunks)src/server/system_services/config_file_store.js(2 hunks)src/server/system_services/replication_store.js(2 hunks)src/test/integration_tests/api/s3/test_lifecycle.js(5 hunks)src/test/integration_tests/internal/test_agent_blocks_reclaimer.js(5 hunks)src/test/integration_tests/internal/test_agent_blocks_verifier.js(9 hunks)src/test/integration_tests/internal/test_map_reader.js(2 hunks)src/test/unit_tests/db/test_schema_keywords.js(2 hunks)src/test/unit_tests/internal/test_mapper.js(3 hunks)src/test/unit_tests/internal/test_mirror_writer.js(1 hunks)src/test/unit_tests/util_functions_tests/signature_test_suite/awscli/awscli_iwgdisgt.sreq(4 hunks)src/tools/md_blow_lifecycle.js(3 hunks)src/util/db_client.js(2 hunks)src/util/mongo_client.js(4 hunks)src/util/mongo_utils.js(4 hunks)src/util/postgres_client.js(7 hunks)src/util/schema_keywords.js(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/test/unit_tests/internal/test_mirror_writer.js
- src/server/bg_services/mirror_writer.js
- src/util/schema_keywords.js
🚧 Files skipped from review as they are similar to previous changes (15)
- src/tools/md_blow_lifecycle.js
- src/test/unit_tests/db/test_schema_keywords.js
- src/server/system_services/config_file_store.js
- src/test/integration_tests/internal/test_map_reader.js
- src/sdk/nb.d.ts
- src/test/integration_tests/api/s3/test_lifecycle.js
- src/test/integration_tests/internal/test_agent_blocks_verifier.js
- src/server/analytic_services/activity_log_store.js
- src/server/analytic_services/history_data_store.js
- src/test/integration_tests/internal/test_agent_blocks_reclaimer.js
- src/util/db_client.js
- src/server/node_services/nodes_store.js
- src/test/unit_tests/internal/test_mapper.js
- src/server/system_services/replication_store.js
- src/agent/block_store_speed.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/unit_tests/util_functions_tests/signature_test_suite/awscli/awscli_iwgdisgt.sreq
🧠 Learnings (1)
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/server/notifications/alerts_log_store.js
🧬 Code graph analysis (4)
src/util/postgres_client.js (2)
src/util/db_client.js (1)
mongo_utils(5-5)src/util/mongo_client.js (3)
mongo_utils(9-9)require(19-19)_(4-4)
src/server/notifications/alerts_log_store.js (3)
src/util/db_client.js (2)
mongo_utils(5-5)require(6-6)src/util/mongo_client.js (2)
mongo_utils(9-9)require(19-19)src/server/bg_services/mirror_writer.js (4)
require(6-6)require(13-13)require(14-14)require(15-15)
src/util/mongo_client.js (2)
src/util/db_client.js (2)
mongo_utils(5-5)require(6-6)src/server/bg_services/mirror_writer.js (2)
require(6-6)_(5-5)
src/server/object_services/md_store.js (3)
src/util/db_client.js (1)
mongo_utils(5-5)src/util/mongo_client.js (2)
mongo_utils(9-9)require(19-19)src/server/object_services/object_server.js (7)
require(17-17)require(22-22)require(32-32)require(33-33)require(34-34)require(35-35)require(38-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (6)
src/test/unit_tests/util_functions_tests/signature_test_suite/awscli/awscli_iwgdisgt.sreq (1)
1-11: Test asset: confirm this sreq is exercisedThis looks like an HTTP fixture. Please confirm the suite actually consumes it post-change.
src/server/object_services/md_store.js (3)
95-103: LGTM: time-based ID generation keeps Mongo ordering semanticsSuffix from a random ObjectId slice preserves monotonicity.
102-107: LGTM: is_valid_md_id uses new validatorMatches centralized utility.
2115-2117: Fix validated: make_md_id now generates valid IDsAddresses prior critical finding about returning "undefined". Good change.
src/util/postgres_client.js (2)
59-66: Decode preserves stored IDs (no accidental regeneration)This fixes the earlier corruption risk on read.
108-110: Encode objectid to string only when wrapper is usedCorrect and minimal.
| return new mongo_utils.ObjectId(); | ||
| } | ||
|
|
||
| /** | ||
| * @param {string} id_str | ||
| * @returns {nb.ID} | ||
| */ | ||
| parse_object_id(id_str) { | ||
| return new mongodb.ObjectId(String(id_str || undefined)); | ||
| return new mongo_utils.ObjectId(String(id_str || undefined)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize ID generation/parsing to strings
Keep the Postgres path purely string-based.
- new_object_id() {
- return new mongo_utils.ObjectId();
- }
+ new_object_id() {
+ return mongo_utils.mongoObjectId();
+ }
@@
- parse_object_id(id_str) {
- return new mongo_utils.ObjectId(String(id_str || undefined));
- }
+ parse_object_id(id_str) {
+ return mongo_utils.is_object_id(String(id_str || undefined), true);
+ }📝 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.
| return new mongo_utils.ObjectId(); | |
| } | |
| /** | |
| * @param {string} id_str | |
| * @returns {nb.ID} | |
| */ | |
| parse_object_id(id_str) { | |
| return new mongodb.ObjectId(String(id_str || undefined)); | |
| return new mongo_utils.ObjectId(String(id_str || undefined)); | |
| } | |
| new_object_id() { | |
| return mongo_utils.mongoObjectId(); | |
| } | |
| /** | |
| * @param {string} id_str | |
| * @returns {nb.ID} | |
| */ | |
| parse_object_id(id_str) { | |
| return mongo_utils.is_object_id(String(id_str || undefined), true); | |
| } |
🤖 Prompt for AI Agents
In src/util/postgres_client.js around lines 1878 to 1887, the ID
generation/parsing currently returns mongo ObjectId instances; make both
functions string-based for the Postgres path: have the ID generator return the
ObjectId as a string (e.g. call toString() on the new mongo_utils.ObjectId())
and have parse_object_id return a plain string (e.g. return String(id_str ||
'')) so the Postgres code works with strings only.
71963d3 to
3f29441
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/util/mongo_utils.js (2)
8-8: Remove dead commented importDrop the leftover
mongodbimport comment to reduce noise.-// const mongodb = require('mongodb');
257-290: Align valueOf() with mongodb.ObjectId semanticsReturn the hex string from
valueOf()to match driver behavior and avoid surprising coercions.valueOf() { - return this; + return this._id; }src/util/postgres_client.js (4)
1229-1230: Avoid constructing ObjectId in groupBy results; keep _id as stringGroup results don’t need the wrapper and may throw if
_idisn’t a valid 24-hex string. Prefer strings here.- new_row._id = new mongo_utils.ObjectId(row[key]); + new_row._id = String(row[key]);
1839-1846: Also handle string refs in resolve_object_ids_recursiveCovers cases where refs are already strings, improving robustness.
- if (val instanceof mongo_utils.ObjectId) { + if (val instanceof mongo_utils.ObjectId) { if (key !== '_id') { const obj = idmap[val.toHexString()]; if (obj) { item[key] = obj; } } + } else if (typeof val === 'string' && mongo_utils.is_object_id(val)) { + if (key !== '_id') { + const obj = idmap[val]; + if (obj) { + item[key] = obj; + } + } } else if (_.isObject(val) && !_.isString(val)) { this.resolve_object_ids_recursive(idmap, val); }
1878-1879: Prefer string IDs for PostgresClient.new_object_id()Match
generate_id()and storage type by returning a string.- return new mongo_utils.ObjectId(); + return mongo_utils.mongoObjectId();
1893-1894: Avoid double-wrapping ObjectIds in fix_id_typeSkip wrapping when already an instance.
- doc._id = new mongo_utils.ObjectId(doc._id); + doc._id = (doc._id instanceof mongo_utils.ObjectId) ? doc._id : new mongo_utils.ObjectId(doc._id);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sdk/nb.d.ts(3 hunks)src/util/mongo_utils.js(4 hunks)src/util/postgres_client.js(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sdk/nb.d.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/util/mongo_utils.js (1)
src/test/integration_tests/internal/test_agent_blocks_reclaimer.js (1)
id(140-140)
src/util/postgres_client.js (2)
src/util/db_client.js (1)
mongo_utils(5-5)src/util/mongo_client.js (3)
mongo_utils(9-9)require(19-19)_(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (7)
src/util/postgres_client.js (6)
22-23: LGTM: centralized ID utilities importImporting
mongo_utilshere matches the migration goal.
59-66: Preserve nulls and avoid accidental generation/throws in decode_json(objectid)This fixes the prior issue of generating new IDs (good) , but decoding
null/undefinedwill currently throw. Return the value as-is when nullish.- if (schema.objectid === true) { - if (typeof val === 'string') { + if (schema.objectid === true) { + if (val === undefined || val === null) { + return val; + } else if (typeof val === 'string') { return new mongo_utils.ObjectId(val); } else if (val instanceof mongo_utils.ObjectId) { return val; } else { return new mongo_utils.ObjectId(String(val)); }
108-110: LGTM: encode ObjectId wrapper to stringEncoding only
instanceof mongo_utils.ObjectIdto string keeps plain strings unchanged.
1668-1669: Prefer string IDs for Postgres path in generate_id()Keep the Postgres path purely string-based to reduce wrapper churn and align with storage type.
- return new mongo_utils.ObjectId(); + return mongo_utils.mongoObjectId();
1886-1887: LGTM: parse_object_id returns normalized stringReturning a validated 24-hex string is consistent with string-based storage.
1899-1900: LGTM: flexible ObjectId detectionCovers both wrapper instances and plain 24-hex strings.
src/util/mongo_utils.js (1)
311-311: Verify no leftover usages of mongodb.ObjectId / toHexStringexports.ObjectId = ObjectID is still present at src/util/mongo_utils.js:311. ripgrep in the verification run reported "No files were searched", so absence elsewhere wasn't confirmed — run a repo-wide search for mongodb.ObjectId, ObjectID, ObjectId(...), and .toHexString(...) and migrate any remaining call sites to mongo_utils.ObjectId or use string IDs.
aayushchouhan09
force-pushed
the
mongo-object-id-fix
branch
from
3f29441 to
a901e11
Compare
Signed-off-by: Aayush Chouhan <[email protected]>
aayushchouhan09
force-pushed
the
mongo-object-id-fix
branch
from
a901e11 to
6da37c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/util/mongo_utils.js (1)
287-289: ObjectID.isValid should not accept non-strings indirectlyOnce is_object_id stops accepting integers, this is fine. If you keep broader inputs, guard to strings here to avoid surprises.
static isValid(id) { - return is_object_id(id); + return typeof id === 'string' && is_object_id(id); }src/util/postgres_client.js (2)
59-65: Handle null/undefined gracefully in decode_json for objectidDecoding null/undefined should pass through instead of throwing when coercing to String(val).
if (schema.objectid === true) { - if (typeof val === 'string') { + if (val === null || val === undefined) { + return val; + } else if (typeof val === 'string') { return new mongo_utils.ObjectId(val); } else if (val instanceof mongo_utils.ObjectId) { return val; } else { return new mongo_utils.ObjectId(String(val)); } }
1668-1669: Optional: return string IDs from generate_id in the Postgres pathMost call sites stringify anyway. Returning a string avoids accidental object leakage into layers expecting strings.
generate_id() { - return new mongo_utils.ObjectId(); + return mongo_utils.mongoObjectId(); }If you adopt this, verify call sites either store strings or still call String() defensively.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sdk/nb.d.ts(3 hunks)src/util/mongo_utils.js(4 hunks)src/util/postgres_client.js(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sdk/nb.d.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (9)
src/util/mongo_utils.js (3)
8-8: Commented-out mongodb import can stay for nowNo action needed if intentional for reference. Consider removing later if fully deprecating mongodb in this module.
311-313: API export looks goodExporting ObjectId and mongoObjectId from a single place improves consistency.
161-186: Do not accept integers as ObjectIds; tighten validation and fix error messageAccepting numbers as valid ObjectIds will misclassify numeric fields and can break ref resolution. Also, the error message mentions 12‑byte strings which this implementation doesn’t support.
Apply this diff:
function is_object_id(id, generate = false) { - const ERROR_MSG = 'Argument passed must be a single string of 12 bytes or a string of 24 hex characters'; + const ERROR_MSG = 'Argument passed must be a string of 24 hex characters'; if (id === null || id === undefined) { return generate ? mongoObjectId() : false; } - if (typeof id === 'number' && Number.isInteger(id) && id > 0) { - if (!generate) return true; - return id.toString(16).padStart(8, '0') + mongoObjectId().substring(8); - } - let hex_string = null; if (typeof id === 'string') { hex_string = id; } else if (id._id && typeof id._id === 'string') { hex_string = id._id; } if (hex_string && (/^[0-9a-f]{24}$/i).test(hex_string)) { return generate ? hex_string.toLowerCase() : true; } if (generate) throw new Error(ERROR_MSG); return false; }src/util/postgres_client.js (6)
22-23: Good: centralizing on mongo_utilsSwitching to mongo_utils keeps ID handling consistent across drivers.
108-110: Encode to string when given our ObjectIdMatches the storage format (char(24)) and keeps the Postgres path string-based.
1229-1230: groupBy: reconstructing _id as ObjectId is fineMaintains in-memory consistency after reads.
1839-1846: instanceof check updated to mongo_utils.ObjectIdCorrect and consistent with the new ID type.
1878-1887: new/parse/fix_id_type: consistent use of mongo_utils.ObjectIdThese helpers now align with the centralized ID utilities.
1899-1900: is_object_id wrapper is fine (post mongo_utils fix)After tightening mongo_utils.is_object_id, this provides a reliable check.
Ensure you apply the mongo_utils validation fix to avoid numbers being treated as valid IDs along this path.
| // const _ = require('lodash'); | ||
| // const util = require('util'); | ||
| const mongodb = require('mongodb'); | ||
| // const mongodb = require('mongodb'); |
There was a problem hiding this comment.
I think we can remove it later at the time of cleaning this file as there are many other functions also present which are commented as well.
| return doc; | ||
| } | ||
|
|
||
| is_object_id(id) { |
There was a problem hiding this comment.
did you get a chance to test this flow in upgrade case,
Just wanted to confirm id changes works also with previously created mongo ids
There was a problem hiding this comment.
Sure, I will test the changes for upgrade.
|
|
||
| function is_object_id(id) { | ||
| return (id instanceof mongodb.ObjectId); | ||
| function is_object_id(id, generate = false) { |
There was a problem hiding this comment.
Why need this change?
Are these validations aligned as a MongoDB object id?
There was a problem hiding this comment.
Yes we are trying to make it work like mongodb objectid()
Signed-off-by: Aayush Chouhan <[email protected]>
aayushchouhan09
force-pushed
the
mongo-object-id-fix
branch
from
544a921 to
dd0bb4c
Compare
|
Issue (blocker): Unit tests with DB_TYPE=mongodb fail because our custom ObjectID class doesn't work the same way as MongoDB's real ObjectId. Our fake ObjectID behaves differently when converting to strings, saving to database, loading from database, and being used in authentication tokens. This causes problems in many parts of the system like user login, key management, and account creation. Root Cause: Our custom ObjectID class is not a perfect copy of MongoDB's real ObjectId, so unit tests that expect real MongoDB behavior doesn't work with our fake version. |
2 participants
Describe the Problem
Part of mongo reference removal task.
Explain the Changes
mongodb.ObjectId()withmongo_utils.mongoObjectId()--> using class and constructor.Issues: Fixed #xxx / Gap #xxx
mongodb.ObjectId()withmongo_utils.mongoObjectId().Testing Instructions:
Summary by CodeRabbit
Refactor
SDK / Type Declarations
Utilities
Database / Postgres
Tools
Tests