fix: synchronize with typeorm_metadata table only if needed

[julianpoemp]

Description of change

Creating a view by calling createQueryRunner().createView(...) results in an error: QueryFailedError: relation "typeorm_metadata" does not exist (see #9173). This pull request fixes #9173 by creating the metadata table if it doesn't exist.

Fixes: #9173

Pull-Request Checklist

• Code is up-to-date with the master branch
• npm run format to apply prettier formatting
• npm run test passes with this change
• This pull request links relevant issues as Fixes #0000
• There are new or updated unit tests validating the change
• Documentation has been updated to reflect this change N/A
• The new commits follow conventions explained in CONTRIBUTING.md

[nacholupotti]

HI, getting same error here. Will this PR be merged soon?

[julianpoemp]

@nacholupotti the PR is ready for merge, but it's waiting for a reviewer. @AlexMesser can you review it, please?

[christian-forgacs]

We've the same issue :(

[julianpoemp]

@christian-forgacs the fastest solution is to clone the branch fix-missing-typeorm-metadata branch from my fork and build the typeorm package from it. After that you get a .zip archive that you can copy to your project and install using npm. That's how I do it in my projects.

Unfortunately I don't know when and whether this PR is going to be merged or not.

[julianpoemp]

@AlexMesser I applied your suggestions from your review. Now my test doesn't succeeds any more, even if it just calls

await connection.createQueryRunner().createView(
                new View({
                    name: "test_view",
                    expression: "SELECT * FROM test_table",
                }),
            )

So this issue isn't one that just happens in migrations, but rather every time createView() is called. So I had a look at how this function works. I found out that the insertTypeormMetadataSql function from BaseQueryrunner.ts doesn't checks if the meta data table already exists and fails.

So I think there are two options:

1. We check in insertTypeormMetadataSql if the meta data table exists. If not we create it. (EDIT: this doesn't help, because it's only called in migrations.

2. The createMetadataTableIfNecessary() function somehow recognizes that a metadata_table is needed in the future as soon as insertTypeormMetadataSql is called.

3. Each QueryRunner has to add a check for the typeorm table in createView

I tried to implement option 1, but because this function isn't async, I can't create the meta data table using the schema builder. Option 3 seems the "easiest" one but requires a change to each createView function for each QueryRunner.

EDIT: Added Option 3.

[pleerock]

The way createMetadataTableIfNecessary works - it checks if you have classes decorated by @View and if you do have - it knows it must create metadata table for you.

Looks like you don't have such classes (decorated by @View) and it means you don't generate migration based on changes in your code, but instead manually write them. Correct me if I'm wrong.

Option #1 doesn't work as you pointed.

Option #2 isn't implementable because there is no way to TypeORM to guess if you have views hard-coded.

Option #3 won't work as well because all these QueryRunner methods must be side-effect-less and they only generate SQL strings (by pushing into up/down queries), we do it because we generate migrations out of SQLs pushed into these arrays.

typeorm_metadata is needed only if you have classes decorated by @View / @Entity, because it uses the metadata to sync old view/entity class definition with new one. Since you don't have a class and instead creating view dynamically using static code in the migrations, sync won't ever work. So, we don't really need metadata in such case.

I suggest to add additional boolean param for such cases:

async createView(view: View, syncWithMetadata: boolean = false): Promise<void> {
        const upQueries: Query[] = []
        const downQueries: Query[] = []
        upQueries.push(this.createViewSql(view))
        if (syncWithMetadata)
            upQueries.push(await this.insertViewDefinitionSql(view))
        downQueries.push(this.dropViewSql(view))
        if (syncWithMetadata)
            downQueries.push(await this.deleteViewDefinitionSql(view))
        await this.executeQueries(upQueries, downQueries)
    }

We can make it false by default and send true where we use it in TypeORM codebase to keep current behavior.

[julianpoemp]

@pleerock

Looks like you don't have such classes (decorated by @view) and it means you don't generate migration based on changes in your code, but instead manually write them. Correct me if I'm wrong.

Yes.

typeorm_metadata is needed only if you have classes decorated by @view / @entity, because it uses the metadata to sync old view/entity class definition with new one. Since you don't have a class and instead creating view dynamically using static code in the migrations, sync won't ever work. So, we don't really need metadata in such case.

I understand. So, a workaround would be to create a separate View class even if I don't use it.

I suggest to add additional boolean param for such cases:

That is a really short and easy solution, very nice! But wouldn't we have to change all createView() functions for each QueryRunner and change all code occurrences that needs the old behaviour? If we set the default to true, the old behaviour would be the default (no code changes needed) and only in special cases like my one the param is set to false...

await connection.createQueryRunner().createView(
                new View({
                    name: "test_view",
                    expression: "SELECT * FROM test_table",
                }),
                false
)

in order to prevent the synchronization with metadata_table.

If you think this is the best solution, I could implement and test it. Please let me know how you decide.

[pleerock]

But wouldn't we have to change all createView() functions for each QueryRunner and change all code occurrences that needs the old behaviour?

We only need to supply true in TypeORM codebase, where we actually need current behavior.

Cases where users were using code call of createView (and they had metadata table) were not valid - because they don't need metadata for code-created views.

[julianpoemp]

@pleerock I implemented it as you described. I ran the test and I tested it in my project, it works fine. Thank you! 😄

[pleerock]

tests seems to fail

[julianpoemp]

@pleerock most of the tests run successfully but not the cockroachdb one. I can't find the reason, the test fails with my build and with that one from the origin typeorm master repository. Every time it's another error, I don't know what's going on here.

 1) "before each" hook for "should load data when additional condition used"


  582 passing (11m)
  14 pending
  1 failing

  1) query builder > joins
       "before each" hook for "should load data when additional condition used":
     Error: Timeout of 120000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/ips/repos/typeorm-orig/typeorm/build/compiled/test/functional/query-builder/join/query-builder-joins.js)
      at listOnTimeout (node:internal/timers:559:17)
      at processTimers (node:internal/timers:502:7)

 1) "before each" hook for "should not override already set properties"


  569 passing (16m)
  14 pending
  1 failing

  1) query builder > entity updation
       "before each" hook for "should not override already set properties":
     QueryFailedError: internal error: deadline below read timestamp is nonsensical; txn has would have no chance to commit. Deadline: 1663672502.269035525,0. Read timestamp: 1663672555.764900000,1 Previous Deadline: 1663672502.269035525,0.
      at CockroachQueryRunner.query (src/driver/cockroachdb/CockroachQueryRunner.ts:302:19)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at CockroachQueryRunner.commitTransaction (src/driver/cockroachdb/CockroachQueryRunner.ts:190:25)
      at CockroachQueryRunner.clearDatabase (src/driver/cockroachdb/CockroachQueryRunner.ts:2479:46)
      at DataSource.dropDatabase (src/data-source/DataSource.ts:356:17)
      at DataSource.synchronize (src/data-source/DataSource.ts:315:29)
      at async Promise.all (index 3)

[pleerock]

@julianpoemp nevermind cockroachdb is dumb and fails sometimes. This one is a good to merge. Thank you for contribution!