-
Notifications
You must be signed in to change notification settings - Fork 201
Fix: Properly quote database and schema identifiers for SQL compatibility #1987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix: Properly quote database and schema identifiers for SQL compatibility #1987
Conversation
WalkthroughWrap retrieved relation identifier(s) in double quotes: if the relation contains a dot, split into database and schema and return Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DataMonitor
participant Logger
Caller->>DataMonitor: get_relation()
DataMonitor->>DataMonitor: retrieve relation string
alt relation contains dot
DataMonitor->>DataMonitor: split into db and schema
DataMonitor->>DataMonitor: format as "<db>"."<schema>"
else single identifier
DataMonitor->>DataMonitor: format as "<relation>"
end
DataMonitor->>Logger: log quoted relation
DataMonitor->>Caller: return quoted relation
opt exception
DataMonitor->>Logger: log error
DataMonitor->>Caller: return fallback "<elementary_database>.<elementary_schema>"
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
👋 @mschmidoev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
elementary/monitor/data_monitoring/data_monitoring.py (2)
82-83: If embedding inside single-quoted SQL literals, escape single quotes insteadIf the goal is to safely place the relation inside a single-quoted SQL string literal (e.g., in a Slack snippet), converting double quotes is unnecessary and potentially harmful. Instead, escape single quotes in the content:
- relation = relation.replace('"', "'") if relation else relation + display_relation = relation.replace("'", "''") if relation else relationThis yields a string-literal-safe version while preserving any double quotes that are harmless inside single-quoted literals.
82-85: Scope quote-normalization for display onlyWe verified that the
get_elementary_database_and_schemamacro always returns an unquoteddatabase.schemastring, and thatself.elementary_database_and_schemais used broadly (in tests, Slack messages, alert formatting, etc.). Replacing quotes on the canonical return value is a no-op today but could strip valid identifier quoting in the future. To avoid mutating the source-of-truth, only normalize for display:In
elementary/monitor/data_monitoring/data_monitoring.py(lines 82–85):- # Replace double quotes with single quotes for proper SQL compatibility - relation = relation.replace('"', "'") if relation else relation - logger.info(f"Elementary's database and schema: '{relation}'") - return relation + # Display-only: replace double quotes with single quotes in logs + display_relation = relation.replace('"', "'") if isinstance(relation, str) else relation + logger.info(f"Elementary's database and schema: '{display_relation}'") + return relationIf downstream callers (e.g. Slack templates) need a “safe” string, consider adding a derived
elementary_database_and_schema_displayproperty or performing.replace('"', "'")at the call site instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
elementary/monitor/data_monitoring/data_monitoring.py(1 hunks)
4c14bf7 to
6a66be5
Compare
|
@mschmidoev The CI is failing for some reason. Can you update your branch to the latest |
…and_schema - Replace double quotes with single quotes for proper SQL compatibility - Fixes SQL errors in Slack alert messages when database/schema names contain quotes - Added conditional check to handle None values safely
6a66be5 to
a3a3581
Compare
a3a3581 to
68defa0
Compare
68defa0 to
60faf1c
Compare
|
@elazarlachkar should be good to go :) |
|
Hi @mschmidoev! The CI still has the issue we solved on Thanks, and sorry for the trouble. |
…e-quotes-with-single-quotes
60faf1c to
701198f
Compare
|
Hey @elazarlachkar , no dramas, should be good now |
…e-quotes-with-single-quotes
|
Hi @mschmidoev! |
|
Thanks @elazarlachkar, was off last week but this PR should be up to date with master branch now! |
|
@elazarlachkar looks like that's worked this time around! Let me know if there's anything else you need :) ) |
|
Hey @elazarlachkar just checking if there's any progress here :) |
Summary
This PR updates the logic for constructing the elementary_database_and_schema identifier so that the database and schema are each wrapped in double quotes, e.g. "database"."schema". This ensures correct SQL syntax and avoids treating the entire string as a single identifier.
Details
Previously, the code would wrap the whole database.schema string in quotes, resulting in "database.schema", which is not valid in standard SQL.
Now, the code splits the identifier on the dot and wraps each part individually: "database"."schema".
If only a schema is present, it is wrapped as "schema".
#1986
Summary by CodeRabbit