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

updateOne with embedded discriminators in strict mode throws with $push, but not with $set #9977

Closed
david-nader opened this issue Feb 28, 2021 · 3 comments
Assignees
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue
Milestone

Comments

@david-nader
Copy link

Do you want to request a feature or report a bug?
Report a bug

What is the current behavior?

Using updateOne to update an array field inside an embedded array document with discriminators is not consistent.

If the current behavior is a bug, please provide the steps to reproduce.

mongoose.set("strict", "throw");

/**
 * Question
 */
const questionSchema = new Schema(
    {
        question_type: { type: String, enum: ["mcq", "essay"] },
    },
    {
        discriminatorKey: "question_type",
        timestamps: true,
    }
);

/**
 * Quiz
 */
const quizSchema = new Schema(
    {
        quiz_title: String,
        questions: [questionSchema],
    },
    {
        timestamps: true,
    }
);
const Quiz = model("quiz", quizSchema);

/**
 * MCQ Question
 */
const mcqQuestionSchema = new Schema(
    {
        text: String,
        choices: [
            new Schema(
                { choice_text: String, is_correct: Boolean },
                { timestamps: true }
            ),
        ],
    },
    {
        timestamps: true,
    }
);

const McqQuestion = quizSchema
    .path("questions")
    .discriminator("mcq", mcqQuestionSchema);

Create a quiz

const id1 = new mongoose.Types.ObjectId();
const id2 = new mongoose.Types.ObjectId();

const quiz = await Quiz.create({
    quiz_title: "quiz",
    questions: [
        {
            _id: id1,
            question_type: "mcq",
            text: "A or B?",
            choices: [
                { choice_text: "A", is_correct: false },
                { choice_text: "B", is_correct: true },
            ],
        },
        {
            _id: id2,
            question_type: "mcq",
        },
    ],
});

Try to update the second question:

const filter = {
    questions: {
        $elemMatch: {
            _id: id2,
            question_type: "mcq",
        },
    },
};

/**
 *  Using `$set` works, but does not add timestamps to the `choice` subdocument.
 */
await Quiz.updateOne(filter, {
    $set: {
        "questions.$.choices": [{ choice_text: "choice 1", is_correct: false }],
    },
});

/**
 * However, using `$push` throws:
 *
 * StrictModeError: Field `questions.$.choices` is not in schema and strict
 * mode is set to throw.
 */
await Quiz.updateOne(filter, {
    $push: {
        "questions.$.choices": {
            choice_text: "choice 2",
            is_correct: true,
        },
    },
});

/**
 * and this throws too:
 *
 * StrictModeError: Field `questions.$[q].choices` is not in schema and strict
 * mode is set to throw.
 */
await Quiz.updateOne(
    filter,
    {
        $push: {
            "questions.$[q].choices": {
                choice_text: "choice 3",
                is_correct: false,
            },
        },
    },
    {
        arrayFilters: [{ "q._id": id2, "q.question_type": "mcq" }],
    }
);

What is the expected behavior?

  1. I think at least $push should not throw because it's not different from $set

    Maybe the filtered positional operator ($[q]) is different, because mongoose needs to also check the arrayFilters to be sure that the question that will be updated has the correct discriminator value (mcq). (I think that would be a new feature)

  2. $set should update the timestamps (this may be a different issue)

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.

Mongoose: 5.11.18
MongoDB: 3.6.4
Node.js: 14.15.3
@vkarpov15 vkarpov15 added this to the 5.11.19 milestone Mar 1, 2021
@vkarpov15 vkarpov15 added confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. and removed seen labels Mar 3, 2021
@vkarpov15
Copy link
Collaborator

I fixed the $push issue. The arrayFilters issue is proving to be a bit tricky, but I think I narrowed down the issue to getEmbeddedDiscriminatorPath() not taking into account array filters.

@david-nader
Copy link
Author

david-nader commented Mar 6, 2021

Thank you, but there is still an issue with the positional operators $[]/$[q] and arrayFilters.

It seems that mongoose does not take the arrayFilters into account for updates on embedded discriminators.

Consider this:

await Quiz.updateOne(
    {
        questions: {
            $elemMatch: {
                _id: id2,
                // if we don't include `question_type`, mongoose will not
                // allow the update
                question_type: "mcq",
            },
        },
    },
    {
        $push: {
            // but note that here we are updating _all_ the questions, even if
            // the `question_type` is not `mcq`.
            // mongoose does not check this and will send the update although
            // it should not.
            "questions.$[].choices": {
                choice_text: "choice 3",
                is_correct: false,
            },
        },
    }
);

Also the opposite may happen, although the following should be a valid
update, mongoose will not allow it because it does not take arrayFilters into
account:

await Quiz.updateOne(
    {
        questions: {
            $elemMatch: {
                _id: id2,
                // note: not including the `question_type` here
            },
        },
    },
    {
        $push: {
            "questions.$[q].choices": {
                choice_text: "choice 3",
                is_correct: false,
            },
        },
    },
    {
        // instead we include it as part of the arrayFilters
        arrayFilters: [{ "q.question_type": "mcq" }],
    }
);

This is tested using mongoose 5.11.19

@vkarpov15 vkarpov15 reopened this Mar 8, 2021
@vkarpov15 vkarpov15 modified the milestones: 5.11.19, 5.11.20 Mar 8, 2021
@vkarpov15 vkarpov15 added has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue and removed confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. labels Mar 10, 2021
This was referenced Mar 13, 2021
vkarpov15 added a commit that referenced this issue Mar 16, 2021
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. has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue
Projects
None yet
Development

No branches or pull requests

3 participants