feat(replace-operation): ensure column is string-like before replace #170
Conversation
- Checks if the column type is string-like (VARCHAR, TEXT, BLOB) - Converts the column to VARCHAR if it's not a string-like type - This ensures the replace operation works correctly for non-string columns feat(project-store): Add pagination state and refresh current page - Adds current offset and limit to the project store - Adds a `refreshCurrentPage` action to fetch the current page again - This allows the UI to easily refresh the current page without losing pagination state fix(replace-dialog): Handle replace operation errors properly - Emits the `replace-completed` event with the affected rows count - Displays a generic error message if the replace operation fails - Removes unnecessary check for `affectedRows` being undefined or null
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughBackend now ensures target columns are string-like (converting and rolling back if needed) before running replace; new helpers and tests validate behavior across datatypes. Frontend emits a payload-less replaceCompleted event, shows a warning when zero rows affected, and refreshes the current page using preserved pagination state. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Service as ReplaceOperationService
participant DB as Database
Service->>DB: PRAGMA table_info(table)
Service->>Service: getColumnType / isStringLikeType
alt Column not string-like
Service->>DB: ALTER TABLE ... ALTER COLUMN column TYPE VARCHAR
end
Service->>DB: UPDATE table SET column = REPLACE(column, find, replace)
DB-->>Service: affectedRows
alt affectedRows == 0 and conversion happened
Service->>DB: ALTER TABLE ... ALTER COLUMN column TYPE <originalType> %% rollback
end
Service-->>Caller: affectedRows
sequenceDiagram
autonumber
participant User as User
participant Dialog as ReplaceDialog.vue
participant Menu as ColumnHeaderMenu.vue
participant Panel as DataTabPanel.vue
participant Store as project.store
participant API as Backend API
User->>Dialog: Submit replace
Dialog->>API: call replace endpoint
API-->>Dialog: { affectedRows }
Dialog->>Menu: emit('replaceCompleted') %% no payload
Menu->>Panel: @replace-completed
Panel->>Store: refreshCurrentPage(projectId)
Store->>API: fetchProject(projectId, currentOffset, currentLimit)
API-->>Store: page data
Store-->>Panel: updated page
Panel-->>User: refreshed table view
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/services/replace-operation.service.ts(2 hunks)backend/tests/services/replace-operation.alter-table.service.test.ts(1 hunks)frontend/src/features/data-processing/components/ColumnHeaderMenu.vue(1 hunks)frontend/src/features/data-processing/components/DataTabPanel.vue(2 hunks)frontend/src/features/data-processing/components/ReplaceDialog.vue(5 hunks)frontend/src/features/project-management/stores/project.store.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/services/replace-operation.alter-table.service.test.ts (2)
backend/src/plugins/database.ts (3)
initializeDb(8-48)closeDb(57-63)getDb(50-55)backend/src/services/replace-operation.service.ts (1)
ReplaceOperationService(12-202)
| // Check if column is string-like, if not, convert it first | ||
| await this.ensureColumnIsStringType(table, column) |
There was a problem hiding this comment.
Avoid mutating schema on no-op replace
Calling ensureColumnIsStringType before we know whether anything matches permanently converts, say, an INTEGER column to VARCHAR even when affectedRows turns out to be 0. A “find” typo now silently changes the schema without touching data, which is a correctness regression. Please defer the ALTER until after you’ve confirmed there are matches (e.g., fetch the current type, count using CAST(column AS VARCHAR) when needed, bail out early on zero hits, then run the ALTER/UPDATE). That way we preserve the original type on no-ops and still satisfy the replace flow when a change is actually required.
- // Check if column is string-like, if not, convert it first
- await this.ensureColumnIsStringType(table, column)
-
- // Count rows that will be affected before the update
- const affectedRows = await this.countAffectedRows(table, column, find, caseSensitive, wholeWord)
-
- // Only proceed if there are rows to update
- if (affectedRows === 0) {
- return 0
- }
+ const columnType = await this.getColumnType(table, column)
+ const isStringColumn = this.isStringLikeType(columnType)
+
+ const affectedRows = await this.countAffectedRows(
+ table,
+ column,
+ find,
+ caseSensitive,
+ wholeWord,
+ isStringColumn,
+ )
+ if (affectedRows === 0) {
+ return 0
+ }
+ if (!isStringColumn) {
+ await this.ensureColumnIsStringType(table, column)
+ }(Adjust countAffectedRows to accept the extra flag and CAST the column when it’s not already string-like.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/src/services/replace-operation.service.ts around lines 21-22, calling
ensureColumnIsStringType before checking for matches mutates the schema on no-op
replaces; change flow to first detect whether column is string-like, then call
countAffectedRows with a new flag (e.g., castToStringWhenNeeded) so the count
query uses CAST(column AS VARCHAR) only for counting when column is not
string-like; if count is zero bail out without altering schema, otherwise run
ensureColumnIsStringType then perform the UPDATE. Also update countAffectedRows
signature to accept the flag and perform the CAST in its SQL when flagged.
| @replace-completed="() => refreshCurrentPage(projectId)" | ||
| /> |
There was a problem hiding this comment.
Pass the project ID string, not the Ref, to refreshCurrentPage.
The current handler invokes refreshCurrentPage(projectId), but projectId here is a Ref<string>. When the event fires, the store receives [object Object] as the ID, so the subsequent API call uses an invalid project identifier. Forward the emitted payload (already a string) or use projectId.value.
Apply this diff:
- @replace-completed="() => refreshCurrentPage(projectId)"
+ @replace-completed="(pid) => refreshCurrentPage(pid)"📝 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.
| @replace-completed="() => refreshCurrentPage(projectId)" | |
| /> | |
| @replace-completed="(pid) => refreshCurrentPage(pid)" | |
| /> |
🤖 Prompt for AI Agents
In frontend/src/features/data-processing/components/DataTabPanel.vue around
lines 66 to 67, the event handler is passing the Ref object `projectId` to
refreshCurrentPage which results in "[object Object]" being used as the project
ID; change the handler to pass a plain string by either forwarding the emitted
payload (e.g. use the event argument passed to the handler) or dereferencing the
Ref (use projectId.value) so refreshCurrentPage receives a string ID.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/src/services/replace-operation.service.ts (2)
116-123: Teach countAffectedRows to CAST when needed.Add a flag to count via CAST(column AS VARCHAR) instead of altering the schema to count matches. Used by the refactored flow.
- private async countAffectedRows( + private async countAffectedRows( table: string, column: string, find: string, caseSensitive: boolean, wholeWord: boolean, - ): Promise<number> { + castToVarchar: boolean = false, + ): Promise<number> { @@ - if (wholeWord) { + const colExpr = castToVarchar ? `CAST("${column}" AS VARCHAR)` : `"${column}"` + if (wholeWord) { @@ - WHERE "${column}" IS NOT NULL - AND regexp_matches("${column}", $1, $2) + WHERE ${colExpr} IS NOT NULL + AND regexp_matches(${colExpr}, $1, $2) @@ - WHERE "${column}" IS NOT NULL - AND position($1 in "${column}") > 0 + WHERE ${colExpr} IS NOT NULL + AND position($1 in ${colExpr}) > 0 @@ - WHERE "${column}" IS NOT NULL - AND regexp_matches("${column}", $1, 'i') + WHERE ${colExpr} IS NOT NULL + AND regexp_matches(${colExpr}, $1, 'i')Also applies to: 126-153, 155-159
67-82: Inline regex flags as literals
DuckDB enforces that regex option flags be constant literals, so removereplaceFlagsfrom the bind parameters and inline bothreplaceFlagsandmatchFlagsin the SQL. For example:const replaceFlags = caseSensitive ? 'g' : 'gi' const matchFlags = caseSensitive ? '' : 'i' const pattern = `\\b${this.escapeRegex(find)}\\b` const query = ` UPDATE "${table}" SET "${column}" = regexp_replace("${column}", $1, $2, '${replaceFlags}') WHERE "${column}" IS NOT NULL AND regexp_matches("${column}", '${pattern}', '${matchFlags}') ` return { query, params: [pattern, replace] }Applies also to lines 126–136 and 145–152.
🧹 Nitpick comments (1)
backend/tests/services/replace-operation.alter-table.service.test.ts (1)
14-36: Strong coverage across types; add BLOB and unique test names.
- Coverage is solid (INTEGER/DOUBLE/BOOLEAN/DATE/VARCHAR/JSON).
- Two cases have the same name "JSON"; make them unique for clearer output.
- Please add a BLOB case to catch the string-like misclassification and ensure conversion.
Minimal rename diff for the second JSON block:
- { - name: 'JSON', + { + name: 'JSON (substring)',Example BLOB test case snippet:
{ name: 'BLOB', tableSql: ` CREATE TABLE test (id INTEGER, payload BLOB); `, insertSql: [ `INSERT INTO test (id, payload) VALUES (1, hex('48656c6c6f'))`, -- "Hello" `INSERT INTO test (id, payload) VALUES (2, hex('776f726c64'))`, -- "world" ], columnName: 'payload', initialType: 'BLOB', find: 'o', replace: '0', expectedAffectedRows: 2, expectedResults: ['Hell0', 'w0rld'], }Also applies to: 37-56, 58-77, 79-98, 100-118, 120-143, 145-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/src/services/replace-operation.service.ts(2 hunks)backend/tests/services/replace-operation.alter-table.service.test.ts(1 hunks)frontend/src/features/data-processing/components/ColumnHeaderMenu.vue(1 hunks)frontend/src/features/data-processing/components/ReplaceDialog.vue(5 hunks)frontend/src/shared/composables/useErrorHandling.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/services/replace-operation.alter-table.service.test.ts (2)
backend/src/plugins/database.ts (3)
initializeDb(8-48)closeDb(57-63)getDb(50-55)backend/src/services/replace-operation.service.ts (1)
ReplaceOperationService(12-221)
🔇 Additional comments (5)
frontend/src/features/data-processing/components/ReplaceDialog.vue (1)
46-47: Thanks for stringifying caught errors.This keeps the toast readable by surfacing the actual error message instead of
[object Object]. Nice cleanup.backend/tests/services/replace-operation.alter-table.service.test.ts (3)
5-12: Good DB lifecycle management.Per-test initialize/close guards state leakage across cases.
284-300: Type assertions look right; reverted vs. persisted conversions covered.Asserting reverted type on 0 matches and VARCHAR on mutations matches the contract.
Consider adding an assertion that other columns’ types remain unchanged to catch unintended ripple effects.
301-305: Data validation is deterministic.ORDER BY id ensures stable comparisons; good call.
backend/src/services/replace-operation.service.ts (1)
21-36: Defer ALTER; make the whole replace atomic.Altering the column before counting mutates schema even for no-ops and, without a transaction, an exception between ALTER and UPDATE can leave the table in a wrong type. Count using CAST (no schema change), bail on 0, then ALTER+UPDATE within a transaction. This eliminates double-ALTER churn and closes the error-window.
Apply this diff:
@@ - // Get the original column type before any modifications - const originalColumnType = await this.getColumnType(table, column) - - // Check if column is string-like, if not, convert it first - const wasConverted = await this.ensureColumnIsStringType(table, column) - - // Count rows that will be affected before the update - const affectedRows = await this.countAffectedRows(table, column, find, caseSensitive, wholeWord) - - // Only proceed if there are rows to update - if (affectedRows === 0) { - // Revert column type if it was converted and no rows were affected - if (wasConverted) { - await this.changeColumnType(table, column, originalColumnType) - } - return 0 - } + const originalColumnType = await this.getColumnType(table, column) + const isStringColumn = this.isStringLikeType(originalColumnType) + // Count without mutating schema; cast when needed + const affectedRows = await this.countAffectedRows( + table, + column, + find, + caseSensitive, + wholeWord, + /*castToVarchar*/ !isStringColumn, + ) + if (affectedRows === 0) { + return 0 + } + await this.db.run('BEGIN') + try { + if (!isStringColumn) { + await this.changeColumnType(table, column, 'VARCHAR') + } @@ - await this.db.run(query, queryParams) - - return affectedRows + await this.db.run(query, queryParams) + await this.db.run('COMMIT') + } catch (err) { + await this.db.run('ROLLBACK') + throw err + } + return affectedRowsAlso applies to: 39-51
| private async getColumnType(table: string, column: string): Promise<string> { | ||
| const result = await this.db.runAndReadAll(`PRAGMA table_info("${table}")`) | ||
| const columns = result.getRowObjectsJson() as Array<{ | ||
| cid: number | ||
| name: string | ||
| type: string | ||
| pk: boolean | ||
| notnull: boolean | ||
| dflt_value: string | null | ||
| }> | ||
|
|
||
| const columnInfo = columns.find((col) => col.name === column) | ||
| if (!columnInfo) { | ||
| throw new Error(`Column '${column}' not found in table '${table}'`) | ||
| } | ||
|
|
||
| return columnInfo.type.toUpperCase() | ||
| } |
There was a problem hiding this comment.
Escape identifiers to prevent SQL injection via table/column names.
Directly interpolating table/column without escaping can break queries or allow injection if names contain quotes. Double-quote and escape inner quotes consistently.
Add helper and use everywhere identifiers are interpolated:
// helper
private q(id: string): string {
return `"${id.replace(/"/g, '""')}"`
}Then replace occurrences of "${table}"/"${column}" with ${this.q(table)} / ${this.q(column)} across queries.
Also applies to: 199-220
🤖 Prompt for AI Agents
In backend/src/services/replace-operation.service.ts around lines 171 to 188
(and also update usages at 199-220), identifiers are interpolated directly into
PRAGMA/SQL which can break queries or allow injection; add a private helper
q(id: string): string that returns the identifier wrapped in double-quotes with
inner double-quotes escaped by doubling, then replace occurrences like
`"${table}"` and `"${column}"` with `${this.q(table)}` / `${this.q(column)}` in
all SQL/PRAGMA string interpolations (including the areas flagged at 199-220) so
table and column names are safely escaped before use.
| private isStringLikeType(columnType: string): boolean { | ||
| const stringTypes = ['VARCHAR', 'TEXT', 'BLOB', 'CHAR', 'BPCHAR'] | ||
|
|
||
| return stringTypes.some((type) => columnType.includes(type)) | ||
| } |
There was a problem hiding this comment.
BLOB should not be treated as string-like.
Regex/replace on BLOB is invalid; letting it bypass conversion can error. Remove BLOB from string-like.
- const stringTypes = ['VARCHAR', 'TEXT', 'BLOB', 'CHAR', 'BPCHAR']
+ const stringTypes = ['VARCHAR', 'TEXT', 'CHAR', 'BPCHAR']Also add a BLOB test to ensure the column is converted before replace (see test suggestion below).
📝 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.
| private isStringLikeType(columnType: string): boolean { | |
| const stringTypes = ['VARCHAR', 'TEXT', 'BLOB', 'CHAR', 'BPCHAR'] | |
| return stringTypes.some((type) => columnType.includes(type)) | |
| } | |
| private isStringLikeType(columnType: string): boolean { | |
| const stringTypes = ['VARCHAR', 'TEXT', 'CHAR', 'BPCHAR'] | |
| return stringTypes.some((type) => columnType.includes(type)) | |
| } |
DaxServer
deleted the
replace-operation-ensure-column-is-string-like-before-replace
branch
feat(project-store): Add pagination state and refresh current page
refreshCurrentPageaction to fetch the current page againfix(replace-dialog): Handle replace operation errors properly
replace-completedevent with the affected rows countaffectedRowsbeing undefined or null