Sqlite attach #8396
Sqlite attach #8396
Conversation
|
Looks like I also can't reopen previous PR since you already created a new one (even if I close new one). Okay, so let's continue discussion here. |
...onal/multi-database/multi-database-basic-functionality/multi-database-basic-functionality.ts
Outdated
Show resolved
Hide resolved
| for (const database of databases) { | ||
| await queryRunner.clearDatabase(database); | ||
| // await queryRunner.clearDatabase(); | ||
| if (databases.length === 0) { |
There was a problem hiding this comment.
how this is possible if on line 265 you push the database in case of databases.length === 0 ?
There was a problem hiding this comment.
that push also has a conditional with && this.driver.database ?? Maybe it has no effect but thought it better to be safe?
Can remove the .clearDatabase() if wanted though...
| @@ -115,6 +115,11 @@ export interface BaseConnectionOptions { | |||
| */ | |||
| readonly extra?: any; | |||
|
|
|||
| /** | |||
| * The directory where ormconfig has been read from | |||
There was a problem hiding this comment.
It's not clear from a comment what this option does.
There was a problem hiding this comment.
ConnectionOptions should not depend on ormconfig.json.
ormconfig.json is just one of the connection options source.
There was a problem hiding this comment.
It exposes the baseDirectory so it can be used to create absolute paths to the database files
I have added better comment and marked it as '@internal'
| // decorated name will be assumed relative to main database file when non absolute. Paths supplied as absolute won't be portable | ||
| const absFilepath = isAbsolute(database) ? database : path.join(this.getMainDatabasePath(), database); | ||
|
|
||
| this.attachedDatabases[database] = { |
There was a problem hiding this comment.
this seems to be a side effect of buildTableName. Can't we do this "population" logic in a different "initialization phase" place?
There was a problem hiding this comment.
(so here we can simply return the name based on this.attachedDatabases if needed)
There was a problem hiding this comment.
Do you have any suggestions which code could trigger this initialization?
For it to work, I need to ensure that all tables will be passed over (Including migration cli etc also)
test/core/column-kinds/create-date-column/create-date-column.ts
Outdated
Show resolved
Hide resolved
test/functional/connection-options-reader/connection-options-reader.ts
Outdated
Show resolved
Hide resolved
| @@ -1124,15 +1148,16 @@ export abstract class AbstractSqliteQueryRunner extends BaseQueryRunner implemen | |||
| */ | |||
| protected createIndexSql(table: Table, index: TableIndex): Query { | |||
| const columns = index.columnNames.map(columnName => `"${columnName}"`).join(", "); | |||
| return new Query(`CREATE ${index.isUnique ? "UNIQUE " : ""}INDEX "${index.name}" ON "${table.name}" (${columns}) ${index.where ? "WHERE " + index.where : ""}`); | |||
| const [database, tableName] = this.splitTablePath(table.name); | |||
| return new Query(`CREATE ${index.isUnique ? "UNIQUE " : ""}INDEX IF NOT EXISTS ${database ? `"${database}".` : ""}${this.escapePath(index.name!)} ON "${tableName}" (${columns}) ${index.where ? "WHERE " + index.where : ""}`); | |||
There was a problem hiding this comment.
IF NOT EXISTS is something new here. Did you have particular problems without it?
There was a problem hiding this comment.
Honestly, I can't remember. I have a feeling it was required though.
Anyways, i have removed it and will re-submit if my testing shows a need
|
Sorry for the delay in updating @pleerock |
|
Let's merge it. |
|
We also need to add documentation regarding to this feature. And please remove yarn stuff in the next PR. Thanks. |
|
That's awesome. I'll add a note to create Docs. I'll be battle-testing this feature soon in my app so will be sure to feed back any improvements. Thank you! |
This is a copy of this closed PR which i am unable to reopen.
I'd prefer to have the other reopened as it already has a review 馃檹