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
  

  moleculer-db/packages/moleculer-db-adapter-mongoose/src/index.js

  Lines 80 to 85 in 072f1b6

  	} else if (this.schema) {
  	conn = new Promise(resolve =>{
  	const c = mongoose.createConnection(this.uri, this.opts);
  	this.model = c.model(this.modelName, this.schema);
  	resolve(c);
  	});

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

  moleculer-db/packages/moleculer-db/src/index.js

  Lines 1107 to 1110 in 072f1b6

  	setTimeout(() => {
  	this.logger.warn("Reconnecting...");
  	connecting();
  	}, 1000);

• A stroke of luck has happened here
  At line 83, when adapter got schema field, adapter create a model
  

  moleculer-db/packages/moleculer-db-adapter-mongoose/src/index.js

  Line 83 in 072f1b6

  this.model = c.model(this.modelName, this.schema);

  
  When adapter do connect() again, because model field is set, so it jump to below code that use mongoose.connection to connect
  

  moleculer-db/packages/moleculer-db-adapter-mongoose/src/index.js

  Lines 70 to 80 in 072f1b6

  	if (this.model) {
  	/* istanbul ignore next */
  	if (mongoose.connection.readyState == 1) {
  	this.db = mongoose.connection;
  	return Promise.resolve();
  	} else if (mongoose.connection.readyState == 2) {
  	conn = mongoose.connection.asPromise();
  	} else {
  	conn = mongoose.connect(this.uri, this.opts);
  	}
  	} else if (this.schema) {

  
  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 use this.model.db as NativeConnection instance #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
  

  moleculer-db/packages/moleculer-db-adapter-mongoose/src/index.js

  Lines 59 to 151 in 81f4f3f

  	/**
  	* Connect to database
  	*
  	* @returns {Promise}
  	*
  	* @memberof MongooseDbAdapter
  	*/
  	connect() {
  	return new Promise((resolve) => {
  	if (this.model) {
  	// model field exists in service schema, should check if model has been connected
  	if (this.model.db) {
  	// if this.model.db is existed, adapter had connected before, just return this.model.db
  	// Model.prototype.db
  	// Connection the model uses.
  	// https://mongoosejs.com/docs/api/model.html#model_Model-db
  	resolve(this.model.db);
  	return;
  	}
  	/* istanbul ignore next */
  	if (
  	mongoose.connection.readyState === mongoose.connection.states.connected ||
  	mongoose.connection.readyState === mongoose.connection.states.connecting
  	) {
  	// if mongoose is connecting, will handle below
  	resolve(mongoose.connection);
  	} else {
  	// everything else cases mean we not yet do connect before, make it
  	const conn = mongoose.createConnection(this.uri, this.opts);
  	resolve(conn);
  	}
  	} else if (this.schema) {
  	const conn = mongoose.createConnection(this.uri, this.opts);
  	this.model = conn.model(this.modelName, this.schema);
  	resolve(conn);
  	}
  	}).then(conn => {
  	this.conn = conn;
  	if (this.conn.db != null) {
  	return this.conn.db;
  	} else if (this.conn.readyState === mongoose.connection.states.connecting) {
  	return new Promise((resolve, reject) => {
  	this.conn.once("error", reject);
  	this.conn.once("connected", () => {
  	this.conn.removeListener("error", reject);
  	resolve(this.conn.db);
  	});
  	});
  	}
  	throw new MoleculerError("MongoDB connection failed to get DB object");
  	}).then(db => {
  	this.db = db;
  	this.service.logger.info("MongoDB adapter has connected successfully.");
  	// do not use this.db.on() because of next line
  	// DeprecationWarning: Listening to events on the Db class has been deprecated and will be removed in the next major version.
  	/* istanbul ignore next */
  	this.conn.on("disconnected", () => this.service.logger.warn("Mongoose adapter has disconnected."));
  	this.conn.on("error", err => this.service.logger.error("MongoDB error.", err));
  	this.conn.on("reconnect", () => this.service.logger.info("Mongoose adapter has reconnected."));
  	});
  	}
  	/**
  	* Disconnect from database
  	*
  	* @returns {Promise}
  	*
  	* @memberof MongooseDbAdapter
  	*/
  	disconnect() {
  	return new Promise(resolve => {
  	if (this.conn == null) {
  	// model was created and mongoose maybe connected before call .connect()
  	mongoose.connection.close(resolve);
  	} else if (this.conn.readyState === mongoose.connection.states.connecting) {
  	// disconnect() was called before connect() success
  	// try to disconnect at nextTick
  	setTimeout(() => resolve(this.disconnect()), 0);
  	} else if (this.conn.close) {
  	this.conn.close(resolve);
  	} else if (this.db != null && this.db.close) {
  	this.db.close(resolve);
  	} else {
  	// for unknown cases
  	mongoose.connection.close(resolve);
  	}
  	});
  	}

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 #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

moleculer-db/packages/moleculer-db-adapter-mongoose/src/index.js

Line 78 in 47f0357

return mongoose.createConnection(this.uri, this.mongooseOpts).asPromise().then(conn => {

this line will create new connection but the model can be connected before

This case was handled in current version and in my pull

moleculer-db/packages/moleculer-db-adapter-mongoose/src/index.js

Lines 70 to 76 in 81f4f3f

	if (this.model.db) {
	// if this.model.db is existed, adapter had connected before, just return this.model.db
	// Model.prototype.db
	// Connection the model uses.
	// https://mongoosejs.com/docs/api/model.html#model_Model-db
	resolve(this.model.db);
	return;

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 .

moleculer-db/packages/moleculer-db-adapter-mongoose/src/index.js

Line 91 in 47f0357

this.model = this.conn.model(this.model["modelName"],this.model["schema"]);

( + 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

moleculer-db/packages/moleculer-db-adapter-mongoose/src/index.js

Line 91 in 47f0357

this.model = this.conn.model(this.model["modelName"],this.model["schema"]);

( 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?