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

[RFC WIP] feat(typegoose): add a component for typegoose/mongoose integration #5152

Closed
wants to merge 4 commits into from

Conversation

mschnee
Copy link
Contributor

@mschnee mschnee commented Apr 19, 2020

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

Unlike TypeORM, which provides connections that have typed entity managers, mongoose and Typegoose put the Model above the connection, and developers interact with Model objects directly to fetch/query/update documents through the underlying connection.

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

Goals

  • Easily take your existing mongoose or typegoose schemas, and use them in loopback without creating a specialized connection provider of your own.
  • @inject() the connection-models you need when you need them, instead of require()ing them in the global scope, which is common in lots of mongoose/typegoose examples.
  • Keep it simple.

Example?

/**
 * schemas/User.schema.ts
 */
export class User {
  @prop()
  firstName: string;
}
export const UserBindingKey = BindingKey.create<ReturnModelType<typeof User>>(
  'key.for.this.connection.Model',
);

/**
 * schemas/Event.schema.ts
 */
@modelOptions({schemaOptions: {discriminatorKey: 'kind'}})
@pre<Event>('save', function () {
  this.createdAt = new Date();
})
export class Event {
  @prop()
  createdAt: Date;
}
export const EventBindingKey = BindingKey.create<ReturnModelType<typeof Event>>(
  'key.for.this.connection.Event',
);

/**
 * schemas/LoginEvent.schema.ts
 */
export class LoginEvent extends Event {
  @prop({ref: 'User'})
  _user: Ref<User>;
}
const LoginEventBindingKey = BindingKey.create<
  ReturnModelType<typeof LoginEvent>
>('key.for.this.connection.LoginEvent');

/**
 * application.ts
 */
import {Event, LoginEvent, User } from './schemas';
import {EventBindingKey, LoginEventBindingKey, UserEventBindingKey} from './schemas';
import {TypegooseBindings, TypegooseCompeonent} from '@loopback/typegoose';

class Application extends RestApplication {
  constructor(options?: ApplicationOptions) {
    super(options);
  
    this.configure(TypegooseBindings.COMPONENT).to({
      uri: options.MONGO_URL || 'mongodb://localhost:27017',
      schemas: [
        {bindingKey: UserBindingKey, schema: User},
        {bindingKey: EventBindingKey, schema: Event},
      ],
      discriminators: [
        {
          bindingKey: LoginEventBindingKey,
          modelKey: EventBindingKey,
          schema: LoginEvent,
        },
      ],
    });
    this.component(TypegooseComponent);
  }
}

/**
 * controllers/user.controller.ts
 */
export class UserController {
  constructor(
    @inject(UserBindingKey) UserModel: ReturnModelType<typeof User>
  )

  @get('/first-user')
  public async function() {
    const user = await this.UserModel.findOne();
    return user?._id ?? null;
  }
}

TODO

  • Tests need to be greatly expanded beyond the simple integration test
  • The TestTypegooseComponent is messy, and I'm not sure if it's a good pattern.
  • Requiring BindingKey may be too strict, and too inflexible for some cases where applications may have dynamic connections.
  • What's the best way to pass configuration when an application has hundreds of schemas? application.ts seems like the wrong place.
  • Also provide a similar component for mongoose
  • Need to identify a way to share connections via configuration. Specifically, typegoose accepts an existing mongoose connection, and this is absolutely necessary for any system that have both mongoose and typegoose models.

Background

The Hello World example for mongoose looks like this:

const mongoose = require('mongoose');
await mongoose.connect('mongodb://127.0.0.1:27017');

const userSchema = new mongoose.Schema({
  firstName: { type: String },
})
const UserModel = mongoose.model(userSchema, 'User');

const newUser = await UserModel.create({ firstName: 'Matt' });

What's hidden under the hood are the connections. You can connect to multiple database hosts.

const mongoose = require('mongoose');
const connection1 = await mongoose.createConnection('mongodb://127.0.0.1:27017');
const connection2 = await mongoose.createConnection('mongodb://localhost:27017');

const userSchema = new mongoose.Schema({
  firstName: { type: String },
})

const UserModel1 = connection1.model(userSchema, 'User');
const UserModel2 = connection2.model(userSchema, 'User');

Even though these two connections refer to the same database, mongoose treats references separately.

Typegoose adds stricter type checking with Typescript to mongoose:

class User  {
  @prop()
  firstName: string;
}

const UserModel = getModelForClass(User);
class User  {
  @prop()
  firstName: string;
}

const UserModel = getModelForClass(User, {
  existingConnection, // using an already created connection from mongoose.createConnection()
  existingMongoose, // using an already connected mongoose singleton
});

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

@bajtos
Copy link
Member

bajtos commented Apr 20, 2020

Thank you for starting the discussion about adding support for Mongoose-based ORM framework for working with MongoDB! ❤️ 👏

Few high-level comments:

@raymondfeng can you please review? You are the author of #4794 which inspired this work.
@agnes512 @hacksparrow can you please review too? You are standby owners of MongoDB and Model/Repository areas.

@mschnee
Copy link
Contributor Author

mschnee commented Apr 21, 2020

In fact, I did not. I will do so, and commit changes.

@raymondfeng
Copy link
Contributor

@mschnee Great initiative. In addition to my comments inline, I would suggest:

  1. Split the PR into two - one for mongoose and the other for typegoose
  2. Have more information in README

docs: add more example code to README.md

fix: use build-extension to create @loopback/typegoose

feat: adds mongoose variant

docs: update README files

fix: license headers

fix: remove mongoose
@mschnee mschnee force-pushed the feat/typegoose-extension branch from aa783dc to 742bf6c Compare April 23, 2020 02:19
@mschnee
Copy link
Contributor Author

mschnee commented Apr 23, 2020

Done!

I'm not entirely sure how I feel about having to pass all the schemas in via configuration. It's not the worst possible pattern- for both the typegoose and mongoose extensions I'm using the index.ts file and an exported function for this purpose.

@mschnee mschnee mentioned this pull request Apr 23, 2020
7 tasks
@raymondfeng
Copy link
Contributor

I'm not entirely sure how I feel about having to pass all the schemas in via configuration. It's not the worst possible pattern- for both the typegoose and mongoose extensions I'm using the index.ts file and an exported function for this purpose.

We can possibly define an extension point for Mongoose schemas. Then there can be a booter to load such schemas or have app code to explicitly contribute to the extension point.

@bajtos
Copy link
Member

bajtos commented Jun 26, 2020

I'm not entirely sure how I feel about having to pass all the schemas in via configuration. It's not the worst possible pattern- for both the typegoose and mongoose extensions I'm using the index.ts file and an exported function for this purpose.

Will this design allow applications to register additional schemas dynamically at runtime, after the application started?

@hacksparrow
Copy link
Contributor

@mschnee thanks for the PR. Here's my feedback.

  1. The DX should be in tune with the LB4 way - declarative, bootable, and dynamic (usable after the app starts).
  2. We should expose a friendlier API than directly exposing that of Mongoose when it comes to configuration. Bootable artifacts provide that opportunity.
  3. The TypeORM extension is recently added extension, new extensions could use it as a template.

@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.

5 participants