chore(rethinkdb): QueryMap: One-shot #10005
Conversation
Signed-off-by: Matt Krick <[email protected]>
WalkthroughThis update transitions the database operations for query persistence and retrieval from RethinkDB to PostgreSQL. Key functions and classes now utilise PostgreSQL methods, and a new migration script has been introduced to handle the transition of the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| const pg = new Kysely<any>({ | ||
| dialect: new PostgresDialect({ | ||
| pool: getPg() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Ensure proper resource cleanup for PostgreSQL connection.
The PostgreSQL connection created by Kysely should be properly closed after the migration to avoid potential resource leaks.
- const pg = new Kysely<any>({
- dialect: new PostgresDialect({
- pool: getPg()
- })
- })
+ const pg = new Kysely<any>({
+ dialect: new PostgresDialect({
+ pool: getPg()
+ })
+ })
+ try {
+ // Migration logic here
+ } finally {
+ await pg.destroy()
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pg = new Kysely<any>({ | |
| dialect: new PostgresDialect({ | |
| pool: getPg() | |
| }) | |
| }) | |
| const pg = new Kysely<any>({ | |
| dialect: new PostgresDialect({ | |
| pool: getPg() | |
| }) | |
| }) | |
| try { | |
| // Migration logic here | |
| } finally { | |
| await pg.destroy() | |
| } |
| const queries = await r | ||
| .table('QueryMap') | ||
| .filter((row) => row('createdAt').default(null).ne(null)) | ||
| .run() | ||
| await pg.insertInto('QueryMap').values(queries).execute() |
There was a problem hiding this comment.
Add error handling for data migration.
Currently, there is no error handling for the data migration from RethinkDB to PostgreSQL. Consider wrapping the migration logic in a try-catch block to handle potential errors gracefully.
- const queries = await r
- .table('QueryMap')
- .filter((row) => row('createdAt').default(null).ne(null))
- .run()
- await pg.insertInto('QueryMap').values(queries).execute()
+ try {
+ const queries = await r
+ .table('QueryMap')
+ .filter((row) => row('createdAt').default(null).ne(null))
+ .run()
+ await pg.insertInto('QueryMap').values(queries).execute()
+ } catch (error) {
+ console.error('Failed to migrate data from RethinkDB to PostgreSQL:', error)
+ throw error
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const queries = await r | |
| .table('QueryMap') | |
| .filter((row) => row('createdAt').default(null).ne(null)) | |
| .run() | |
| await pg.insertInto('QueryMap').values(queries).execute() | |
| try { | |
| const queries = await r | |
| .table('QueryMap') | |
| .filter((row) => row('createdAt').default(null).ne(null)) | |
| .run() | |
| await pg.insertInto('QueryMap').values(queries).execute() | |
| } catch (error) { | |
| console.error('Failed to migrate data from RethinkDB to PostgreSQL:', error) | |
| throw error | |
| } |
| const client = new Client(getPgConfig()) | ||
| await client.connect() | ||
| await client.query(` | ||
| DROP TABLE "QueryMap"; | ||
| ` /* Do undo magic */) | ||
| await client.end() |
There was a problem hiding this comment.
Add error handling and proper resource cleanup.
Currently, there is no error handling for the PostgreSQL connection and query execution. Additionally, ensure the client is properly closed in case of errors.
- const client = new Client(getPgConfig())
- await client.connect()
- await client.query(`
- DROP TABLE "QueryMap";
- ` /* Do undo magic */)
- await client.end()
+ const client = new Client(getPgConfig())
+ try {
+ await client.connect()
+ await client.query(`
+ DROP TABLE "QueryMap";
+ ` /* Do undo magic */)
+ } catch (error) {
+ console.error('Failed to drop table "QueryMap":', error)
+ throw error
+ } finally {
+ await client.end()
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const client = new Client(getPgConfig()) | |
| await client.connect() | |
| await client.query(` | |
| DROP TABLE "QueryMap"; | |
| ` /* Do undo magic */) | |
| await client.end() | |
| const client = new Client(getPgConfig()) | |
| try { | |
| await client.connect() | |
| await client.query(` | |
| DROP TABLE "QueryMap"; | |
| ` /* Do undo magic */) | |
| } catch (error) { | |
| console.error('Failed to drop table "QueryMap":', error) | |
| throw error | |
| } finally { | |
| await client.end() | |
| } |
| await sql` | ||
| CREATE TABLE IF NOT EXISTS "QueryMap" ( | ||
| "id" VARCHAR(24) PRIMARY KEY, | ||
| "query" TEXT NOT NULL, | ||
| "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW() | ||
| ) | ||
| `.execute(pg) |
There was a problem hiding this comment.
Improve SQL execution for table creation.
Instead of using raw SQL, consider using Kysely's schema builder for better readability and maintainability.
- await sql`
- CREATE TABLE IF NOT EXISTS "QueryMap" (
- "id" VARCHAR(24) PRIMARY KEY,
- "query" TEXT NOT NULL,
- "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW()
- )
- `.execute(pg)
+ await pg.schema
+ .createTable('QueryMap')
+ .ifNotExists()
+ .addColumn('id', 'varchar(24)', (col) => col.primaryKey())
+ .addColumn('query', 'text', (col) => col.notNull())
+ .addColumn('createdAt', 'timestamp', (col) => col.notNull().defaultTo(pg.fn.now()))
+ .execute()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await sql` | |
| CREATE TABLE IF NOT EXISTS "QueryMap" ( | |
| "id" VARCHAR(24) PRIMARY KEY, | |
| "query" TEXT NOT NULL, | |
| "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW() | |
| ) | |
| `.execute(pg) | |
| await pg.schema | |
| .createTable('QueryMap') | |
| .ifNotExists() | |
| .addColumn('id', 'varchar(24)', (col) => col.primaryKey()) | |
| .addColumn('query', 'text', (col) => col.notNull()) | |
| .addColumn('createdAt', 'timestamp', (col) => col.notNull().defaultTo(pg.fn.now())) | |
| .execute() |
| import getPgConfig from '../getPgConfig' | ||
|
|
||
| export async function up() { | ||
| await connectRethinkDB() |
There was a problem hiding this comment.
Add error handling for RethinkDB connection.
Currently, there is no error handling for the RethinkDB connection. Consider wrapping the connection logic in a try-catch block to handle potential connection failures gracefully.
- await connectRethinkDB()
+ try {
+ await connectRethinkDB()
+ } catch (error) {
+ console.error('Failed to connect to RethinkDB:', error)
+ throw error
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await connectRethinkDB() | |
| try { | |
| await connectRethinkDB() | |
| } catch (error) { | |
| console.error('Failed to connect to RethinkDB:', error) | |
| throw error | |
| } |
| .select('query') | ||
| .where('id', '=', docId) | ||
| .executeTakeFirst() | ||
| let queryString = queryStringRes?.query |
There was a problem hiding this comment.
Ensure queryStringRes is defined before accessing its properties.
Add a check to ensure queryStringRes is defined before accessing its query property to avoid potential runtime errors.
- let queryString = queryStringRes?.query
+ let queryString = queryStringRes ? queryStringRes.query : undefinedCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let queryString = queryStringRes?.query | |
| let queryString = queryStringRes ? queryStringRes.query : undefined |
| const pg = getKysely() | ||
| const queryStringRes = await pg | ||
| .selectFrom('QueryMap') | ||
| .select('query') | ||
| .where('id', '=', docId) | ||
| .executeTakeFirst() |
There was a problem hiding this comment.
Add error handling for PostgreSQL query execution.
Currently, there is no error handling for the PostgreSQL query execution. Consider wrapping the query logic in a try-catch block to handle potential errors gracefully.
- const pg = getKysely()
- const queryStringRes = await pg
- .selectFrom('QueryMap')
- .select('query')
- .where('id', '=', docId)
- .executeTakeFirst()
+ const pg = getKysely()
+ let queryStringRes
+ try {
+ queryStringRes = await pg
+ .selectFrom('QueryMap')
+ .select('query')
+ .where('id', '=', docId)
+ .executeTakeFirst()
+ } catch (error) {
+ console.error('Failed to retrieve query string from PostgreSQL:', error)
+ throw error
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pg = getKysely() | |
| const queryStringRes = await pg | |
| .selectFrom('QueryMap') | |
| .select('query') | |
| .where('id', '=', docId) | |
| .executeTakeFirst() | |
| const pg = getKysely() | |
| let queryStringRes | |
| try { | |
| queryStringRes = await pg | |
| .selectFrom('QueryMap') | |
| .select('query') | |
| .where('id', '=', docId) | |
| .executeTakeFirst() | |
| } catch (error) { | |
| console.error('Failed to retrieve query string from PostgreSQL:', error) | |
| throw error | |
| } |
| @@ -41,6 +37,7 @@ const preDeploy = async () => { | |||
|
|
|||
| // The we can prime the DB & CDN | |||
| await Promise.all([storePersistedQueries(), primeIntegrations(), pushToCDN()]) | |||
| await getKysely().destroy() | |||
There was a problem hiding this comment.
Ensure proper resource cleanup for PostgreSQL connection.
The PostgreSQL connection created by Kysely should be properly closed after the pre-deployment tasks to avoid potential resource leaks.
- await getKysely().destroy()
+ try {
+ await getKysely().destroy()
+ } catch (error) {
+ console.error('Failed to destroy PostgreSQL connection:', error)
+ throw error
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await getKysely().destroy() | |
| try { | |
| await getKysely().destroy() | |
| } catch (error) { | |
| console.error('Failed to destroy PostgreSQL connection:', error) | |
| throw error | |
| } |
| const pg = getKysely() | ||
| const res = await pg.insertInto('QueryMap').values(records).onConflict((oc) => oc.doNothing()).returning('id').execute() | ||
| console.log(`🔗 QueryMap Persistence Complete: ${res.length} records added`) |
There was a problem hiding this comment.
Add error handling for PostgreSQL query execution.
Currently, there is no error handling for the PostgreSQL query execution. Consider wrapping the query logic in a try-catch block to handle potential errors gracefully.
- const pg = getKysely()
- const res = await pg.insertInto('QueryMap').values(records).onConflict((oc) => oc.doNothing()).returning('id').execute()
- console.log(`🔗 QueryMap Persistence Complete: ${res.length} records added`)
+ const pg = getKysely()
+ try {
+ const res = await pg.insertInto('QueryMap').values(records).onConflict((oc) => oc.doNothing()).returning('id').execute()
+ console.log(`🔗 QueryMap Persistence Complete: ${res.length} records added`)
+ } catch (error) {
+ console.error('Failed to persist queries to PostgreSQL:', error)
+ throw error
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pg = getKysely() | |
| const res = await pg.insertInto('QueryMap').values(records).onConflict((oc) => oc.doNothing()).returning('id').execute() | |
| console.log(`🔗 QueryMap Persistence Complete: ${res.length} records added`) | |
| const pg = getKysely() | |
| try { | |
| const res = await pg.insertInto('QueryMap').values(records).onConflict((oc) => oc.doNothing()).returning('id').execute() | |
| console.log(`🔗 QueryMap Persistence Complete: ${res.length} records added`) | |
| } catch (error) { | |
| console.error('Failed to persist queries to PostgreSQL:', error) | |
| throw error | |
| } |
Description
moves QueryMap to PG
Summary by CodeRabbit
Refactor
Chores
Performance Improvements