fix(studio): resolve SQL-console mutation PK from declared metadata, drop the 'id'-name guess - #218
Merged
Merged
Conversation
Contributor
|
🧙 Sourcery has finished reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…drop the 'id'-name guess The mutation path used to fall back to any column named 'id' as the WHERE key, silently updating/deleting every matching row on tables without a real primary key. The derivation is now a pure, tested resolveMutationPrimaryKey() that only trusts declared PK metadata — from the result's column definitions or from the loaded schema for the extracted source table — and requires the PK column to be present in the result set. Closes #207
Contributor
📝 WalkthroughWalkthroughSQL console mutation key resolution is extracted into a schema-aware resolver, integrated into result rendering, and covered by tests. Mutations now require a single primary key that exists in the selected result columns. ChangesSQL mutation safety
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/studio/src/features/sql-console/mutation-primary-key.ts (1)
8-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename unexported args type to
Props.Per coding guidelines, an unexported single props/arguments type should be named
type Props(or defined inline), not a distinct name likeResolveArgs.As per coding guidelines, "name it
type Propsor define it inline instead of naming itComponentNameProps."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/sql-console/mutation-primary-key.ts` around lines 8 - 13, Rename the unexported ResolveArgs type to Props and update all references to it, preserving its fields and optionality unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/studio/src/features/sql-console/mutation-primary-key.ts`:
- Around line 82-92: Update findSchemaTable to collect all case-insensitive
table-name matches when schemaPart is absent, returning a table only when
exactly one schema-less name match exists; return undefined for multiple matches
so the schema-table primary-key fallback is disabled. Preserve the existing
schema-qualified lookup behavior.
---
Nitpick comments:
In `@packages/studio/src/features/sql-console/mutation-primary-key.ts`:
- Around line 8-13: Rename the unexported ResolveArgs type to Props and update
all references to it, preserving its fields and optionality unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6207dba1-833f-4a5c-b35f-954b567bcbfc
📒 Files selected for processing (4)
__tests__/apps/desktop/src/features/sql-console/mutation-primary-key.test.tspackages/studio/src/features/sql-console/components/sql-results.tsxpackages/studio/src/features/sql-console/mutation-primary-key.tspackages/studio/src/features/sql-console/sql-console.tsx
Comment on lines
+82
to
+92
| function findSchemaTable(sourceTable: string, schemaTables: TableInfo[]): TableInfo | undefined { | ||
| const separatorIndex = sourceTable.lastIndexOf('.') | ||
| const schemaPart = separatorIndex === -1 ? undefined : sourceTable.slice(0, separatorIndex) | ||
| const namePart = separatorIndex === -1 ? sourceTable : sourceTable.slice(separatorIndex + 1) | ||
|
|
||
| return schemaTables.find(function (table) { | ||
| if (table.name.toLowerCase() !== namePart.toLowerCase()) return false | ||
| if (!schemaPart) return true | ||
| return (table.schema ?? '').toLowerCase() === schemaPart.toLowerCase() | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'function extractMutationSourceTable' -A 20 packages/studio/src/features/sql-consoleRepository: remcostoeten/dora
Length of output: 2145
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files packages/studio/src/features/sql-console | rg '(mutation-primary-key|query-target|sql-console|TableInfo|schemaTables)' || true
echo
echo "== mutation-primary-key outline =="
ast-grep outline packages/studio/src/features/sql-console/mutation-primary-key.ts --view expanded || true
echo
echo "== mutation-primary-key relevant section =="
sed -n '1,180p' packages/studio/src/features/sql-console/mutation-primary-key.ts
echo
echo "== query-target relevant extraction implementation =="
sed -n '1,180p' packages/studio/src/features/sql-console/query-target.ts
echo
echo "== schemaTables/table metadata references =="
rg -n 'schemaTables|TableInfo|refreshSchema|findSchemaTable|extractMutationSourceTable' packages/studio/src/features/sql-console packages/studio/src -g '*.ts' -g '*.tsx'Repository: remcostoeten/dora
Length of output: 25371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== query-target tests/fixtures =="
fd -i 'query-target|mutation-primary-key|data-provider|mock' packages/studio/src | sed -n '1,120p'
echo
echo "== locate adapters/mock relevant section =="
sed -n '600,660p' packages/studio/src/core/data-provider/adapters/mock.ts
echo
echo "== inspect query-target exports/usages with context =="
rg -n 'extractMutationSourceTable|resolveMutationPrimaryKey' packages/studio/src -g '*.ts' -g '*.tsx' -A5 -B5
echo
echo "== deterministic parser probe =="
node - <<'JS'
function unquoteIdentifierPart(part) {
const trimmed = part.trim()
if (trimmed.startsWith('"') && trimmed.endsWith('"') && trimmed.length >= 2) {
return trimmed.slice(1, -1).replace(/""/g, '"')
}
return trimmed
}
function normalizeIdentifier(identifier) {
const trimmed = identifier.trim()
if (!trimmed) return undefined
const parts = trimmed.split('.').map(unquoteIdentifierPart).filter(Boolean)
if (parts.length === 0 || parts.length > 2) return undefined
return parts.join('.')
}
function matchLeadingIdentifier(segment) {
const match = segment.match(/^\s*((?:"[^"]+"|[A-Za-z_][\w$]*)(?:\s*\.\s*(?:"[^"]+"|[A-Za-z_][\w$]*))?)/)
if (!match) return undefined
return normalizeIdentifier(match[1])
}
function extractSingleSelectTable(query) {
const fromMatch = query.match(/\bfrom\b([\s\S]*)/i)
if (!fromMatch) return undefined
const afterFrom = fromMatch[1]
const boundaryMatch = afterFrom.match(/\b(where|group\s+by|order\s+by|limit|offset|returning|union|intersect|except|having|for)\b/i)
if (!boundaryMatch || boundaryMatch.index === undefined)
return afterFrom.trim().replace(/\s+/g, ' ').trim()
return afterFrom.slice(0, boundaryMatch.index).trim().replace(/\s+/g, ' ').trim()
}
function normalizeQueryForProbe(query) {
let normalized = query.trim().toUpperCase().replace(/\s+/g, ' ')
if (normalized.startsWith('SELECT') || normalized.startsWith('WITH')) return query
let pattern = null
if (normalized.startsWith('DELETE FROM ')) {
pattern = /^DELETE FROM ((?:\"[^\"]+\"|[A-Za-z_][\w$]*)(?:\s*\.\s*(?:\"[^\"]+\"|[A-Za-z_][\w$]*))?)/i
const m = query.match(pattern)
return m ? normalizeIdentifier(m[1]) : undefined
}
if (normalized.startsWith('UPDATE ')) return extractSingleSelectTable(query) // no, wait:
}
function extractFromClauseSegmentForProbe(query) {
const fromMatch = query.match(/\bfrom\b([\s\S]*)/i)
if (!fromMatch) return undefined
const afterFrom = fromMatch[1]
const boundaryMatch = afterFrom.match(/\b(where|group\s+by|order\s+by|limit|offset|returning|union|intersect|except|having|for)\b/i)
if (!boundaryMatch || boundaryMatch.index === undefined) {
return afterFrom.trim().replace(/\s+/g, ' ')
} else {
return afterFrom.slice(0, boundaryMatch.index).trim().replace(/\s+/g, ' ')
}
}
function extractSingleDmlTableForProbe(query, pattern) {
const match = query.match(pattern)
if (!match) return undefined
return normalizeIdentifier(match[1])
}
const cases = [
'SELECT id, name FROM users',
'SELECT id, name FROM "public".users',
'DELETE FROM users',
'DELETE FROM "public".users',
'UPDATE users SET name = 1'
]
for (const query of cases) {
const upper = query.trim().toUpperCase()
let out = 'UNK'
if (/^SELECT\b|^WITH\b/i.test(query)) {
out = extractSingleSelectTable(query)
out += ` [fromSegment:${extractFromClauseSegmentForProbe(query)}]`
} else if (/^UPDATE\b/i.test(query)) {
out = extractSingleDmlTableForProbe(query, /^update\s+((?:"[^"]+"|[A-Za-z_][\w$]*)(?:\s*\.\s*(?:"[^"]+"|[A-Za-z_][\w$]*))?)/i)
} else if (/^DELETE\b/i.test(query)) {
out = extractSingleDmlTableForProbe(query, /^delete\s+from\s+((?:"[^"]+"|[A-Za-z_][\w$]*)(?:\s*\.\s*(?:"[^"]+"|[A-Za-z_][\w$]*))?)/i)
}
console.log(query, '=>', out)
}
function findSchemaTable(sourceTable, schemaTables) {
const separatorIndex = sourceTable.lastIndexOf('.')
const schemaPart = separatorIndex === -1 ? undefined : sourceTable.slice(0, separatorIndex)
const namePart = separatorIndex === -1 ? sourceTable : sourceTable.slice(separatorIndex + 1)
return schemaTables.find(function (table) {
if (table.name.toLowerCase() !== namePart.toLowerCase()) return false
if (!schemaPart) return true
return (table.schema ?? '').toLowerCase() === schemaPart.toLowerCase()
})
}
const ambig = [
{name:'users', schema:'public', columns:[{name:'id', primaryKey:true}]},
{name:'users', schema:'audit', columns:[{name:'id', primaryKey:false}]},
]
console.log('unqualified sourceTable=', findSchemaTable('users', ambig))
console.log('qualified sourceTable=', findSchemaTable('audit.users', ambig))
JSRepository: remcostoeten/dora
Length of output: 13088
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sql-console refreshSchema section =="
sed -n '180,340p' packages/studio/src/features/sql-console/sql-console.tsx
echo
echo "== types TableInfo =="
sed -n '1,80p' packages/studio/src/features/sql-console/types.tsRepository: remcostoeten/dora
Length of output: 6684
Guard schema-table resolution against duplicate table names.
extractMutationSourceTable can return unqualified names such as UPDATE users or DELETE FROM users. When schemaTables has multiple users tables across different schemas, findSchemaTable() returns the first match by iteration order instead of a table that is actually unambiguously targeted by the query. Treat multiple schema-less name matches as ambiguous and disable the schema-table PK fallback for them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/studio/src/features/sql-console/mutation-primary-key.ts` around
lines 82 - 92, Update findSchemaTable to collect all case-insensitive table-name
matches when schemaPart is absent, returning a table only when exactly one
schema-less name match exists; return undefined for multiple matches so the
schema-table primary-key fallback is disabled. Preserve the existing
schema-qualified lookup behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
SQL-console cell edit / row delete used to pick its
WHEREkey by falling back to any column namedid, case-insensitively, with no uniqueness check. On a table without a declared primary key but with a non-uniqueid-named column (staging/join/log tables), editing one cell silently updated every matching row, and deleting one row deleted all of them.How
resolveMutationPrimaryKey()inpackages/studio/src/features/sql-console/mutation-primary-key.ts(mirrors the existingquery-target.tspattern). It only trusts declared PK metadata:sql-results.tsxloses the duplicated inline derivation;sql-console.tsxpasses its already-loadedtablesschema down.Net effect: mutations now also work on tables whose PK isn't named
id(previously impossible, since engines return result columns as bare name strings), and never fire on an unverifiedidcolumn.Verification
__tests__/apps/desktop/src/features/sql-console/mutation-primary-key.test.tsbun run test:desktop: 789 passedbun run lint, studio + desktoptypecheck: cleanCloses #207
Summary by Sourcery
Resolve SQL-console row mutation primary key strictly from declared metadata and disable unsafe mutations when no reliable primary key is available.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes