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

Invalid ping query for TypeOrmHealthIndicator using oracle connection #374

Closed
mvandervliet opened this issue Sep 23, 2019 · 3 comments
Closed

Comments

@mvandervliet
Copy link
Contributor

I'm submitting a...


[ ] Regression 
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Fails when using TypeOrmHealthIndicator with TypeOrmModule configuration for an type: 'oracle' connection. The issue is due to the invalid SELECT 1 query used as the ping.

Expected behavior

The proper statement for type: 'oracle' should be SELECT 1 FROM DUAL

Minimal reproduction of the problem with instructions

  1. Create TypeOrm connection with type: 'oracle'
  2. Create HealthIndicator for TypeORM as described in docs
  3. Load /health and see 503 with status: 'down' and no message

What is the motivation / use case for changing the behavior?

Make TypeORM health indicator function for all supported connection types

Environment


Nest version: 6.7.1

 
For Tooling issues:
- Node version: v10.16.3  
- Platform:  Mac, Docker/Linux

Others:

@BrunnerLivio
Copy link
Member

Thanks a lot for reporting! Seems to be a valid issue. Would you like to create a PR? You could easily add this to the existing TypeOrmHealthIndicator#pingDb function.

private async pingDb(connection: Connection, timeout: number) {
const check =
connection.options.type === 'mongodb'
? connection.isConnected
? Promise.resolve()
: Promise.reject()
: connection.query('SELECT 1');
return await promiseTimeout(timeout, check);
}

Maybe you'll need to refactor the statement to a switch/case or if/else statement, since the "if"-shorthand gets hard to read with 3+ options.

@mvandervliet
Copy link
Contributor Author

sure @BrunnerLivio, I can definitely open the PR... I'd actually solved in a workaround by extending the class for now, but I can refactor the method and let y'all review 😉

@BrunnerLivio
Copy link
Member

Included in 6.5.2 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants