Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.91k stars 3.83k forks source link

Decouple discriminators from Models #6002

Open saiichihashimoto opened 6 years ago

saiichihashimoto commented 6 years ago

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

Feature Request

What is the current behavior?

What is the expected behavior?

Discriminators being a method of Schema inheritance, not Model inheritance. Possibly via es6 class inheritance or through the options.

Discriminators are a schema inheritance mechanism. They enable you to have multiple models with overlapping schemas on top of the same underlying MongoDB collection. documentation

A Schema's job is to define the shape of the documents within that collection. A discriminator's Schema, currently, doesn't fit that role. It describes the shape of the difference. If Schemas are meant to decouple the document shape from the Model, then Discriminators don't have this.

Potential Drawbacks:

I came across this working on mongoose-normalizr, which generates normalizr Entities using mongoose Schemas. This allows you to define the relationships between resources once (in mongoose schemas) and still be able to normalize & denormalize them via normalizr, most commonly in a browser (probably into a redux/flux store). Discriminators currently can't be supported, since they only exist in Models. A workaround would be to pass in which Schemas discriminate which Schemas in a separate mapping. The motivation behind mongoose-normalizr is to reduce repeating the relationship definitions so this doesn't make sense.

Since this is an overhaul of discriminators, perhaps it isn't possible. In that case, I think Discriminators need to be redefined as Model inheritance and Schema inheritance would be put in place. This could have the interesting side effect that Schema inheritance doesn't necessarily have to be in the same underlying MongoDB collection. Two different models could have the same underlying collection, as long as the Schemas inherit each other. But they could also be used entirely separately, albeit having a very similar document shape.

vkarpov15 commented 6 years ago

As you pointed out, discriminators are model-based because they are designed to store documents matching different schemas in the same collection. Client-side mongoose currently doesn't have any notion of collections, so discriminators as implemented wouldn't really fit.

It sounds what you really want is just the ability to inherit schemas and extend existing schemas. Here's a couple methods for extending schemas, the first by creating a schema base class and the second by cloning and then modifying a schema, that you might want to try instead. Both should work fine with discriminators as well, so you should be able to do something like Model.discriminator('child', extendSchema(baseSchema, { parent: String }));

class SpecialBaseSchema extends Schema {
  constructor(obj, options) {
    super(obj, options);
    this.add({ name: String });
  }
}
function extendSchema(schema, obj) {
  const clone = schema.clone();
  clone.add(obj);
}
saiichihashimoto commented 6 years ago

I think my confusion is that there's:

  1. Inheritance with Schemas
  2. Schemas storing into the same collection
  3. 1 doesn't require 2, but 2 requires 1.

Calling Discriminators "a schema inheritance mechanism" is misleading. Discriminators overlap Schemas but it isn't the mechanism for inheritance, it just forces it. Schemas that inherit from each other don't necessarily use Discriminators. The same as a square is a rectangle but a rectangle isn't necessarily a square.

This still leaves browser Schemas in an awkward position:

/* schemas */
import mongoose from 'mongoose';

const FooSchema              = mongoose.Schema({ something: String });
// Anything that will be discriminated server side *must* clone the schema to be usable browser side
const FooDiscriminatorSchema = FooSchema.clone().add({ else: String });
const BarSchema              = FooSchema.clone().add({ another: String });
const FooContainerSchema     = mongoose.Schema({ foos: [{ ref: 'Foo' }] });

export { FooSchema, FooDiscriminatorSchema, BarSchema, FooContainerSchema };
/* server */
import mongoose from 'mongoose';
import { FooSchema, FooDiscriminatorSchema, BarSchema, FooContainerSchema } from './schemas';

const Foo              = mongoose.model('Foo', FooSchema);
// FIRST definition of the Foo/FooDiscriminator relationship
const FooDiscriminator = Foo.discriminator('FooDiscriminator', FooDiscriminatorSchema);
const Bar              = mongoose.model('Bar', BarSchema);
const FooContainer     = mongoose.model('FooContainer', FooContainerSchema);

console.log(Foo.discriminators); // should have FooDiscriminator but not Bar
/* browser */
import mongoose from 'mongoose';
import { FooSchema, FooDiscriminatorSchema, BarSchema, FooContainerSchema } from './schemas';

const normalizeUsingSchemas = (document, schemas) => {
  // Unaware references to 'Foo' could be 'FooDiscriminator'
};

const fooContainer = mongoose.Document({ /* ... */ }, FooContainerSchema);

// fooContainer.foos normalize to Foos but not FooDiscriminators
const normalizedData = normalizeUsingSchemas(
  fooContainer,
  {
    Foo: FooSchema,
    FooDiscriminator: FooDiscriminatorSchema,
    Bar: BarSchema,
    FooContainer: FooContainerSchema,
  }
);

The browser can't detect any relationship between Foo and FooDiscriminator. Any reference to a 'Foo' has no way to determine that that could be a 'FooDiscriminator' there.

One workaround is passing in a mapping:

/* browser-mapping */
import mongoose from 'mongoose';
import { FooSchema, FooDiscriminatorSchema, BarSchema, FooContainerSchema } from './schemas';

const normalizeUsingSchemas = (document, schemas, discriminators) => {
  // discriminators can tell us that FooDiscriminator could be anywhere a Foo is referenced
};

const fooContainer = mongoose.Document({ /* ... */ }, FooContainerSchema);

const normalizedData = normalizeUsingSchemas(
  fooContainer,
  {
    Foo: FooSchema,
    FooDiscriminator: FooDiscriminatorSchema,
    Bar: BarSchema,
    FooContainer: FooContainerSchema,
  },
  {
    // SECOND definition of the Foo/FooDiscriminator relationship
    FooDiscriminator: 'Foo',
  }
);

That required defining that FooDiscriminator discriminates Foo both from model discriminating and a separate mapping. This isn't elegant.

Another option is to look at discriminatorKey, making it required:

/* schemas-discriminatorKey */
import mongoose from 'mongoose';

// Will require defining discriminatorKey, even if it's the default
const FooSchema              = mongoose.Schema({ something: String }, { discriminatorKey: '__t' });
const FooDiscriminatorSchema = FooSchema.clone().add({ else: String });
const BarSchema              = FooSchema.clone().add({ another: String });
const FooContainerSchema     = mongoose.Schema({ foos: [{ ref: 'Foo' }] });

export { FooSchema, FooDiscriminatorSchema, BarSchema, FooContainerSchema };
/* browser-discriminatorKeys */
// ...

const normalizeUsingSchemas = (document, schemas) => {
  // Now any schema with schema.options.discriminatorKey could be *something* else.
};

// ...

Now we have FooDiscriminator's model define it's a discriminator and FooSchema defining it will be discriminated. It still requires defining the relationship on both ends, but it's better.

What would be the suggested course of action? The discriminatorKey solution isn't bad but it isn't really great, either. I'm ok with the answer being that this is an edge case mongoose isn't concerned with handling.

vkarpov15 commented 6 years ago

Right now mongoose has no way for you to handle discriminator relationships elegantly in the browser. Browser-side support for mongoose is extremely minimal and, as you mentioned, has no notion of models. I think this problem could be solved by having models in the browser, so instead of doing new mongoose.Document({}, fooSchema); you would have to do:

const Foo = mongoose.model('Foo', fooSchema);
const doc = new Foo();

That way you could use discriminators, but save() would still fail. Would that fix your case?

saiichihashimoto commented 6 years ago

Interesting... so even though models don't mean anything in the browser, they'd exist for parity. That would solve my use case, but I'd say the decision here is whatever the long term vision for mongoose in the browser is, ie. how you want users to use mongoose in the browser.

If you want it to be completely isomorphic/universal/whatever-its-called-these-days, I think having models in the browser is the right approach. The save/update/etc calls would throw but, otherwise, you could use the same code on the browser or server. Maybe you could provide an "adapter" to mongoose and all the save/update/etc would delegate what it's doing to the adapter (http calls to a server, etc). That would make Models less of a connection-to-mongodb construct, but more of a connection-to-whatever construct, with a default of mongodb in node (rather than webpack/browser/browser-whatnot).

If you don't want to "split" models up like that, I'd stick to having models be in the context of mongodb, so they'd have no place in the browser. Maybe instead of new mongoose.Document({}, FooSchema) in the browser, it would be new FooSchema({})...? So you get the similar feel of new ThingIWantADocumentOf(...) but skipping the model implicitly conveys that models are irrelevant to the browser.

Sorry for all the complaining. :-) I ended up going with my second solution of discriminating schemas extending their base schema + always defining a discriminatorKey.

I really like mongoose and congrats on the 5.0.0 launch!

vkarpov15 commented 6 years ago

I think the vision has always been the former, making mongoose as isomorphic as reasonably possible. The current iteration of that vision is limited for several reasons, mostly because it was already a major struggle to get mongoose to work in IE9 back in 2014 when we got the first browser-compatible version out the door, primarily to support Angular 1 form validation. Naturally, frontend JS has changed a lot since then, and we haven't quite been able to keep up.

We've bounced around the adapter concept quite a bit, but that's a much longer term idea. I think we really need to dedicate some time in Q2 to get models working in the browser and better support for webpack.

Thanks for the kind words, I appreciate it!

saiichihashimoto commented 6 years ago

Sounds good! It seems like, until browser mongoose has been sorted out a bit more, this issue is just going to be noise.

Feel free to close!

vkarpov15 commented 6 years ago

I'll leave this open to track the issue

pwhissell commented 6 years ago

It would be awesome if Schema definitions would be done using es6 classes and if support for inheritance would be transparent to the developper (slash mongoose user). Discriminators are one way of supporting class inheritance in the database but the developper using mongoose shouldn't need to know how the inheritance hierchy is represented in mongodb.

It would be much cooler if the fact of extending another schema would define the background needed discriminators. I know Es6 doesnt offer much reflection that would ease this kind of implementation but I was hoping someone might have an idea as to how this could be done.

Heres an example of how schemas are currently defined

const HeroSchema = new mongoose.Schema({
  "name": {
    "type": String,
    "required": true
  },
  "weapons": {
    "type": [WeaponSchema],
    "required": true
  }
}

HeroSchema.path('weapons').discriminator('sword', sword.schema);
HeroSchema.path('weapons').discriminator('hammer', hammer.schema);
const options = {
  discriminatorKey: 'type'
};

const WeaponSchema = new mongoose.Schema({
  "name": {
    "type": String,
    "required": true
  }
}, options);

const SwordSchema = new mongoose.Schema({
  "sharpness": {
    "type": Number,
    "required": true
  }
});

const HammerSchema = new mongoose.Schema({
  "bluntness": {
    "type": Number,
    "required": true
  }
});

heres how it would be cool for them to be defined:

class HeroSchema extends Schema {
  "name": {
    "type": String,
    "required": true
  },
  "weapons": {
    "type": [WeaponSchema],
    "required": true
  }
}

class WeaponSchema extends Schema {
  "name": {
    "type": String,
    "required": true
  }
};

class SwordSchema extends WeaponSchema {
  "sharpness": {
    "type": Number,
    "required": true
  }
};

class HammerSchema extends WeaponSchema {
  "bluntness": {
    "type": Number,
    "required": true
  }
};

If somehow the Schema class's constructor would use reflection to define the discriminators using the child class's name then inheritance would be transparent to the developper

vkarpov15 commented 6 years ago

@pwhissell but then how would that map to models? How would models know about the classes that extend from a given schema?

pwhissell commented 6 years ago

@vkarpov15 Sorry for taking so long to respond, I realized how what I meant to say has nothing to do with the title of this issue so I created a new one here.

Even though I beleive discriminators are part of the model, what I meant to say is I would love for schemas to be invisible to the mongoose user. This could be implemented by using the Decorator TC39 proposal