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

Discriminator schema should inherit base schema options #12135

Closed
2 tasks done
chaeron opened this issue Jul 21, 2022 · 7 comments
Closed
2 tasks done

Discriminator schema should inherit base schema options #12135

chaeron opened this issue Jul 21, 2022 · 7 comments
Labels
backwards-breaking enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Milestone

Comments

@chaeron
Copy link

chaeron commented Jul 21, 2022

Prerequisites

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

Mongoose version

6.4.6

Node.js version

16.x

MongoDB server version

6.0

Description

We use discriminators a lot, and have multiple discriminator models built on a base model/schema.

Tried to do a query against the base model, using fields in some of the discriminator schemas, but Mongoose seems to strip out the query fields that are not found in the base model, even though they are in the discriminator models/schemas.

For the Event example found here: https://mongoosejs.com/docs/discriminators.html, this would be doing a query as follows

let events = Event.find( { url: "some value" } );

where url is only specified in the ClickedLinkEvent discriminator schema, not in the base Event schema.

If you do this, you get all events back, since the url field is removed and the query becomes just {}.

OK...maybe this is the expected behaviour, so I tried to set strictQuery: false on the base Event schema. But then Mongoose throws an error saying this is not allowed.

I believe we should be able to specify strictQuery: false on a base schema/discriminator and have it not strip out any fields that are not in the base schema. That would resolve our issue.

The workaround for this was to just get the db connection and do a query using the MongoDB driver directly on the discriminated collection, but we shouldn't have to do this, since a query like this using Mongoose does not return expected results.

Steps to Reproduce

See description

Expected Behavior

No response

@IslandRhythms IslandRhythms added the confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. label Jul 22, 2022
@IslandRhythms
Copy link
Collaborator

It doesn't throw an error, but it doesn't do its job either

const mongoose = require('mongoose');
const assert = require('assert');

const options = { discriminatorKey: 'kind' };

const eventSchema = new mongoose.Schema({ time: Date }, options);
const Event = mongoose.model('Event', eventSchema);

// ClickedLinkEvent is a special type of Event that has
// a URL.
const ClickedLinkEvent = Event.discriminator('ClickedLink',
  new mongoose.Schema({ url: String }, options));


async function run() {
    await mongoose.connect('mongodb://localhost:27017');
    await mongoose.connection.dropDatabase();

    // When you create a generic event, it can't have a URL field...
const genericEvent = await Event.create({ time: Date.now(), url: 'google.com' });
assert.ok(!genericEvent.url);
await Event.create({url: 'test'})

// But a ClickedLinkEvent can
const clickedEvent = await ClickedLinkEvent.create({ time: Date.now(), url: 'google.com' });
await ClickedLinkEvent.create({ url: 'clicked test'})
assert.ok(clickedEvent.url);


    const test = await Event.find({ url: 'google.com'}, {strictQuery: false})
    console.log(test);
}

run();

@chaeron
Copy link
Author

chaeron commented Jul 23, 2022

It doesn't throw an error, because I believe strictQuery is a schema option, not a query one, per the documentation:

https://mongoosejs.com/docs/guide.html#strictQuery

If you specify the schema option on eventSchema in your example, I believe it will throw an error when you try to run.

@vkarpov15 vkarpov15 added this to the 6.4.8 milestone Jul 26, 2022
@vkarpov15
Copy link
Collaborator

@chaeron when you say "OK...maybe this is the expected behaviour, so I tried to set strictQuery: false on the base Event schema. But then Mongoose throws an error saying this is not allowed.", I think the issue you're seeing is that you need to set strictQuery: false on both base schema and discriminator schema. Below script works as expected:

const mongoose = require('mongoose');
const assert = require('assert');

mongoose.set('debug', true);

const options = { discriminatorKey: 'kind', strictQuery: false };

const eventSchema = new mongoose.Schema({ time: Date }, options);
const Event = mongoose.model('Event', eventSchema);

// ClickedLinkEvent is a special type of Event that has
// a URL.
const ClickedLinkEvent = Event.discriminator('ClickedLink',
  new mongoose.Schema({ url: String }, options));


async function run() {
    await mongoose.connect('mongodb://localhost:27017');
    await mongoose.connection.dropDatabase();

    // When you create a generic event, it can't have a URL field...
const genericEvent = await Event.create({ time: Date.now(), url: 'google.com' });
assert.ok(!genericEvent.url);
await Event.create({url: 'test'})

// But a ClickedLinkEvent can
const clickedEvent = await ClickedLinkEvent.create({ time: Date.now(), url: 'google.com' });
await ClickedLinkEvent.create({ url: 'clicked test'})
assert.ok(clickedEvent.url);


    const test = await Event.find({ url: 'google.com'})
    console.log(test);
}

run();

For 7.0, we should consider making discriminator schemas inherit the base schema's options by default, rather than requiring people to explicitly specify the exact same options.

Side note: @IslandRhythms the issue with your script is that you're doing await Event.find({ url: 'google.com'}, {strictQuery: false}), you should do await Event.find({ url: 'google.com'}, null, {strictQuery: false}), or await Event.find({ url: 'google.com'}).setOptions({strictQuery: false}). 2nd arg to find() is projection, not options.

@vkarpov15 vkarpov15 changed the title Query on Base model of discriminator throws away query parameters not in base schema Discriminator schema should inherit base schema options Jul 27, 2022
@vkarpov15 vkarpov15 added enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature backwards-breaking and removed confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. labels Jul 27, 2022
@vkarpov15 vkarpov15 modified the milestones: 6.5.1, 7.0 Jul 27, 2022
@oonsamyi
Copy link

@vkarpov15 What if I already have option strictQuery: 'throw' for all app?

// before mongoose schemas created
mongoose.set('strictQuery', 'throw')

What is workaround in that case? I don't want change behavior for discriminator schemas via strictQuery: false :(

@oonsamyi
Copy link

oonsamyi commented Aug 11, 2022

I found workaround for my case: define all fields that will be searched by on base schema with optional property.

I use nest.js with mongoose and my solution looks like:

@Schema({ discriminatorKey: 'type' })
export class BaseLog {
  @Prop({
    type: String,
    required: true,
    enum: LogType,
    index: true,
  })
  type: LogType
  
  @Prop({ required: true, index: true })
  userId: number

  // field that exist in CreateCabinetLog only and will be searched by: Log.find({ cabinetId })
  @Prop({ sparse: true })
  cabinetId?: number
}

export class CreateCabinetLog {
  type: LogType.createCabinet
  userId: number
  
  // specify field as type, without Prop decorator
  cabinetId: number

  // field that exist in CreateCabinetLog only and will not be searched by
  @Prop({ required: true })
  name: string
}

export const BaseLogSchema = SchemaFactory.createForClass(BaseLog)
export const CreateCabinetLogSchema = SchemaFactory.createForClass(CreateCabinetLog)

@vkarpov15
Copy link
Collaborator

If you did mongoose.set('strictQuery', 'throw'), that works. You don't need to set 'strictQuery' on every schema.

@vkarpov15
Copy link
Collaborator

Fixed by #12928

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-breaking enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Projects
None yet
Development

No branches or pull requests

4 participants