Skip to content
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.

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

Document.prototype.toObject() with discriminators returns all fields and selectively applies getters #15218

Open
2 tasks done
lcrosetto opened this issue Feb 1, 2025 · 5 comments
Labels
new feature This change adds new functionality, like a new method or class
Milestone

Comments

@lcrosetto
Copy link

lcrosetto commented Feb 1, 2025

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Mongoose version

8.9.7

Node.js version

20.18.0

MongoDB server version

8.0.4

Typescript version (if applicable)

No response

Description

After updating a discriminator key in a document with schema discriminators and getters, the Document includes the fields for the current discriminator, as expected. However, Document.toObject() returns all fields, including those defined in a different discriminator schema, but only applies getters to the fields matching the current discriminator.

Steps to Reproduce

The following script will fail when checking the discriminator fields. In particular, the Document contains only the fields relevant to the discriminator, but the object returned by Document.toObject() contains all fields, including those for the other discriminator schema.

      const conn = mongoose.createConnection(..);

      const baseType = 'baseType';
      const type1Key = 'Type1';
      const type2Key = 'Type2';

      const baseSchema = new mongoose.Schema({
        field: String,
        key: String,
      }, {discriminatorKey: 'key', toObject: {getters: true}});
      const dSchema1 = new mongoose.Schema({
        field1: {
          type: String,
          get(value) {
            return `${value} transformed`;
          },
        },
      });
      const dSchema2 = new mongoose.Schema({
        field2: {
          type: String,
          get(value) {
            return `${value} transformed`;
          },
        },
      });
      baseSchema.discriminator(type1Key, dSchema1);
      baseSchema.discriminator(type2Key, dSchema2);

      const TestModel = conn.model(baseType, baseSchema);

      const field = 'field val';
      const field1 = 'field1 val';
      const field1Transformed = `${field1} transformed`;
      const cdm = new TestModel({
        key: type1Key,
        field,
        field1,
      });
      let cd = await cdm.save();
      assert.equal(cd.field2, undefined);
      assert.equal(cd.field1, field1Transformed);
      cd = cd.toObject();
      assert.equal(cd.field, field);
      assert.equal(cd.key, type1Key);
      assert.equal(cd.field1, field1Transformed);
      assert.equal(cd.field2, undefined);

      const field2 = 'field2 val';
      const field2Transformed = `${field2} transformed`;
      await TestModel.updateOne(
        { _id: cd._id },
        {
          key: type2Key,
          field2,
        },
        {overwriteDiscriminatorKey: true},
      );
      let cd2 = await TestModel.findById(cd._id);
      assert.equal(cd2.field2, field2Transformed);
      assert.equal(cd2.field1, undefined);
      cd2 = cd2.toObject();
      assert.equal(cd2.field, field);
      assert.equal(cd2.key, type2Key);
      assert.equal(cd2.field2, field2Transformed);
      assert.equal(cd2.field1, undefined);

      conn.deleteModel(baseType);

Expected Behavior

As shown in the script above, since a Document with a discriminator includes only the fields relevant to the currently set discriminator key (prior to calling toObject()), I would expect the object returned by toObject() to also only include those fields defined in that discriminator schema.

If all fields must be returned by toObject() (even those not relevant to the current discriminator schema) I would expect all getters to have been called.

@lcrosetto lcrosetto changed the title Document.prototype.toObject() with discriminators returns all fields but selectively applies getters Document.prototype.toObject() with discriminators returns all fields and selectively applies getters Feb 1, 2025
@vkarpov15 vkarpov15 added this to the 8.10.1 milestone Feb 5, 2025
@vkarpov15 vkarpov15 added the has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue label Feb 5, 2025
@vkarpov15
Copy link
Collaborator

vkarpov15 commented Feb 6, 2025

Unfortunately this is expected behavior, although I admit the fact that cd2.field1 === undefined is an unpleasant gotcha (see #5159, #5111) - you would need to do cd2.get('field1') to see that field1 is not in fact undefined.

The issue is that the following update does not unset field1

      await TestModel.updateOne(
        { _id: cd._id },
        {
          key: type2Key,
          field2,
        },
        {overwriteDiscriminatorKey: true},
      );

You can see the issue more clearly if you add a field that isn't in any schema in this updateOne():

      await TestModel.updateOne(
        { _id: cd._id },
        {
          key: type2Key,
          field2,
          field3: 'taco'
        },
        {overwriteDiscriminatorKey: true, strict: false},
      );

// Later
cd2.field3; // undefined
cd2.get('field3'); // 'taco'
cd2.toObject().field3; // 'taco'

Your best bet would be to explicitly unset field1 in your update. Unfortunately this requires setting strict: false because field1 isn't in the new discriminator type's schema.

      await TestModel.updateOne(
        { _id: cd._id },
        { 
          $set: { key: type2Key, field2 },
          $unset: { field1: 1 }
        },
        {overwriteDiscriminatorKey: true, strict: false},
      );

We will take a closer look at #5159 for our next major release.

@vkarpov15 vkarpov15 added has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue and removed has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue labels Feb 7, 2025
@lcrosetto
Copy link
Author

lcrosetto commented Feb 7, 2025

If I log cd2 after toObject() is called, I see:

    {
      _id: new ObjectId('67a6659d06947589d6bf4454'),
      field: 'field val',
      key: 'Type2',
      field1: 'field1 val',
      __v: 0,
      field2: 'field2 val transformed',
      id: '67a6659d06947589d6bf4454'
    }

Above, field2 has had its getter called, but field1 has not. This seems unexpected. I believe this is because when the getters are applied, it is filtering the paths based on the schema (which is using the discriminator):

mongoose/lib/document.js

Lines 4176 to 4178 in 16d2407

function applyGetters(self, json) {
const schema = self.$__schema;
const paths = Object.keys(schema.paths);

Could similar logic be applied in toObject()? As a proof of concept, this test passes if I do something similar in $__toObjectShallow().

@vkarpov15
Copy link
Collaborator

The fact that field2's setter is called but field1's is not is expected. field1 is not in the schema, so there's no getter to call.

We could add a strict mode analog to toObject() where we can do toObject({ strict: true }) to avoid fields that aren't in the schema, would that help?

@lcrosetto
Copy link
Author

The fact that field2's setter is called but field1's is not is expected. field1 is not in the schema, so there's no getter to call.

We could add a strict mode analog to toObject() where we can do toObject({ strict: true }) to avoid fields that aren't in the schema, would that help?

That would be helpful, yes!

vkarpov15 added a commit that referenced this issue Feb 8, 2025
fix: infer discriminator key if set in `$set` with overwriteDiscriminatorKey
@vkarpov15 vkarpov15 added new feature This change adds new functionality, like a new method or class and removed has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue labels Feb 8, 2025
@vkarpov15 vkarpov15 modified the milestones: 8.10.1, 8.11 Feb 8, 2025
@vkarpov15
Copy link
Collaborator

I would still recommend running a migration to unset the unexpected fields in MongoDB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This change adds new functionality, like a new method or class
Projects
None yet
Development

No branches or pull requests

2 participants