fix (nocodb): multi user replace for order #11370
Conversation
📝 WalkthroughWalkthroughThe changes introduce database-specific helper functions to generate SQL expressions for replacing delimited user-related columns with mapped values during group-by operations. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant groupBy
participant PG_Helper as replaceDelimitedWithKeyValuePg
participant SQLITE_Helper as replaceDelimitedWithKeyValueSqlite3
participant DB
Client->>groupBy: Request group-by with user-related sorting
groupBy->>groupBy: Detect database client type
alt PostgreSQL
groupBy->>PG_Helper: Build replacement SQL expression
else SQLite3
groupBy->>SQLITE_Helper: Build replacement SQL expression
else Other DB
groupBy->>groupBy: Use original nested REPLACE logic
end
groupBy->>DB: Execute group-by query with constructed SQL
DB-->>groupBy: Return results
groupBy-->>Client: Return grouped results
Possibly related PRs
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nocodb/src/db/BaseModelSqlv2/group-by.ts (1)
1254-1261: Consider refactoring this part to use the new helper functions.For consistency, this code block in the
bulkListfunction should also be updated to use the new database-specific helper functions similar to what was done in thegroupByfunction. This would ensure consistent behavior and maintainability across the codebase.- // create nested replace statement for each user - const finalStatement = baseUsers.reduce((acc, user) => { - const qb = baseModel.dbDriver.raw(`REPLACE(${acc}, ?, ?)`, [ - user.id, - user.display_name || user.email, - ]); - return qb.toQuery(); - }, baseModel.dbDriver.raw(`??`, [columnName]).toQuery()); + let finalStatement = ''; + if (baseModel.dbDriver.clientType() === 'pg') { + finalStatement = `(${replaceDelimitedWithKeyValuePg({ + knex: baseModel.dbDriver, + needleColumn: columnName, + stack: baseUsers.map((user) => ({ + key: user.id, + value: user.display_name || user.email, + })), + })})`; + } else if (baseModel.dbDriver.clientType() === 'sqlite3') { + finalStatement = `(${replaceDelimitedWithKeyValueSqlite3({ + knex: baseModel.dbDriver, + needleColumn: columnName, + stack: baseUsers.map((user) => ({ + key: user.id, + value: user.display_name || user.email, + })), + })})`; + } else { + // use the original replace + finalStatement = baseUsers.reduce((acc, user) => { + const qb = baseModel.dbDriver.raw(`REPLACE(${acc}, ?, ?)`, [ + user.id, + user.display_name || user.email, + ]); + return qb.toQuery(); + }, baseModel.dbDriver.raw(`??`, [columnName]).toQuery()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nocodb/src/db/BaseModelSqlv2/group-by.ts(3 hunks)packages/nocodb/src/db/aggregations/pg.ts(2 hunks)packages/nocodb/src/db/aggregations/sqlite3.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/nocodb/src/db/BaseModelSqlv2/group-by.ts (2)
packages/nocodb/src/db/aggregations/pg.ts (1)
replaceDelimitedWithKeyValuePg(413-453)packages/nocodb/src/db/aggregations/sqlite3.ts (1)
replaceDelimitedWithKeyValueSqlite3(426-466)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
🔇 Additional comments (6)
packages/nocodb/src/db/BaseModelSqlv2/group-by.ts (2)
6-7: LGTM: Good addition of database-specific helper imports.The two imports for database-specific replacements will make the code more maintainable by separating database-specific logic into appropriate files.
Also applies to: 20-20
306-334: Good database-specific SQL generation approach.This refactoring improves maintainability by using database-specific helpers for replacing user IDs with names. The code now cleanly handles:
- PostgreSQL-specific implementation using
string_to_arrayandunnest- SQLite3-specific implementation using
json_each- Fallback to nested
REPLACEcalls for other databasesThe solution is more modular and easier to maintain than the previous approach.
packages/nocodb/src/db/aggregations/pg.ts (2)
11-11: LGTM: Appropriate type import.The addition of the
CustomKnextype import is needed for the new function parameter typing.
413-453: Well-implemented PostgreSQL-specific replacer function.This function effectively handles replacing delimited user IDs with display names specifically for PostgreSQL. The implementation:
- Creates a key-value map through a UNION ALL query
- Splits the delimited input column using PostgreSQL's
string_to_arrayandunnest- Performs a left join to map keys to values
- Aggregates results back with
string_agg- Properly uses parameter binding for SQL injection prevention
The function is well-structured and uses PostgreSQL's features effectively.
packages/nocodb/src/db/aggregations/sqlite3.ts (2)
14-14: LGTM: Appropriate type import.The addition of the
CustomKnextype import is needed for the new function parameter typing.
426-466: Well-implemented SQLite3-specific replacer function.This function effectively handles replacing delimited user IDs with display names specifically for SQLite3. The implementation:
- Creates a key-value map through a UNION ALL query
- Cleverly splits the delimited input using SQLite's
json_eachby first converting the delimited string to a JSON array- Performs a left join to map keys to values
- Aggregates results back with
GROUP_CONCAT- Properly uses parameter binding for SQL injection prevention
The approach is elegant considering SQLite3's more limited string manipulation capabilities compared to PostgreSQL.
|
the unit test error is a fluke. Tried locally. Will monitor if keeps happening. |
Change Summary
fix (nocodb): multi user replace for order
Change type