Skip to content

Conversation

caleberi
Copy link

@caleberi caleberi commented Aug 22, 2025

Summary

This PR addresses an issue where ObjectId values returned from MongoDB were not being correctly recognised during the validation stage in process-native-record.js. Specifically, the instanceof ObjectId check was returning false, even when the value was an ObjectId. This caused primary keys to remain as raw ObjectId objects, leading to downstream errors such as missing or invalid id attributes in Sails models.

Problem

  • Observed Error (from Sails):

    Warning: Records sent back from a database adapter should always have a valid property
    that corresponds with the primary key attribute (`id`). But in this result set,
    after transforming columnNames back to attribute names for the model `business`,
    there is a record with a missing or invalid `id`.
    
    Record:
    {
      id: new ObjectId('68a719aa320c7d7207c8cda8'),
      createdAt: '2025-08-21T13:05:46.205Z',
    }
    
  • Debug logs revealed:

    pkValue: new ObjectId("68a891f8c760e876123594df")
    Primary key value: 68a891f8c760e876123594df
    Primary key is Object: true
    Primary key is instance of ObjectId: false
    
  • This occurs because different mongodb driver instances (or contexts) can produce ObjectId references that don’t strictly match the one used for instanceof checks.

Fix

  • Replaced strict instanceof ObjectId validation with a safer check that:

    1. Confirms the value is an object.
    2. Uses ObjectId.isValid(pkValue.toString()) to ensure validity.
  • Ensures that all primary keys are consistently converted to string form before being returned to Sails.

Benefits

  • Prevents id attributes from being left as raw ObjectId instances.
  • Eliminates the missing/invalid id warnings raised by Sails.
  • Makes process-native-record more robust to differences in mongodb driver instances.

Example

Before fix (error in logs):

Primary key is instance of ObjectId: false

After fix (correct behavior):

Primary key is instance of ObjectId: true
Primary key value: "68a891f8c760e876123594df"

Related Issue

Closes #502

@sailsbot
Copy link
Collaborator

Thanks for submitting this pull request, @caleberi! We'll look at it ASAP.

In the mean time, here are some ways you can help speed things along:

  • discuss this pull request with other contributors and get their feedback. (Reactions and comments can help us make better decisions, anticipate compatibility problems, and prevent bugs.)
  • ask another JavaScript developer to review the files changed in this pull request. (Peer reviews definitely don't guarantee perfection, but they help catch mistakes and enourage collaborative thinking. Code reviews are so useful that some open source projects require a minimum number of reviews before even considering a merge!)
  • if appropriate, ask your business to sponsor your pull request. (Open source is our passion, and our core maintainers volunteer many of their nights and weekends working on Sails. But you only get so many nights and weekends in life, and stuff gets done a lot faster when you can work on it during normal daylight hours.)
  • make sure you've answered the "why?" (Before we can review and merge a pull request, we feel it is important to fully understand the use case: the human reason these changes are important for you, your team, or your organization.)

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

@caleberi caleberi changed the title Fix ObjectId handling for validation and conversion Fix: Correct Issues With ObjectId Conversion Between Mongodb Data Source And Adapter Aug 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants