mongoose adapter trouble with connect

[0x0a0d]

I created a pull #338 that fix a problem in connect() method of adapter mongoose Today, I've created a new project with moleculer-db-adapter-mongoose and I still got error message when connect to mongodb

problem

The code below can be used to reproduce the problem

const { ServiceBroker } = require('moleculer');
const { Schema } = require('mongoose')
const DbService = require('moleculer-db')
const MongooseDbAdapter = require('moleculer-db-adapter-mongoose')

const broker = new ServiceBroker({
  logger: {
    type: 'Console',
    options: {
      formatter: 'short',
    }
  }
})

broker.createService({
  name: 'foo',
  schema: new Schema({}),
  modelName: 'foo',
  mixins: [DbService],
  adapter: new MongooseDbAdapter("mongodb://127.0.0.1/test"),
})

broker.start().then(() => {
  console.log('Broker started')
  return broker.stop()
}).catch(() => {
  console.log('Broker failed to start')
  console.error(err)
})

Here is screenshot

You can see that the foo service was not make connection to db server but success at retry

For more detail, here what happen

• First because I used schema, so the line below resolve with the conn from mongoose.createConnection https://github.com/moleculerjs/moleculer-db/blob/072f1b67ceb8798d061ca587ea21cb01703195a3/packages/moleculer-db-adapter-mongoose/src/index.js#L80-L85
• Next, after connect successfully, the promise should be resolved with conn.then(), but instead check conn is connected or not, the code use mongoose.connection that always point to mongoose.connections[0] (default mongoose connection, but with any call of mongoose.createConnection will result to append new connection to mongoose.connections).
• The connect() was thrown an error and catched by moleculer-db that asked adapter to reconnect. https://github.com/moleculerjs/moleculer-db/blob/072f1b67ceb8798d061ca587ea21cb01703195a3/packages/moleculer-db/src/index.js#L1107-L1110
• A stroke of luck has happened here At line 83, when adapter got schema field, adapter create a model https://github.com/moleculerjs/moleculer-db/blob/072f1b67ceb8798d061ca587ea21cb01703195a3/packages/moleculer-db-adapter-mongoose/src/index.js#L83 When adapter do connect() again, because model field is set, so it jump to below code that use mongoose.connection to connect https://github.com/moleculerjs/moleculer-db/blob/072f1b67ceb8798d061ca587ea21cb01703195a3/packages/moleculer-db-adapter-mongoose/src/index.js#L70-L80 So at this time, connection was successfully

But for every services use schema will share same mongoose connection

updated code

const { ServiceBroker } = require('moleculer');
const { Schema } = require('mongoose')
const DbService = require('moleculer-db')
const MongooseDbAdapter = require('moleculer-db-adapter-mongoose')

const broker = new ServiceBroker({
  logger: {
    type: 'Console',
    options: {
      formatter: 'short',
    }
  }
})

const foo = broker.createService({
  name: 'foo',
  schema: new Schema({}),
  modelName: 'foo',
  mixins: [DbService],
  adapter: new MongooseDbAdapter("mongodb://127.0.0.1/test"),
})

const bar = broker.createService({
  name: 'bar',
  schema: new Schema({}),
  modelName: 'bar',
  mixins: [DbService],
  adapter: new MongooseDbAdapter("mongodb://127.0.0.1/test"),
})

broker.start().then(() => {
  console.log('Broker started')
  console.log(`Connection of foo and bar is ${foo.adapter.conn === bar.adapter.conn ? 'same' : 'different'}`)
  return broker.stop()
}).catch(() => {
  console.log('Broker failed to start')
  console.error(err)
})

screenshot

fix

• I copied #338 connect and disconnect methods, create a patch file using by patch-package module that fix this problem patches.zip Unzip this file to root path then run npx patch-package

• or copy below lines to overwrite connect, disconnect method in moleculer-db-adapter-mongoose/src/index.js https://github.com/moleculerjs/moleculer-db/blob/81f4f3f1f9ab8e05036a6192851d320712f6f1a3/packages/moleculer-db-adapter-mongoose/src/index.js#L59-L151

need support

@icebob if you think this fix is correct, can you (or someone else) can help me to write tests? I not good to write test and dont have enough time at this moment

[icebob]

Please create a fix PR and I will help with tests

[icebob]

Btw, maybe this PR will conflict with https://github.com/moleculerjs/moleculer-db/pull/370

ping @thib3113

[thib3113]

@icebob more than "conflict" . This bug will be handled in the Pr #370 .

In fact, the problem come from reusing the same connection ... correcting the database per service pattern will use different connections per services, and so this bug will be fixed .

[0x0a0d]

I dont think #370 is good solution https://github.com/moleculerjs/moleculer-db/blob/47f03579f03c850573c3d02ae8f495f3feaeffa1/packages/moleculer-db-adapter-mongoose/src/index.js#L78 this line will create new connection but the model can be connected before

This case was handled in current version and in my pull https://github.com/moleculerjs/moleculer-db/blob/81f4f3f1f9ab8e05036a6192851d320712f6f1a3/packages/moleculer-db-adapter-mongoose/src/index.js#L70-L76

I think #370 is good to upgrade mongoose to newer version but handle connect/disconnect is not

[thib3113]

@0x0a0d can you describe "model" ? because actually, model is not directly used, but extracted ... So, not connected to the global connection .

https://github.com/moleculerjs/moleculer-db/blob/47f03579f03c850573c3d02ae8f495f3feaeffa1/packages/moleculer-db-adapter-mongoose/src/index.js#L91

( + main concept of moleculer-db is "database per service" . Reusing global connection is not "database per service" : https://moleculer.services/docs/0.14/moleculer-db )

[0x0a0d]

See this

require('dotenv').config()
const { createConnection, Schema } = require('mongoose')
process.env.MONGO_URI = process.env.MONGO_URI || 'mongodb://localhost:27017/test'
!async function(){
  const conn1 = await createConnection(process.env.MONGO_URI).asPromise()
  const conn2 = await createConnection(process.env.MONGO_URI).asPromise()

  const foo = conn1.model('foo', new Schema({}))
  const bar = conn2.model('bar', foo.schema)

  console.log(`Connection of foo and bar is ${foo.db === bar.db ? 'same' : 'different'}`)

  await conn1.close()
  await conn2.close()
}()

Result

Your code create new connection for every services but in some cases, dev can do this

const shareConnection = createConnection(...)
const fooModel = shareConnection.model('foo', {...})
const barModel = shareConnection.model('foo', {...})

const serviceFoo = {
...,
model: fooModel,
}
const serviceBar = {
...,
model: barModel,
}

[thib3113]

@0x0a0d yes, and if dev do this, they don't respect the base concept of moleculer-db : "Database per service" pattern : https://moleculer.services/docs/0.14/moleculer-db .

[0x0a0d]

I dont think one database mean one database connection

[thib3113]

yes . But in my opinion (and just my opinion here), a service need to manage his own connection, to be totally autonomous . If another service crash the connection or disconnect it, it need to doesn't impact other services .

If a service want to reuse his connection, he can acces it from functions .

[0x0a0d]

see this https://mongoosejs.com/docs/models.html#constructing-documents

there are 2 ways to create a model

• mongoose.model() => default mongoose.connection
• conn.model() => use connection from conn

that mean: if model is existed, there are 50% chances a connection was made before if connection was made before, now adapter create a new connection, who will handle model current connection? if dev need to create new connection, why dont pass schema instead of model?

at the end, this also just my opinion, lets icebod decide

[thib3113]

yes, @icebob will decide .

About passing schema or model . I see model only like "sugar" . because schema need two parameters ( schema + schemaName )

[icebob]

As I see the mongoose connection handling is not an easy problem. All adapters (except mongoose) create a custom connection to the db. So it would be good to follow the same logic in Mongoose as well, but using Model instance with a global connection can cause issues.

@thib3113 @0x0a0d Can we create a solution which can cover both logic? As @0x0a0d mentioned, if we use schema it creates a connection, but for model the service uses the global connection and we describe this logic in the documentation. WDYT?

[0x0a0d]

As I see the mongoose connection handling is not an easy problem. All adapters (except mongoose) create a custom connection to the db. So it would be good to follow the same logic in Mongoose as well, but using Model instance with a global connection can cause issues.

@thib3113 @0x0a0d Can we create a solution which can cover both logic? As @0x0a0d mentioned, if we use schema it creates a connection, but for model the service uses the global connection and we describe this logic in the documentation. WDYT?

My pull code follow current adapter mongoose logic

1. if model is present, try to reuse it connection
2. if schema is present, create new connection for it

[thib3113]

So it would be good to follow the same logic in Mongoose as well, but using Model instance with a global connection can cause issues.

Actually, the connection from the Model is not used . The model is just used to extract modelName and schema https://github.com/moleculerjs/moleculer-db/blob/47f03579f03c850573c3d02ae8f495f3feaeffa1/packages/moleculer-db-adapter-mongoose/src/index.js#L91

( so only one parameter instead of two with schema / modelName ) .

I understand that reusing the global connection can save a connection to mongodb ... But so the services will be linked together . (and so => if I stop serviceA, serviceB will crash because connection is closed ?) .

A always think it's better to follow the same path than other modules ... one connection per service, and so they are totally independent .

@icebob you are the expert here, it's up to you to do a choice

[icebob]

I don't want to kill any common use-case. My suggestion is to add a reuseModelConnection: true|false to the service settings (similar to useNativeMongooseVirtuals) and handle the connection by this setting.

Is it acceptable to both of you?