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

feat: mongoose extension #5201

Closed
wants to merge 2 commits into from

Conversation

mschnee
Copy link
Contributor

@mschnee mschnee commented Apr 23, 2020

Inspired by #4794 and by a project I am working with that has a large number of existing Typegoose Mongoose schemas/models already defined.

This is a very rough WIP, and more of a weekend project to start a discussion.

Most of the discussion is happening in #5152 , the mongoose extension is essentially an untyped version of it, using mongoose directly

Checklist

👉 [Read and sign the CLA (Contributor License Agreement)]

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

```ts
this.configure(MongooseBindings.COMPONENT).to({
uri: 'mongodb://localhost:27017',
connectionOptions: {} // anything valid to mongoose.createConnection(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might consider to use settings from a datasource.

this.configure(MongooseBindings.COMPONENT).to({
uri: 'mongodb://localhost:27017',
connectionOptions: {} // anything valid to mongoose.createConnection(),
schemas: [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to declare mongoose.schemas as an extension point to register Mongoose schemas as extensions.

name: 'MyModel'
}
],
discriminators: [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each schema/model itself can be an extension point for its discriminators.

},
{
uri: 'mongodb://127.0.0.12:27017',
schemas: [...]
Copy link
Contributor

@raymondfeng raymondfeng Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit similar as how we make AJV configurable and extensible. In https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/validation/ajv-factory.provider.ts, we use @inject (or @config) for options, keywords, and formats. Both keywords and formats can be either passed in via options, or their own extension points.

@config({fromBinding: 'datasources.mongodb'})
private connectionOptions;

@extensions('mongoose.schemas')
private mongooseSchemas: Getter<MongooseSchema[]>;

Copy link
Contributor Author

@mschnee mschnee Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extension system/pattern is going to be new to me: is the linked AVJ code a good example to base off of as a starting point?

The question mark I have is that schemas are orthogonal to connections: a model object represents a schema defined on a specific connection, so schemas can be used across multiple connections. I've seen systems that export all the schemas from an schems/index.js file, and then have a separate connection that only uses a handful of them to create models specific to that connection.

And then there's having both mongoose and typegoose models (two different extensions) use the same mongoose connection. This is actually important to me here because I have an API server that has both mongoose and typegoose models (sharing one mongoose connection so that the references can be properly determined)

My first thought is to change the uri configuration to be

uriOrConnection: string | mongoose.Connection

to allow you to pass an existing connection instead.

Second thought: having a connection manager separate from mongoose/typegoose components but used by both.

Third thought: try to use the mongoose/mongodb internals to determine if the given uri already references a connection. Unlikely since nothing I've seen in the documentation can be used for an exact match- we'd have to make the connection first to determine if we're connecting to the same database.

These patterns are new to me- I'd love to get some direction :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mschnee I have updated the TypeORM extension to leverage the ideas for more flexible configuration. See https://github.com/strongloop/loopback-next/blob/typeorm/extensions/typeorm/src/services/connection-manager.ts.

As we show in https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/validation/ajv-factory.provider.ts as well, the configuration be injected. Extensions can contribute additional pieces to the configuration.

We can build connection option adapter for data source settings such as existing mongodb datasources can be discovered and referenced for mongoose usage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #5801, we are moving to a different direction than presented in https://github.com/strongloop/loopback-next/blob/typeorm/extensions/typeorm/src/services/connection-manager.ts. Instead of keeping a list of connection options at LoopBack side, we are introducing a new application API for registering a new connection and then we keep the actual connection object around.

https://github.com/strongloop/loopback-next/blob/d714638984da4deced7c36b4eb25457492759c69/packages/typeorm/src/mixin.ts#L37-L44

In this new design, users can use regular @inject, @inject.getter etc. decorators to receive connection instances.

Second thought: having a connection manager separate from mongoose/typegoose components but used by both.

Can we use LoopBack's context as the connection manager, the same way as @loopback/repository uses application context to keep track of datasources?

And then there's having both mongoose and typegoose models (two different extensions) use the same mongoose connection.

IIUC, typegoose is using mongoose under the hood, expecting app authors to pick & install a compatible version (mongoose is in peerDependency). I think that means the LoopBack extension should be using mongoose APIs to create connections and then find a way how to use those connections with typegoose models?

The question mark I have is that schemas are orthogonal to connections: a model object represents a schema defined on a specific connection, so schemas can be used across multiple connections. I've seen systems that export all the schemas from an schems/index.js file, and then have a separate connection that only uses a handful of them to create models specific to that connection.

@loopback/repository is already designed this way:

  • a datasource represents a MongoDB connection
  • a model class describes a schema
  • a repository class binds a model with a datasource
  • it's possible for a single model to be attached to multiple datasources

Can you perhaps use a similar design for Mongoose/Typegoose too?

@dhmlau
Copy link
Member

dhmlau commented Jun 18, 2020

@mschnee, do you have a chance to take a look at @raymondfeng's comments above? Thanks.

@dhmlau
Copy link
Member

dhmlau commented Aug 19, 2020

We just switch the contribution method from CLA to DCO, making your contribution easier in the future. Please sign the commits with DCO by amending your commit messages with -s flag and push the changes again. If you're using the command line, you can:

git commit --amend -s
git push --force-with-lease

Please refer to this docs page for details. Thanks!

@mschnee mschnee closed this Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants