Skip to content

Fix: cast id parameter based on schema _id type in DocumentArray.id() #15725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 10, 2025
Merged

Fix: cast id parameter based on schema _id type in DocumentArray.id() #15725

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c6c662
fix: cast id parameter based on schema _id type in DocumentArray.id()
Lex-Ashu Nov 6, 2025
79adc2c
fix: cast document array ids using schema type
Lex-Ashu Nov 8, 2025
d2aaf37
Update lib/types/documentArray/methods/index.js
vkarpov15 Nov 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 33 additions & 12 deletions lib/types/documentArray/methods/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

const ArrayMethods = require('../../array/methods');
const Document = require('../../../document');
const castObjectId = require('../../../cast/objectid');
const getDiscriminatorByValue = require('../../../helpers/discriminator/getDiscriminatorByValue');
const internalToObjectOptions = require('../../../options').internalToObjectOptions;
const utils = require('../../../utils');
Expand Down Expand Up @@ -121,23 +120,39 @@ const methods = {
*
* @return {EmbeddedDocument|null} the subdocument or null if not found.
* @param {ObjectId|String|Number|Buffer} id
* @TODO cast to the _id based on schema for proper comparison
* @method id
* @api public
* @memberOf MongooseDocumentArray
*/

id(id) {
let casted;
let sid;
let _id;
if (id == null) {
return null;
}

const schemaType = this[arraySchemaSymbol];
let idSchemaType = null;

if (schemaType && schemaType.schema) {
idSchemaType = schemaType.schema.path('_id');
} else if (schemaType && schemaType.casterConstructor && schemaType.casterConstructor.schema) {
idSchemaType = schemaType.casterConstructor.schema.path('_id');
} else if (this.length > 0 && this[0] != null && this[0].$__schema) {
idSchemaType = this[0].$__schema.path('_id');
}

try {
casted = castObjectId(id).toString();
} catch (e) {
casted = null;
let castedId = id;
if (idSchemaType) {
try {
castedId = idSchemaType.cast(id);
} catch (e) {
castedId = null;
}
}

let sid;
let _id;

for (const val of this) {
if (!val) {
continue;
Expand All @@ -152,11 +167,17 @@ const methods = {
if (sid == _id._id) {
return val;
}
} else if (!isBsonType(id, 'ObjectId') && !isBsonType(_id, 'ObjectId')) {
if (id == _id || utils.deepEqual(id, _id)) {
} else if (castedId != null && !isBsonType(castedId, 'ObjectId') && !isBsonType(_id, 'ObjectId')) {
if (castedId == _id || utils.deepEqual(castedId, _id)) {
return val;
}
} else if (casted == _id) {
} else if (castedId != null && isBsonType(castedId, 'ObjectId') && isBsonType(_id, 'ObjectId')) {
if (castedId.toString() === _id.toString()) {
return val;
}
} else if (castedId != null && castedId == _id) {
return val;
} else if (castedId == null && (id == _id || utils.deepEqual(id, _id))) {
return val;
Comment on lines +180 to 181
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The fallback condition else if (castedId == null && (id == _id || utils.deepEqual(id, _id))) could potentially match when casting failed but the original id happens to match. This might give false positives if the intent is that casting failure should prevent matches.

Consider whether this fallback is necessary, or if a failed cast should always result in no match. If the fallback is intentional, a comment explaining why would be helpful.

Suggested change
} else if (castedId == null && (id == _id || utils.deepEqual(id, _id))) {
return val;

Copilot uses AI. Check for mistakes.
}
Comment on lines +170 to 182
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The comparison logic starting at line 170 handles multiple scenarios but lacks explanatory comments. Consider adding brief comments for each condition to improve maintainability:

// Both castedId and _id are non-ObjectId types (e.g., string, number, object)
} else if (castedId != null && !isBsonType(castedId, 'ObjectId') && !isBsonType(_id, 'ObjectId')) {
  if (castedId == _id || utils.deepEqual(castedId, _id)) {
    return val;
  }
// Both castedId and _id are ObjectIds - compare as strings
} else if (castedId != null && isBsonType(castedId, 'ObjectId') && isBsonType(_id, 'ObjectId')) {
  if (castedId.toString() === _id.toString()) {
    return val;
  }
// Mixed types or other valid castedId scenarios
} else if (castedId != null && castedId == _id) {
  return val;
// Fallback to original id when casting failed
} else if (castedId == null && (id == _id || utils.deepEqual(id, _id))) {
  return val;
}

Copilot uses AI. Check for mistakes.
}
Expand Down