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

Regression of embedded discriminators defined using Schema.prototype.discriminator() #14109

Closed
2 tasks done
ZachLeviPixel opened this issue Nov 22, 2023 · 3 comments
Closed
2 tasks done
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@ZachLeviPixel
Copy link
Contributor

ZachLeviPixel commented Nov 22, 2023

Prerequisites

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

Mongoose version

7.6.5, 8.0.1

Node.js version

18.16

MongoDB server version

6.0.5

Typescript version (if applicable)

No response

Description

Regression of #13898.
I remember testing the fix at the time and that it worked (although other issues cropped up).
I went back and tested this on previous versions and I couldn't find one it works on (7.6.4, 7.6.5, 8.0.1) or when checking out commit 3042ac129 specifically.

Steps to Reproduce

'use strict';

const mongoose = require('mongoose');
const { Schema } = mongoose;

const baseNestedDiscriminated = new Schema({
    type: { type: Number, required: true },
}, { discriminatorKey: "type" });

class BaseClass {
    type = 1;

    whoAmI() {
        return "I am baseNestedDiscriminated";
    }
}

baseNestedDiscriminated.loadClass(BaseClass);

const outerDiscriminatedSchema = new Schema({
    type: { type: Number, required: true },
}, { discriminatorKey: "type" });

const containsNestedSchema = new Schema({
    nestedDiscriminatedTypes: { type: [baseNestedDiscriminated], required: true },
})

class NumberTyped extends BaseClass {
    type = 3;

    whoAmI() {
        return "I am NumberTyped";
    }
}

class StringTyped extends BaseClass {
    type = 4;

    whoAmI() {
        return "I am StringTyped";
    }
}

baseNestedDiscriminated.discriminator(1, new Schema({}).loadClass(NumberTyped));
baseNestedDiscriminated.discriminator("3", new Schema({}).loadClass(StringTyped));

// If you uncomment these, the example works as expected
//containsNestedSchema.path("nestedDiscriminatedTypes").discriminator(1, new Schema({}).loadClass(NumberTyped));
//containsNestedSchema.path("nestedDiscriminatedTypes").discriminator("3", new Schema({}).loadClass(StringTyped));

class ContainsNested {
    nestedDiscriminatedTypes = [];
    type = 1;

    whoAmI() {
        return "I am ContainsNested";
    }
}
containsNestedSchema.loadClass(ContainsNested);

outerDiscriminatedSchema.discriminator(1, containsNestedSchema);
const secondModel = mongoose.model("containsNestedSchema", containsNestedSchema);
const containerModel = mongoose.model("container", new Schema({items: [baseNestedDiscriminated]}));

void async function main() 
{
    await mongoose.connect("mongodb://127.0.0.1:27017/example");
    mongoose.connection.db.dropDatabase();

    let instance = await containerModel.create({ items: [{type: 1}] }); // "I am NumberTyped" - Works
    console.log(instance.items[0].whoAmI());

    instance = await containerModel.create({ items: [{type: "3"}] }); // "I am StringTyped" Works
    console.log(instance.items[0].whoAmI());

    instance = await secondModel.create({ type: 1, nestedDiscriminatedTypes: [{type: 1}, {type: "3"}] });
    
    console.log(instance.whoAmI()); // "I am ContainsNested" - Works
    instance.nestedDiscriminatedTypes.forEach(item => console.log(item.whoAmI()));
    // Expected:
    // I am NumberTyped
    // I am StringTyped
    // Actual:
    // I am baseNestedDiscriminated
    // I am baseNestedDiscriminated

    await mongoose.disconnect();
}();

Expected Behavior

Expected Output:

I am NumberTyped
I am StringTyped
I am ContainsNested
I am NumberTyped
I am StringTyped

Actual:
I am NumberTyped
I am StringTyped
I am ContainsNested
I am baseNestedDiscriminated
I am baseNestedDiscriminated
@vkarpov15 vkarpov15 added this to the 7.6.7 milestone Nov 25, 2023
@vkarpov15 vkarpov15 added the has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue label Nov 25, 2023
@IslandRhythms IslandRhythms added confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. and removed has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue labels Nov 27, 2023
@vkarpov15
Copy link
Collaborator

It looks like the issue is that the test we wrote for #13898 doesn't have the outerDiscriminatedSchema, so we don't have the first layer of discriminators in our test suite. That looks to be the cause, because our test for #13898 is passing. We're investigating why this is happening.

@vkarpov15
Copy link
Collaborator

One workaround is to add the discriminators to the schema before using the schema to define a document array. For example, the following prints the correct result:

'use strict';

const mongoose = require('mongoose');
const { Schema } = mongoose;

const baseNestedDiscriminated = new Schema({
    type: { type: Number, required: true },
}, { discriminatorKey: "type" });

class BaseClass {
    type = 1;

    whoAmI() {
        return "I am baseNestedDiscriminated";
    }
}

baseNestedDiscriminated.loadClass(BaseClass);

const outerDiscriminatedSchema = new Schema({
    type: { type: Number, required: true },
}, { discriminatorKey: "type" });

class NumberTyped extends BaseClass {
    type = 3;

    whoAmI() {
        return "I am NumberTyped";
    }
}

class StringTyped extends BaseClass {
    type = 4;

    whoAmI() {
        return "I am StringTyped";
    }
}

baseNestedDiscriminated.discriminator(1, new Schema({}).loadClass(NumberTyped));
baseNestedDiscriminated.discriminator("3", new Schema({}).loadClass(StringTyped));

const containsNestedSchema = new Schema({
    nestedDiscriminatedTypes: { type: [baseNestedDiscriminated], required: true },
})

class ContainsNested {
    nestedDiscriminatedTypes = [];
    type = 1;

    whoAmI() {
        return "I am ContainsNested";
    }
}
containsNestedSchema.loadClass(ContainsNested);

outerDiscriminatedSchema.discriminator(1, containsNestedSchema);
const secondModel = mongoose.model("containsNestedSchema", containsNestedSchema);

void async function main()
{
    await mongoose.connect("mongodb://127.0.0.1:27017/example");
    mongoose.connection.db.dropDatabase();

    let instance = await secondModel.create({ type: 1, nestedDiscriminatedTypes: [{type: 1}, {type: "3"}] });

    console.log(instance.whoAmI()); // "I am ContainsNested" - Works
    instance.nestedDiscriminatedTypes.forEach(item => console.log(item.whoAmI()));
    // I am NumberTyped
    // I am StringTyped

    await mongoose.disconnect();
}();

We will check to see if there's a way we can make the order in which you add baseNestedDiscriminated.discriminator() not matter here; it should be possible by checking for discriminators on the schema when compiling the model.

@ZachLeviPixel
Copy link
Contributor Author

I applied the workaround on my project and I can confirm it now works as expected. I do hope the last fix pans out, but with this workaround and the other recent fixes have greatly simplified our code around the use of discriminators. Many thanks for the prompt replies and fixes!

vkarpov15 added a commit that referenced this issue Nov 29, 2023
…r()` to subdocuments after defining parent schema

Fix #14109
vkarpov15 added a commit that referenced this issue Nov 29, 2023
vkarpov15 added a commit that referenced this issue Dec 1, 2023
fix: allow adding discriminators using `Schema.prototype.discriminator()` to subdocuments after defining parent schema
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Projects
None yet
Development

No branches or pull requests

3 participants