fix: resolve column name handling for postgres #437
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes in this pull request introduce a new utility function, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExportsController
participant ProjectStatsController
participant AliasHelper
Client->>ExportsController: Request export
ExportsController->>AliasHelper: resolveColumnName(projectLocaleId)
AliasHelper-->>ExportsController: Returns resolved column name
ExportsController->>Database: Execute export query with resolved names
Database-->>ExportsController: Returns export data
ExportsController-->>Client: Sends exported data
Client->>ProjectStatsController: Request project stats
ProjectStatsController->>AliasHelper: resolveColumnName(projectLocale)
AliasHelper-->>ProjectStatsController: Returns resolved column name
ProjectStatsController->>Database: Execute stats query with resolved names
Database-->>ProjectStatsController: Returns stats data
ProjectStatsController-->>Client: Sends project stats data
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
dnkodi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
api/src/utils/alias-helper.ts (1)
4-6: Good documentation and clean implementation.The code is well-documented with a clear reference to the TypeORM issue. The utility function provides a clean abstraction that can be easily removed once TypeORM fixes the underlying issue.
Consider caching the converted names for frequently accessed columns to improve performance:
+const columnNameCache = new Map<string, string>(); + export const resolveColumnName = (columnName: string): string => { + const cached = columnNameCache.get(columnName); + if (cached) return cached; + // Convert the column name to snake_case if needed if (config.db.default.type === 'postgres') { - return snakeCase(columnName); + const converted = snakeCase(columnName); + columnNameCache.set(columnName, converted); + return converted; } + columnNameCache.set(columnName, columnName); return columnName; }api/src/controllers/project-stats.controller.ts (1)
49-53: Consider enhancing type safety and error handlingWhile the current implementation works, consider these improvements:
- Use TypeORM's metadata to validate column names at runtime
- Add error handling for cases where database type detection fails
- Consider creating constants for frequently used column names
Example enhancement:
// Create constants for column names const COLUMN_NAMES = { LOCALE_CODE: 'localeCode', PROJECT_LOCALE: 'projectLocale' } as const; // Use constants in query .createQueryBuilder(resolveColumnName(COLUMN_NAMES.PROJECT_LOCALE)) .leftJoin(`${resolveColumnName(COLUMN_NAMES.PROJECT_LOCALE)}.translations`, 'translations')api/src/controllers/exports.controller.ts (1)
117-120: Consider extracting duplicate query builder logic.While the column name resolution is correctly implemented, there's duplicate query building logic between the main locale and fallback locale sections. Consider extracting this into a reusable method.
+ private buildLocaleQuery(projectId: string, projectLocaleId: string) { + return this.termRepo + .createQueryBuilder('term') + .leftJoinAndSelect( + 'term.translations', + 'translation', + `translation.${resolveColumnName('projectLocaleId')} = :projectLocaleId`, + { projectLocaleId } + ) + .where(`term.${resolveColumnName('projectId')} = :projectId`, { projectId }) + .orderBy('term.value', 'ASC'); + }Then use it in both places:
// Main locale const queryBuilder = this.buildLocaleQuery(projectId, projectLocale.id); if (query.untranslated) { queryBuilder.andWhere("translation.value = ''"); } // Fallback locale const fallbackTermsWithTranslations = await this.buildLocaleQuery( projectId, fallbackProjectLocale.id ).getMany();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
api/src/controllers/exports.controller.ts(4 hunks)api/src/controllers/project-stats.controller.ts(2 hunks)api/src/utils/alias-helper.ts(1 hunks)
🔇 Additional comments (4)
api/src/utils/alias-helper.ts (1)
1-2: LGTM! Appropriate use of TypeORM's built-in utility.
Good choice using TypeORM's snakeCase utility instead of implementing custom string conversion.
api/src/controllers/project-stats.controller.ts (2)
15-15: LGTM: Import of resolveColumnName utility
The import is correctly placed and properly utilized in the file.
49-53: LGTM: Query builder modifications for PostgreSQL compatibility
The changes appropriately use resolveColumnName to handle camelCase to snake_case conversion for PostgreSQL compatibility. The implementation is consistent across all query builder methods.
Let's verify the implementation:
api/src/controllers/exports.controller.ts (1)
74-77: LGTM! Column name resolution implemented correctly.
The dynamic column name resolution is properly implemented for both the join condition and where clause, which should resolve the PostgreSQL camelCase alias issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
api/src/utils/alias-helper.ts (1)
4-6: Add JSDoc documentation for better maintainability.While the comment explains the issue, adding JSDoc documentation would improve maintainability by clearly documenting the function's purpose, parameters, and return value.
// TypeORM fails to properly quote camelCase aliases with PostgreSQL // https://github.com/typeorm/typeorm/issues/10961 +/** + * Resolves column names to be compatible with the current database type. + * For PostgreSQL, converts camelCase to snake_case as a workaround for TypeORM's quoting issue. + * @param columnName - The original column name + * @returns The resolved column name appropriate for the current database + * @throws {Error} When columnName is empty + */ export const resolveColumnName = (columnName: string): string => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
api/src/controllers/exports.controller.ts(3 hunks)api/src/utils/alias-helper.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/controllers/exports.controller.ts
🔇 Additional comments (3)
api/src/utils/alias-helper.ts (3)
1-2: LGTM! Appropriate imports used.
The imports are minimal and necessary, using TypeORM's built-in string utilities appropriately.
7-9: Input validation implemented correctly.
The suggested input validation has been properly implemented.
11-17: Add type safety for database configuration.
While the implementation is correct, consider adding type safety for the database configuration to prevent potential runtime errors.
+type SupportedDatabase = 'postgres' | 'mysql' | 'sqlite';
+
export const resolveColumnName = (columnName: string): string => {
if (!columnName) {
throw new Error('Column name cannot be empty');
}
// convert only for postgres until typeorm has a fix
- if (config.db.default.type === 'postgres') {
+ const dbType = config.db.default?.type as SupportedDatabase;
+ if (!dbType) {
+ throw new Error('Database type not configured');
+ }
+
+ if (dbType === 'postgres') {
return snakeCase(columnName);
}
return columnName;
};Let's verify the database configuration usage across the codebase:
This PR addresses an issue raised in #390 , where TypeORM fails to properly quote camelCase aliases in PostgreSQL, leading to errors when querying project statistics and exports endpoint. TypeORM has several open issues, including (typeorm/typeorm#10961), related to camelCase alias handling in PostgreSQL that remain unresolved.
As a temporary workaround, this PR replaces camelCase aliases with snake_case until TypeORM provides a permanent fix.
Summary by CodeRabbit
Summary by CodeRabbit