MikroOrmHealthIndicator does not work with the `defineConfig` helper

[ctfdavis]

Is there an existing issue for this?

• I have searched the existing issues

Current behavior

Summary of the issue

Currently, the pingDb method of the MikroOrmHealthIndicator class makes use of the type option of the MikroOrm configuration. However, the type would be undefined if the defineConfig helper exported from the driver package is used for the MikroOrm configuration without providing the type option, causing the pingDb method to fail with a rather cryptic error message:

undefined ping check is not implemented yet

In addition, the type option will be removed in MikroOrm v6, and the suggested way of defining configuration is using the defineConfig helper exported from the driver package.

The issue

Here’s the current implementation of pingDb method, which would throw a cryptic error message undefined ping check is not implemented yet if the user uses the defineConfig helper exported from the driver package to configure MikroOrm without providing the type option:

  private async pingDb(connection: MikroOrm.Connection, timeout: number) {
    let check: Promise<any>;
    const type = connection.getPlatform().getConfig().get('type’); // type can be undefined here!

    switch (type) {
      case 'postgresql':
      case 'mysql':
      case 'mariadb':
      case 'sqlite':
        check = connection.execute('SELECT 1');
        break;
      case 'mongo':
        check = connection.isConnected();
        break;
      default:
        throw new NotImplementedException(
          `${type} ping check is not implemented yet`, // if type is undefined, then we’d get a cryptic error message
        );
    }
    return await promiseTimeout(timeout, check);
  }

One way to fix it is to check the driver instead, which would work with both the original way of configuration and defineConfig helper:

  private async pingDb(connection: MikroOrm.Connection, timeout: number) {
    let check: Promise<any>;
    const driver = connection.getPlatform().getConfig().getDriver().constructor.name;

    switch (driver) {
      case 'PostgreSqlDriver':
      case 'MySqlDriver':
      case 'MariaDbDriver':
      case 'SqliteDriver':
        check = connection.execute('SELECT 1');
        break;
      case 'MongoDriver':
        check = connection.isConnected();
        break;
      default:
        throw new NotImplementedException(
          `${driver} ping check is not implemented yet`,
        );
    }

    return await promiseTimeout(timeout, check);
  }

Unlike get(‘type’) which may return undefined, the getDriver() method always returns a database driver (of the IDatabaseDriver interface), so the above switch case would no longer have an undefined case. Therefore, even in the default case, the user would no longer encounter a cryptic error message, i.e., undefined ping check is not implemented yet.

Let me know if the above proposal is good enough. I am happy to help create a pull request to fix this issue.

Thanks everyone!

Minimum reproduction code

In the sample 011-mirkoorm-app, make the following changes in health.module.ts to use the defineConfig helper from MikroOrm:

import { MikroOrmModule } from "@mikro-orm/nestjs";
import { Module } from "@nestjs/common";
import { HealthController } from "./health.controller";
import { TerminusModule } from "@nestjs/terminus";
import { defineConfig } from "@mikro-orm/mysql";

@Module({
  imports: [
    // MikroOrmModule.forRoot({
    //   type: 'mysql',
    //   dbName: 'test',
    //   user: 'root',
    //   password: 'root',
    //   host: '0.0.0.0',
    //   port: 3306,
    //   discovery: { warnWhenNoEntities: false }, // disable validation entities
    //   strict: true,
    // }),
    MikroOrmModule.forRoot(
      defineConfig({
        dbName: "test",
        user: "root",
        password: "root",
        host: "0.0.0.0",
        port: 3306,
        discovery: { warnWhenNoEntities: false }, // disable validation entities
        strict: true,
      })
    ),
    TerminusModule,
  ],
  controllers: [HealthController],
})
export class HealthModule {}

Run the app and make a GET request to the /health endpoint. The below cryptic error message would be returned:

undefined ping check is not implemented yet

Steps to reproduce

No response

Expected behavior

• Should not throw a cryptic error message such as undefined ping check is not implemented yet
• Should be able to work with the defineConfig helper

Package version

9.2.0

NestJS version

9.3.2

Node.js version

No response

In which operating systems have you tested?

• macOS
• Windows
• Linux

Other

No response

[BrunnerLivio]

@ctfdavis Awesome investigation! Appreciate it! Would you like to create a PR?

It would also be good to know when connection.getPlatform().getConfig().getDriver() was introduced. If that is a rather newer-ish way to access the driver type we might need to use a fallback to the legacy connection.getPlatform().getConfig().get('type’)

[ctfdavis]

@BrunnerLivio Upon further investigation, I think the differentiation between sql and nosql drivers in the current pingDb method is actually unnecessary.

The Connection abstract class from MikroORM has an abstract method isConnected() (source) intended for checking if the connection to the database has been successfully established. isConnected() has been a required method for the Connection class since MikroORM v2 (4 years ago, source). So it is definitely safe to use it in this library.

All built-in connection classes extend the Connection abstract class. Specifically, the MongoConnection directly extends the abstract class and implements it the mongo way (source). For all sql client connections, including the MySqlConnection class, they extend the AbstractSqlConnection class (source) which extends the Connection class and implements the isConnected() method using a sql query (essentially the same as what the current pingDb method does, source).

So, with the above information, I think we can simplify the pingDb method as below:

  private async pingDb(connection: MikroOrm.Connection, timeout: number) {
    return await promiseTimeout(timeout, connection.isConnected());
  }

Not only is it simpler and works with all built-in connection types since MikroORM v2, it has two additional benefit:

• it is future-proof to added built-in connection types
• it automatically supports custom connection types (see here, custom connections are required to extend the Connection class)

Let me know if this proposal is good. I will create a PR afterwards if you agree. Thanks a lot!

[BrunnerLivio]

@ctfdavis Sounds good to me!

[BrunnerLivio]

Released with v10.1.0 🎉