Skip to content

fix: use dialect-aware identifier quoting in TableQueryBuilder#305

Merged
datlechin merged 1 commit intomainfrom
fix/table-query-builder-identifier-quoting
Mar 13, 2026
Merged

fix: use dialect-aware identifier quoting in TableQueryBuilder#305
datlechin merged 1 commit intomainfrom
fix/table-query-builder-identifier-quoting

Conversation

@datlechin
Copy link
Copy Markdown
Collaborator

@datlechin datlechin commented Mar 13, 2026

Summary

  • Fix identifier quoting inconsistency where opening a table used correct dialect quoting (e.g., backticks for MySQL) but refreshing fell back to hardcoded double-quotes
  • Add dialectQuote closure to TableQueryBuilder so the fallback quote style matches the database dialect when pluginDriver is nil
  • Affects MySQL/MariaDB, SQLite, and ClickHouse (all use backtick quoting); PostgreSQL/MSSQL/Oracle were unaffected

Root Cause

TableQueryBuilder.quote() had a hardcoded double-quote fallback when pluginDriver was nil. For MySQL/MariaDB/SQLite/ClickHouse, pluginDriver is always nil (they don't implement buildBrowseQuery), so refresh queries used "table" instead of `table`.

Meanwhile, the initial table open path (QueryTab.buildBaseTableQuery) correctly used quoteIdentifierFromDialect() to get backtick quoting from the dialect.

Test plan

  • Open a MySQL/MariaDB table — verify query uses backticks
  • Refresh the same table (Cmd+R) — verify query still uses backticks (not double-quotes)
  • Repeat for SQLite and ClickHouse
  • Verify PostgreSQL still uses double-quotes on both open and refresh

Summary by CodeRabbit

  • Improvements
    • Enhanced identifier quoting to support database dialect-specific formatting, improving SQL compatibility across different database systems.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 13, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 421dc493-325b-430e-8769-8830dd8c31a2

📥 Commits

Reviewing files that changed from the base of the PR and between 23cdac8 and fdf7c15.

📒 Files selected for processing (2)
  • TablePro/Core/Services/Query/TableQueryBuilder.swift
  • TablePro/Views/Main/MainContentCoordinator.swift

📝 Walkthrough

Walkthrough

This change introduces a customizable dialectQuote closure property to TableQueryBuilder and integrates it with the plugin-driven dialect system in MainContentCoordinator. The quote() method now delegates to this closure instead of performing inline escaping, allowing dialect-specific identifier quoting.

Changes

Cohort / File(s) Summary
TableQueryBuilder Quoting Logic
TablePro/Core/Services/Query/TableQueryBuilder.swift
Added a dialectQuote closure property and optional initializer parameter with a default quoting function. The quote(_:) method now delegates to dialectQuote instead of inline escaping.
MainContentCoordinator Integration
TablePro/Views/Main/MainContentCoordinator.swift
Wired the dialectQuote parameter into TableQueryBuilder initialization, obtaining the quoting function from quoteIdentifierFromDialect(PluginManager.shared.sqlDialect(for:)).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • PR #301: Aligns with the move of identifier-quoting into the plugin/dialect system and the addition of the quoteIdentifierFromDialect helper.
  • PR #294: Provides the underlying SQLDialectDescriptor and PluginManager dialect API that this change depends on for dialect-aware identifier quoting.
  • PR #295: Overlaps in efforts to centralize identifier quoting through plugin-provided dialects.

Poem

A rabbit hops through SQL gardens fair,
Where quotes dance gracefully in the air,
Dialects whisper how names should be dressed,
With closures and plugins, the identifiers blessed! 🐰✨

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/table-query-builder-identifier-quoting
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

@datlechin datlechin merged commit 6e1b540 into main Mar 13, 2026
2 of 3 checks passed
@datlechin datlechin deleted the fix/table-query-builder-identifier-quoting branch March 13, 2026 03:21
datlechin added a commit that referenced this pull request Mar 13, 2026
datlechin added a commit that referenced this pull request Mar 13, 2026
…adata lookups (#306)

* feat: replace hardcoded DatabaseType switches with dynamic plugin metadata lookups (#305)

* fix: address PR review feedback for plugin metadata lookups

* fix: address PR review round 2 feedback

- Fix Switch Connection button disabled state (connection switching always works)
- Localize cascade description strings in TableOperationDialog
- Sanitize file extension in ConnectionFormView filePathPrompt
- Deduplicate aliased plugins via ObjectIdentifier in PluginManager

* fix: address code review issues in plugin metadata PR

- Remove .db from DuckDB file extensions to avoid collision with SQLite
- Use supportsImport plugin lookup instead of editorLanguage check in sidebar
- Wire SidebarContextMenuLogic.importVisible into production view body
- Simplify window title to unconditional "\(langName) Query" pattern
- Update tests to match supportsImport-based import visibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant