[Chore(deps)]: Upgrades MySQL client to mysql2
#8732
Conversation
Replaces the deprecated `mysql` client with `mysql2` to address compatibility issues and improve performance. This upgrade ensures continued support and leverages the enhanced features of `mysql2`. Modifies database connection configurations and updates related dependencies. Includes a check for Postgres dialect to prevent unnecessary alterations to the schema during migrations. Improves error handling during database migrations. Also, removes unnecessary timezone configuration from the MySQL connection.
WalkthroughThis pull request focuses on updating database-related dependencies and providers across the Gauzy desktop application. The primary changes involve transitioning from the Changes
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/desktop-lib/src/lib/offline/databases/provider-factory.ts (1)
60-60: Consider moving connection creation to a separate method.The direct
require('knex')call could be moved to a reusable method for better maintainability and testing.- const connection = require('knex')(this.config); + const connection = await this.createKnexConnection(); + private async createKnexConnection(): Promise<Knex> { + return require('knex')(this.config); + }packages/desktop-lib/src/lib/offline/databases/migrations/20231122133813_alter-table-interval.ts (1)
5-5: Consider adding dialect check in down migration for consistency.While the
upmigration correctly handles PostgreSQL dialect, thedownmigration should have similar handling to maintain consistency.export async function down(knex: Knex): Promise<void> { + if (ProviderFactory.instance.dialect === 'postgres') return; await knex.schema.alterTable(TABLE_NAME_INTERVALS, (table: Knex.TableBuilder) => { table.dropForeign(['timerId']); }); }packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts (1)
63-66: Replace dynamic require with static import for better maintainability.Using
require('knex')dynamically could be replaced with a static import at the top of the file for better maintainability and type safety.-const connection: Knex = require('knex')({ +const connection: Knex = knex({apps/desktop-timer/src/package.json (1)
162-162: Document migration steps frommysqltomysql2.While
mysql2is mostly API compatible withmysql, there might be breaking changes that need attention. Please:
- Add migration notes to the PR description
- Document any breaking changes and required code modifications
- Consider adding a migration guide to help other developers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
apps/desktop-timer/package.json(1 hunks)apps/desktop-timer/src/package.json(1 hunks)packages/desktop-lib/package.json(1 hunks)packages/desktop-lib/src/lib/offline/databases/migrations/20231122133813_alter-table-interval.ts(1 hunks)packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts(4 hunks)packages/desktop-lib/src/lib/offline/databases/provider-factory.ts(4 hunks)packages/desktop-lib/src/lib/offline/services/user.service.ts(2 hunks)packages/desktop-lib/src/lib/utilities/util.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (10)
packages/desktop-lib/src/lib/offline/databases/provider-factory.ts (3)
10-10: LGTM! Good practice using a centralized logger.The addition of a structured logger from
@gauzy/desktop-coreimproves logging consistency and maintainability compared to using console statements.
61-75: LGTM! Well-structured error handling and logging.The refactoring improves code readability with:
- Clear async/await flow
- Comprehensive error handling
- Guaranteed connection cleanup
- Informative logging messages
122-123: Verify mysql2 compatibility with existing queries.While upgrading to
mysql2is a good practice for better performance and security, please ensure:
- All existing queries are compatible with mysql2
- Connection pooling configurations are adjusted if needed
- Error handling remains compatible with the new client
✅ Verification successful
mysql2 upgrade appears safe to proceed
The codebase analysis shows:
- No incompatible MySQL-specific features
- Database operations primarily use TypeORM's abstraction layer
- Existing MySQL syntax (ON DUPLICATE KEY) is fully supported by mysql2
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MySQL-specific queries or configurations that might need adjustment rg -l 'createPool|connection\.query|connection\.execute' --type ts # Look for potential MySQL error handling rg -l 'SQLError|MySQLError|ER_|connection\.on\(.*error' --type tsLength of output: 11726
Script:
#!/bin/bash # Check for mysql-specific syntax or features rg -A 3 'LOAD_FILE|UTC_TIMESTAMP|GROUP_CONCAT|ON DUPLICATE KEY|STRAIGHT_JOIN' --type ts # Look for TypeORM configuration files fd -e ts -e js "ormconfig|typeorm.config" # Check for direct SQL queries that might need adjustment ast-grep --pattern 'connection.query($$$)'Length of output: 1146
packages/desktop-lib/package.json (1)
50-51: Verify compatibility with latest stable versions.
- mysql2 ^3.12.0 is relatively new. Verify its stability and compatibility with the current Node.js version (>=20.11.1).
- Consider upgrading pg to a newer version as ^8.13.1 is not the latest stable.
✅ Verification successful
Dependencies are at latest stable versions and compatible
Both [email protected] and [email protected] are the latest stable versions available and are fully compatible with the project's Node.js requirement (>=20.11.1). No upgrades needed at this time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest stable versions and their Node.js compatibility npm view mysql2 version npm view mysql2@^3.12.0 engines npm view pg version npm view pg@^8.13.1 enginesLength of output: 169
apps/desktop-timer/package.json (1)
78-78: Verify mysql2 features and maintain version consistency.The mysql2 version (^3.12.0) matches across packages, which is good for consistency. However, verify that all required features from the old mysql package are available in mysql2.
✅ Verification successful
mysql2 implementation verified successfully
The codebase is properly configured to use mysql2 through Knex.js, and the implementation is compatible with mysql2 version ^3.12.0. No legacy mysql package dependencies were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mysql-specific features in the codebase that might need adaptation rg -l "createConnection|createPool|escape|format|raw" --type ts # Check for any remaining mysql package references rg -l "require\('mysql'\)" --type tsLength of output: 27249
Script:
#!/bin/bash # Check mysql-provider implementation cat packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts # Check recent MySQL migration cat packages/core/src/lib/database/migrations/1705138670108-MysqlTablesMigration.tsLength of output: 67691
apps/desktop-timer/src/package.json (2)
162-162: Verify removal of the legacymysqlpackage.The PR objective is to upgrade from
mysqltomysql2, but I don't see the removal of the oldmysqlpackage in this diff.Let's verify if the old package is still present:
✅ Verification successful
Legacy
mysqlpackage has been properly removedThe verification confirms that the old
mysqlpackage is not present in any package.json files, andmysql2has been consistently adopted across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mysql dependency in package.json files rg '"mysql":' -A 1 'package.json$'Length of output: 90
Script:
#!/bin/bash # Find all package.json files and search for mysql related dependencies fd 'package.json$' | xargs rg -A 1 '"mysql[2]?":' --no-line-numberLength of output: 736
162-164: Clarify the scope expansion with PostgreSQL packages.While the PR title only mentions upgrading to
mysql2, I notice the addition of PostgreSQL-related packages (pgandpg-query-stream). Could you please:
- Clarify if adding PostgreSQL support is intentionally part of this PR's scope
- Consider splitting this into a separate PR if it's not directly related to the MySQL upgrade
Also, if PostgreSQL support is intended, the
pgpackage version seems outdated. Let's check for the latest version:✅ Verification successful
PostgreSQL packages are aligned with project's database strategy
Based on the project documentation:
- PostgreSQL is listed as the primary database for production use
- Both MySQL and PostgreSQL are core supported databases
- The
pgpackage version (8.13.1) is up to date🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest pg version on npm curl -s https://registry.npmjs.org/pg/latest | jq '.version'Length of output: 69
Script:
#!/bin/bash # Check if pg was previously in package.json git log -p -- apps/desktop-timer/src/package.json | grep -A 5 -B 5 '"pg":' # Search for PostgreSQL-related files and configurations fd -t f -e json -e js -e ts . apps/desktop-timer/src | xargs rg -l 'postgres|postgresql' -i # Check README or documentation for PostgreSQL mentions fd README -t f | xargs rg 'postgres|postgresql' -iLength of output: 2431
packages/desktop-lib/src/lib/utilities/util.ts (1)
1-8: 🛠️ Refactor suggestionImprove type safety and performance of JSON validation.
The current implementation has several issues:
- Type safety: Blindly casting input to string could lead to runtime errors
- Performance: Full JSON parsing is expensive for validation
- Scope: This change appears unrelated to the PR's objective of upgrading MySQL client
Consider this more robust implementation:
-export function isJSON<T>(input: T): boolean { +export function isJSON(input: unknown): boolean { + if (typeof input !== 'string') { + return false; + } try { - JSON.parse(input as string); + JSON.parse(input); return true; } catch (e) { return false; } }This change seems unrelated to the MySQL client upgrade. Let's verify if there are any MySQL-related changes:
packages/desktop-lib/src/lib/offline/services/user.service.ts (2)
41-43: 🛠️ Refactor suggestionImprove error handling for employee data parsing.
The current implementation silently ignores invalid JSON data for the employee field. This could lead to unexpected behavior downstream.
Consider this more robust implementation:
-if (isJSON(user.employee)) { - user.employee = JSON.parse(user.employee as string); -} +try { + if (typeof user.employee === 'string' && isJSON(user.employee)) { + user.employee = JSON.parse(user.employee); + } else { + console.warn('Employee data is not in JSON format:', typeof user.employee); + } +} catch (error) { + console.error('Failed to parse employee data:', error); +}These changes seem unrelated to the MySQL client upgrade. Let's verify the database-related code:
✅ Verification successful
The error handling improvement suggestion is valid and database-related.
The JSON parsing is necessary for proper deserialization of the
employeecolumn, which is stored as JSON in the database (as seen in migration20230131142022_alter-table-users.ts). The suggested error handling improvements will make the database retrieval process more robust.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for database-related code rg -l 'mysql2?|database|connection' packages/desktop-lib/Length of output: 2158
Script:
#!/bin/bash # Check employee-related database code and JSON handling rg -A 5 'employee' packages/desktop-lib/src/lib/offline/Length of output: 22982
Line range hint
1-43: Verify PR scope and missing MySQL upgrade changes.The changes in these files focus on JSON validation but don't appear to address the stated PR objective of upgrading the MySQL client to mysql2.
Let's verify if the MySQL upgrade changes are missing or in other files:
packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts
Outdated
Show resolved
Hide resolved
|
@adkif did you tested and after such update on your Mac setup complete successfully? I think there were some issues in Desktop apps before related to that MySQL driver v3.x, so we keep it for a while at 2.x in desktop apps... Are you sure it's good now? Tried to build locally apps and install (using installer)? |
|
@evereq I have tested it on a Mac and it seems working now |
|
View your CI Pipeline Execution ↗ for commit 998ee01.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/desktop-lib/src/lib/offline/databases/provider-factory.ts (1)
Line range hint
34-45: Enhance type safety by using DatabaseTypeEnum consistently.While
DatabaseTypeEnum.postgresis used for PostgreSQL, other database types still use string literals. Consider using the enum consistently for all database types to improve type safety and maintainability.- case DatabaseTypeEnum.postgres: + case DatabaseTypeEnum.postgres: this._dbContext.provider = PostgresProvider.instance; break; - case 'mysql': + case DatabaseTypeEnum.mysql: this._dbContext.provider = MysqlProvider.instance; break; - case 'sqlite': + case DatabaseTypeEnum.sqlite: this._dbContext.provider = SqliteProvider.instance; break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
packages/desktop-lib/src/lib/offline/databases/provider-factory.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (2)
packages/desktop-lib/src/lib/offline/databases/provider-factory.ts (2)
61-76: Well-structured migration implementation!Great improvements in the migration method:
- Better error handling with detailed logging
- Proper resource cleanup with connection.destroy()
- Clear success/failure messaging
123-124: Verify mysql2 compatibility with desktop applications.While the upgrade to
mysql2client looks correct, please ensure:
- All required mysql2-specific configuration options are set
- The desktop application builds and installs correctly
- No compatibility issues exist with version 3.x as mentioned in the PR comments
Let's verify the mysql2 usage across the codebase:
✅ Verification successful
mysql2 compatibility verification completed successfully
The upgrade to mysql2 has been properly implemented with:
- Correct configuration in all necessary files
- Compatible v3.x dependencies across packages
- Proper integration with desktop applications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any mysql-specific configuration or connection options that might need updating rg -l 'mysql\.' --type ts # Look for any remaining references to the old mysql package rg -l "'mysql'" --type ts rg -l '"mysql"' --type ts # Check package.json files for mysql dependencies fd package.json --exec grep -l mysql {} \;Length of output: 745
Script:
#!/bin/bash # Check database configuration and mysql2 specific settings rg -A 10 "mysql2" --type ts # Check package.json files for exact mysql dependencies fd package.json --exec cat {} \; | jq -c 'select(.dependencies != null) | {name: .name, deps: .dependencies | with_entries(select(.key | contains("mysql")))} + {devDeps: .devDependencies | with_entries(select(.key | contains("mysql")))}' # Look for any remaining mysql imports rg "from ['\"]mysql['\"]" --type tsLength of output: 8560
...ages/desktop-lib/src/lib/offline/databases/migrations/20231122133813_alter-table-interval.ts
Show resolved
Hide resolved
packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts
Outdated
Show resolved
Hide resolved
Moves the `isJSON` utility function from `desktop-lib` to the shared `@gauzy/utils` package to avoid code duplication and promote reusability. Removes the now redundant `util.ts` file from `desktop-lib`.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/utils/src/lib/is-json.ts (1)
1-8: Consider adding performance optimization for common cases.For better performance, consider adding quick checks before attempting JSON.parse:
export function isJSON(input: unknown): boolean { if (input === null || input === undefined) { return false; } if (typeof input !== 'string') { return false; } + // Quick check for common invalid cases + input = input.trim(); + if (input === '' || + (input[0] !== '{' && input[0] !== '[' && + input[0] !== '"' && input[0] !== "'")) { + return false; + } try { JSON.parse(input); return true; } catch (e) { return false; } }packages/utils/src/index.ts (1)
34-34: Consider maintaining alphabetical order of exports.The new export could be placed in alphabetical order with other
is-*utilities for better maintainability.export * from './lib/is-function'; +export * from './lib/is-json'; export * from './lib/is-js-object'; export * from './lib/is-not-empty'; export * from './lib/is-not-null-or-undefined'; export * from './lib/is-null-or-undefined'; export * from './lib/is-object'; export * from './lib/is-string'; export * from './lib/match'; -export * from './lib/is-json';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/desktop-lib/src/lib/offline/services/user.service.ts(2 hunks)packages/utils/src/index.ts(1 hunks)packages/utils/src/lib/is-json.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-lib/src/lib/offline/services/user.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (1)
packages/utils/src/lib/is-json.ts (1)
1-8: Verify the usage context of this utility.This utility is being added as part of a MySQL client upgrade PR. Let's verify its usage in the database-related code:
✅ Verification successful
The
isJSONutility is appropriately implemented and usedThe utility is used consistently across the codebase for safe JSON parsing, including in database-related code where string-stored JSON needs validation before parsing. The implementation in user.service.ts follows proper defensive programming practices by validating JSON strings before parsing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of isJSON in database-related code echo "Searching for isJSON usage in database files..." rg -l "isJSON" | rg -i "(\bdb\b|database|mysql|connection)" # Search for specific usage mentioned in the summary echo "Searching for specific usage in user.service.ts..." rg "isJSON.*user\.employee" "packages/desktop-lib/src/lib/offline/services/user.service.ts"Length of output: 399
Script:
#!/bin/bash # Get more context around the isJSON usage echo "Context around isJSON usage:" rg "isJSON.*user\.employee" -B 5 -A 5 "packages/desktop-lib/src/lib/offline/services/user.service.ts" # Find all usages of isJSON echo -e "\nAll isJSON usages:" rg "isJSON\(" -A 2 # Look for User interface/type definition echo -e "\nUser interface definition:" ast-grep --pattern 'interface User { $$$ }'Length of output: 1922
Replaces string literals with DatabaseTypeEnum values for database configuration. This change improves type safety and consistency across different database providers.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/desktop-lib/src/lib/offline/databases/postgres-provider.ts (1)
Line range hint
56-68: Improve database creation security and error handling.Several concerns in the database creation logic:
- The raw query is susceptible to SQL injection
- Error handling doesn't distinguish between different PostgreSQL errors
- Connection isn't destroyed in the error case
Apply these improvements:
public async createDatabase() { + let connection: Knex | undefined; try { - const connection: Knex = require('knex')({ + connection = require('knex')({ client: 'pg', connection: this._connectionConfig }); - const res = await connection.select('datname').from('pg_database').where('datname', this._database); + const res = await connection + .select('datname') + .from('pg_database') + .where('datname', connection.raw('?', [this._database])); if (res.length < 1) { - await connection.raw('CREATE DATABASE ??', this._database); + await connection.raw('CREATE DATABASE ??', [this._database]); } await connection.destroy(); } catch (error) { + await connection?.destroy(); + if (error.code === '42P04') { + throw new AppError('POSTGRES', 'Database already exists'); + } throw new AppError('POSTGRES', error); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/desktop-lib/src/lib/offline/databases/better-sqlite-provider.ts(2 hunks)packages/desktop-lib/src/lib/offline/databases/migrations/20231122133813_alter-table-interval.ts(1 hunks)packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts(4 hunks)packages/desktop-lib/src/lib/offline/databases/postgres-provider.ts(3 hunks)packages/desktop-lib/src/lib/offline/databases/provider-factory.ts(4 hunks)packages/desktop-lib/src/lib/offline/databases/sqlite-provider.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts
- packages/desktop-lib/src/lib/offline/databases/migrations/20231122133813_alter-table-interval.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (10)
packages/desktop-lib/src/lib/offline/databases/sqlite-provider.ts (1)
5-5: LGTM! Clean import addition.The addition of
DatabaseTypeEnumimport aligns with the codebase-wide initiative to use enumerated types for database identification.packages/desktop-lib/src/lib/offline/databases/better-sqlite-provider.ts (2)
5-5: LGTM! Good practice using enums.The addition of
DatabaseTypeEnumimport aligns with TypeScript best practices and improves type safety.
29-29: LGTM! Good refactor replacing magic string.Replacing the hardcoded string with
DatabaseTypeEnum.betterSqlite3improves maintainability and type safety while maintaining the same functionality.However, since this PR is primarily about upgrading the MySQL client, let's verify that this SQLite provider isn't affected by any MySQL-related changes:
✅ Verification successful
SQLite provider is independent of MySQL changes
The verification confirms that:
- The SQLite provider has no direct or indirect MySQL dependencies
- The enum refactor is a safe, isolated change
- The presence of mysql2 in package.json is used by other modules and doesn't affect this provider
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify SQLite provider's independence from MySQL changes # Look for any MySQL-related dependencies or imports that might affect this provider # Check if this file has any MySQL-related dependencies rg -l "mysql2?" "packages/desktop-lib/src/lib/offline/databases/better-sqlite-provider.ts" # Check package.json for any MySQL-related dependencies that might affect SQLite fd "package.json" "packages/desktop-lib" --exec jq '.dependencies | keys[] | select(contains("mysql"))'Length of output: 201
Script:
#!/bin/bash # Check for shared interfaces and types between MySQL and SQLite providers echo "=== Checking imports in SQLite provider ===" rg "^import.*from" "packages/desktop-lib/src/lib/offline/databases/better-sqlite-provider.ts" -N echo -e "\n=== Checking for shared interfaces ===" fd "\.ts$" "packages/desktop-lib/src/lib/offline/databases" --exec ast-grep --pattern 'interface $_ { $$$ }' echo -e "\n=== Checking DatabaseTypeEnum definition ===" rg -A 5 "enum DatabaseTypeEnum" "packages/desktop-lib"Length of output: 754
packages/desktop-lib/src/lib/offline/databases/provider-factory.ts (5)
1-3: LGTM! Good improvements in the imports.The new imports enhance type safety with
DatabaseTypeEnumand improve logging practices by using a proper logger instead of console statements.
34-42: LGTM! Good use of enum instead of string literals.Using
DatabaseTypeEnuminstead of string literals improves type safety and maintainability.
61-76: LGTM! Excellent improvements in error handling and resource cleanup.The migration method has been improved with:
- Better async/await syntax
- More descriptive error messages
- Proper connection cleanup in finally block
- Enhanced logging with appropriate log levels
108-108: Restore trailing comma to maintain consistency.Based on the project's conventions, object literals should include trailing commas.
- port: arg[dialect]?.dbPort + port: arg[dialect]?.dbPort,
120-125: Verify mysql2 compatibility with desktop apps.While updating to
mysql2is a good improvement, please ensure compatibility with desktop applications as mentioned in the PR discussion. Previous issues with MySQL driver 3.x affecting desktop apps were noted.Run the following script to check for any existing workarounds or compatibility fixes in the codebase:
✅ Verification successful
mysql2 compatibility with desktop apps is confirmed ✅
The desktop applications are already using mysql2 driver (^3.12.0) in their dependencies, and no compatibility issues were found in the codebase. The change aligns with the existing desktop app configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mysql-related compatibility fixes or workarounds rg -i "mysql.*workaround|mysql.*fix|mysql.*issue" --type ts # Check for any mysql2-specific configurations rg "mysql2.*config" --type ts # Look for desktop-specific database configurations rg "desktop.*database|database.*desktop" --type tsLength of output: 561
Script:
#!/bin/bash # Search for MySQL/mysql2 related imports and configurations rg "from ['|\"]mysql2?['|\"]|require\(['|\"]mysql2?['|\"]\)" --type ts rg "mysql2?.*connection|connection.*mysql2?" --type ts -A 2 # Look for desktop database initialization ast-grep --pattern 'class $_ implements $_ { $$$ initialize($$$) { $$$ } $$$ }' # Check package.json files for mysql dependencies fd package.json --exec cat {} \; | jq -c 'select(.dependencies != null) | {name: .name, deps: .dependencies | with_entries(select(.key | test("mysql")))}' # Search for upgrade/migration docs fd -e md -e txt | xargs rg -i "mysql.*upgrade|mysql.*migration|mysql2.*guide" -lLength of output: 3579
packages/desktop-lib/src/lib/offline/databases/postgres-provider.ts (2)
5-5: LGTM! Good use of type-safe enums.The introduction of
DatabaseTypeEnumimproves type safety and maintainability by replacing hardcoded strings.
Line range hint
30-36: Review database pool configuration for desktop environment.The current pool configuration might be too aggressive for a desktop application:
- Maximum of 15 connections could be excessive
- 2-minute acquire timeout might mask connection issues
Consider adjusting these values based on desktop app requirements.Run this script to check if other providers use similar pool settings:
✅ Verification successful
Pool configuration is appropriate for desktop environment
The current pool settings are well-tuned for a desktop PostgreSQL connection:
- Max 15 connections balances resource usage with concurrent operation needs
- Matches configuration pattern used across other client-server database providers
- Timeout values are consistent with other providers and suitable for desktop app recovery scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for pool configurations across database providers rg -A 10 'pool:\s*{' packages/desktop-lib/src/lib/offline/databases/Length of output: 152
Script:
#!/bin/bash # Search for pool configurations with various patterns rg -U "pool.*=.*{" packages/desktop-lib/src/lib/offline/databases/ -A 10 rg -U "createPool.*{" packages/desktop-lib/src/lib/offline/databases/ -A 10 rg -U "poolConfig.*=.*{" packages/desktop-lib/src/lib/offline/databases/ -A 10Length of output: 503
Script:
#!/bin/bash # Search for pool-related terms rg -l "pool" packages/desktop-lib/src/lib/offline/databases/ # Use ast-grep to find object literal assignments ast-grep --pattern 'const $_ = { $$$ }' # Search for specific pool config properties rg "min:|max:|createTimeout|acquireTimeout" packages/desktop-lib/src/lib/offline/databases/Length of output: 65954
Script:
#!/bin/bash # Examine the postgres provider file cat packages/desktop-lib/src/lib/offline/databases/postgres-provider.ts # Look at other database providers cat packages/desktop-lib/src/lib/offline/databases/sqlite-provider.ts cat packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts cat packages/desktop-lib/src/lib/offline/databases/better-sqlite-provider.tsLength of output: 7155
packages/desktop-lib/src/lib/offline/databases/sqlite-provider.ts
Outdated
Show resolved
Hide resolved
packages/desktop-lib/src/lib/offline/databases/postgres-provider.ts
Outdated
Show resolved
Hide resolved
Ensures DB config exists for MySQL & PostgreSQL Throws an error if the MySQL or PostgreSQL configuration is missing or incomplete in the application config. This ensures that the application fails early if the required database configurations are not provided, preventing unexpected behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts (1)
71-76: Consider enhancing error handling in database creation.While the implementation is good, consider adding more specific error handling:
try { const connection: Knex = require('knex')({ client: this.CLIENT, connection: this._connectionConfig }); await connection.raw('CREATE DATABASE IF NOT EXISTS ??', this._database).finally(async () => { await connection.destroy(); }); } catch (error) { - throw new AppError('MYSQL', error); + throw new AppError( + 'MYSQL', + `Failed to create database ${this._database}: ${error.message}` + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts(4 hunks)packages/desktop-lib/src/lib/offline/databases/postgres-provider.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (10)
packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts (3)
5-5: LGTM! Good improvements to type safety and maintainability.The addition of
DatabaseTypeEnumand theCLIENTconstant improves type safety and makes the client configuration more maintainable.Also applies to: 12-12
22-25: Verify timezone configuration impact on time tracking.While setting timezone to '+00:00' provides consistency, it might affect time tracking features. The codebase uses timezone-aware operations for:
- Time tracking and activity logging
- Database TIMESTAMP/DATETIME columns
- UI components with explicit timezone handling
Let's verify the impact:
#!/bin/bash # Check timezone usage in time tracking features rg -l "timezone|TIMESTAMP|DATETIME|NOW\(\)" --type ts # Check for time tracking related files rg -l "TimeTracking|ActivityLog|timeZone" --type tsAlso applies to: 65-65
53-59: Great improvement in error handling!The new error handling is more robust with:
- Specific validation for required configuration fields
- Clear error messages
- Type-safe config access using DatabaseTypeEnum
packages/desktop-lib/src/lib/offline/databases/postgres-provider.ts (7)
5-5: Good use ofDatabaseTypeEnumfor configurationReplacing hardcoded strings with
DatabaseTypeEnum.postgresenhances maintainability and reduces the risk of typos or errors.
24-24: Database name correctly added to connection configurationIncluding
database: this._databasein the connection settings ensures that Knex connects to the appropriate database.
27-27: Specified migrations directory properlySetting the migrations directory allows Knex to locate and execute migration files correctly.
36-36: Configured pool retry intervalDefining
createRetryIntervalMilliscontrols the retry interval for acquiring new connections, which can help manage database load effectively.
45-51: Good implementation of configuration validationAdding checks for the presence and validity of the PostgreSQL configuration improves error handling and prevents runtime errors due to missing settings.
65-65: Reusing connection configuration consistentlyUsing
this._connectionConfigwhen creating the Knex instance ensures consistency and reduces the likelihood of configuration mismatches.
69-69: Proper use of identifier binding in raw SQL queryUtilizing
??for identifier binding inconnection.raw('CREATE DATABASE ??', this._database);ensures that the database name is safely escaped, preventing SQL injection attacks.
packages/desktop-lib/src/lib/offline/databases/postgres-provider.ts
Outdated
Show resolved
Hide resolved
|
@adkif What about this PR? |
@rahul-rocket It's ready to be merged |
@adkif We can't merge this PR, because we have some issues with "Desktop Builds". |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/desktop-lib/src/lib/utilities/util.ts (1)
1-8: 🛠️ Refactor suggestionConsider enhancing type safety and handling edge cases.
The current implementation has a few potential improvements to consider:
- The type coercion to string could be unsafe for certain types
- The function will return true for primitive JSON values (numbers, booleans)
- As previously suggested, this utility should be moved to
@gauzy/utilspackageConsider this enhanced implementation:
-export function isJSON<T>(input: T): boolean { +export function isJSON<T>(input: T): boolean { + if (typeof input !== 'string') { + return false; + } try { - JSON.parse(input as string); - return true; + const parsed = JSON.parse(input); + return parsed && typeof parsed === 'object'; } catch (e) { return false; } }
🧹 Nitpick comments (2)
packages/desktop-lib/src/lib/offline/databases/postgres-provider.ts (1)
45-50: Improve error message specificity in configuration validation.While the error handling is good, the error messages could be more specific about which fields are missing.
if (!cfg.dbHost || !cfg.dbPort || !cfg.dbUsername || !cfg.dbPassword) { - throw new AppError('POSTGRES', 'Required PostgreSQL configuration fields are missing'); + const missingFields = [ + !cfg.dbHost && 'dbHost', + !cfg.dbPort && 'dbPort', + !cfg.dbUsername && 'dbUsername', + !cfg.dbPassword && 'dbPassword' + ].filter(Boolean); + throw new AppError('POSTGRES', `Missing required PostgreSQL configuration fields: ${missingFields.join(', ')}`); }packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts (1)
53-58: Improve error message specificity in configuration validation.Similar to the PostgreSQL provider, the error messages could be more specific about which fields are missing.
if (!cfg.dbHost || !cfg.dbPort || !cfg.dbUsername || !cfg.dbPassword) { - throw new AppError('MYSQL', 'Required MySQL configuration fields are missing'); + const missingFields = [ + !cfg.dbHost && 'dbHost', + !cfg.dbPort && 'dbPort', + !cfg.dbUsername && 'dbUsername', + !cfg.dbPassword && 'dbPassword' + ].filter(Boolean); + throw new AppError('MYSQL', `Missing required MySQL configuration fields: ${missingFields.join(', ')}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/desktop-lib/package.json(1 hunks)packages/desktop-lib/src/lib/offline/databases/migrations/20231122133813_alter-table-interval.ts(1 hunks)packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts(4 hunks)packages/desktop-lib/src/lib/offline/databases/postgres-provider.ts(1 hunks)packages/desktop-lib/src/lib/offline/databases/provider-factory.ts(4 hunks)packages/desktop-lib/src/lib/offline/services/user.service.ts(2 hunks)packages/desktop-lib/src/lib/utilities/util.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-lib/package.json
- packages/desktop-lib/src/lib/offline/databases/migrations/20231122133813_alter-table-interval.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (9)
packages/desktop-lib/src/lib/offline/databases/postgres-provider.ts (2)
Line range hint
38-38: Consider disabling asyncStackTraces in production.Enabling
asyncStackTracesprovides detailed stack traces for debugging but can significantly impact performance. Consider making it configurable based on the environment.- asyncStackTraces: true, + asyncStackTraces: process.env.NODE_ENV !== 'production',
42-42: Consider making the database name configurable.The database name is still hardcoded as 'gauzy_timer_db'. Consider making it configurable through the application config for better flexibility.
packages/desktop-lib/src/lib/offline/databases/mysql-provider.ts (4)
11-11: Good practice: Centralizing the client identifier.Good job extracting the client identifier into a constant. This makes it easier to maintain and reduces the risk of typos.
Also applies to: 21-21, 71-71
74-75: Improved database creation with proper cleanup.Good improvement in the database creation logic by:
- Using a single raw query with IF NOT EXISTS
- Ensuring connection cleanup with finally block
39-39: Consider disabling asyncStackTraces in production.Enabling
asyncStackTracesprovides detailed stack traces for debugging but can significantly impact performance. Consider making it configurable based on the environment.- asyncStackTraces: true, + asyncStackTraces: process.env.NODE_ENV !== 'production',
64-64: Verify timezone handling with mysql2.The timezone format has been changed from 'utc' to '+00:00'. While both formats are valid, ensure this change is compatible with mysql2's timezone handling, especially for time tracking features.
✅ Verification successful
The timezone format change is compatible with mysql2.
The change from 'utc' to '+00:00' is a valid modification as both formats are supported by mysql2 for UTC timezone configuration. The new format is more explicit and standard in MySQL environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check timezone handling in queries rg -A 2 "CURRENT_TIMESTAMP|NOW\(\)|CONVERT_TZ|timezone" --type tsLength of output: 66573
packages/desktop-lib/src/lib/offline/databases/provider-factory.ts (3)
60-75: Improved error handling in migrate method.The migration error handling has been enhanced with proper logging and cleanup. This is a good improvement!
107-107: Restore trailing commas for consistency.As noted in previous reviews, the project consistently uses trailing commas in object literals.
port: arg[dialect]?.dbPort +,connection +,connection +,Also applies to: 116-116, 123-123
122-123: Verify mysql2 compatibility with existing queries.The upgrade from
mysqltomysql2client looks good, but we should verify compatibility with existing queries.✅ Verification successful
mysql2 upgrade is compatible with existing queries
The codebase primarily uses TypeORM and Knex query builders which abstract the database driver implementation. The few raw SQL queries found use standard SQL syntax and parameterized queries, with no MySQL-specific features that would be incompatible with mysql2.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MySQL-specific query patterns that might need verification rg "SELECT|INSERT|UPDATE|DELETE|CREATE|ALTER" --type ts -C 2Length of output: 75883
Script:
#!/bin/bash # Search for potential SQL query patterns echo "=== Searching for raw SQL queries ===" rg "\.raw\(|knex\.raw|connection\.query|createQuery" --type ts -C 2 echo -e "\n=== Searching for migration files ===" fd "migration|\.sql$" echo -e "\n=== Searching for TypeORM patterns ===" rg "@Entity|@Column|@Repository|typeorm" --type ts -C 2Length of output: 66093
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by CodeRabbit
Dependencies
mysqltomysql2in desktop timer and desktop librarypgpackage) to desktop libraryDatabase Providers
Utilities
isJSONutility function for JSON validation