Check if datasource is initialized before destroying it

[thomasconner]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Other... Please describe:

What is the current behavior?

If a NestJS app destroys a dataSource instance before app shutdown, they will see the following error:

Cannot execute operation on "default" connection because connection is not yet established.

Issue Number: N/A

What is the new behavior?

A NestJS app can destroy a dataSource instance before app shutdown and they will not see the following error logged:

Cannot execute operation on "default" connection because connection is not yet established.

Does this PR introduce a breaking change?

• Yes
• No

Other information

[kamilmysliwiec]

We call initialize() within the DataSource provider factory. It doesn't seem to be possible then (that DataSource might not be initialized yet at this point). Can you share a minimum reproduction repository?

[thomasconner]

Yes let me try and create something

[thomasconner]

Please give https://github.com/thomasconner/nestjs-typeorm-test a try. I have included a docker-compose.yml file to setup Postgres. If you execute npm run test:e2e you will see the error that I mentioned about. We ran into this with e2e tests we had at work. All the tests would pass but CircleCI would fail because of the error that gets logged.

[thomasconner]

The argument can be made that we should just remove the line that destroys the datasource in our tests which would definitely solve the problem. But my reasoning for changing it here is that the expectation should be that I could call datasource.destroy() without there being an error thrown.

[kamilmysliwiec]

The argument can be made that we should just remove the line that destroys the datasource in our tests which would definitely solve the problem

And I think that's the valid argument TBH. You're trying to destroy a DataSource in your test suite that has been already terminated in the onApplicationShutdown hook, and we can't really remove it from there as we don't want folks to manually terminate data sources that were auto-created & are maintained within the module.

But my reasoning for changing it here is that the expectation should be that I could call datasource.destroy() without there being an error thrown.

What's the reason for manually calling destroy?

[thomasconner]

No specific reason to be honest. We upgraded from TypeORM v2 to v3. Before there was a connection.close() so this was replaced, without much thought, with datasource.destroy(). This is the only reason it existed.

[thomasconner]

Any update on this?