refactor: extract shared SQL wrapper helper for repeated tool flow#1084
Conversation
- Add call_db_tool_with_default_db_warning helper in app/tools/utils/sql_wrapper.py to centralize the repeated pattern of resolving config, calling DB function, and optionally injecting warnings when database defaults - Migrate six SQL tools to use the wrapper, reducing code duplication: - AzureSQLCurrentQueriesTool - AzureSQLSlowQueriesTool - PostgreSQLCurrentQueriesTool - PostgreSQLSlowQueriesTool - MySQLCurrentProcessesTool - MariaDBProcessListTool - Add comprehensive tests for wrapper behavior: - Verify dict preservation - Verify warning injection on database defaulting - Verify no warning when database explicitly provided - Verify correct default database names used All existing tool tests continue to pass with no behavior change.
Greptile SummaryThis PR extracts a repeated six-step SQL-tool pattern (detect default → set default → resolve config → call vendor fn → inject warning → return) into a single helper Confidence Score: 5/5Safe to merge — all remaining findings are P2 style suggestions with no impact on runtime behaviour. Both findings are P2: one is a defensive-copy best practice (the dict mutation has no effect on current callers, which all pass fresh literals), and the other is a stale comment in a test. No logic changes, no schema changes, and 35 tests pass. app/tools/utils/sql_wrapper.py — consider making a copy of resolver_kwargs before mutating it. Important Files Changed
Sequence DiagramsequenceDiagram
participant Tool as SQL Tool (e.g. AzureSQLCurrentQueriesTool)
participant Wrapper as call_db_tool_with_default_db_warning
participant Resolver as config_resolver (e.g. resolve_azure_sql_config)
participant Caller as db_caller (e.g. get_current_queries)
participant Warnings as default_db_warning
Tool->>Wrapper: database, default_db_name, config_resolver, resolver_kwargs, db_caller
alt database is None
Wrapper->>Wrapper: database = default_db_name, _db_defaulted = True
end
Wrapper->>Wrapper: resolver_kwargs[database] = database
Wrapper->>Resolver: **resolver_kwargs
Resolver-->>Wrapper: config
Wrapper->>Caller: config
Caller-->>Wrapper: result dict
alt _db_defaulted
Wrapper->>Warnings: default_db_warning(default_db_name)
Warnings-->>Wrapper: warning string
Wrapper->>Wrapper: result[default_db_warning] = warning
end
Wrapper-->>Tool: result dict
Reviews (1): Last reviewed commit: "refactor: extract shared SQL wrapper hel..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| resolver_kwargs["database"] = database | ||
|
|
||
| # Resolve config using the vendor-specific resolver | ||
| config = config_resolver(**resolver_kwargs) |
There was a problem hiding this comment.
Line 43 writes directly into the resolver_kwargs dict the caller passed in. Every current call site happens to pass a fresh dict literal ({"server": server, "port": port}), so there's no visible bug today, but it breaks the least-surprise contract: a caller that stores its kwargs dict externally (or passes the same dict twice) will find an unexpected "database" key after the first call. Use a shallow copy to avoid the side-effect:
| resolver_kwargs["database"] = database | |
| # Resolve config using the vendor-specific resolver | |
| config = config_resolver(**resolver_kwargs) | |
| kwargs = {**resolver_kwargs, "database": database} | |
| config = config_resolver(**kwargs) |
(And remove the separate resolver_kwargs["database"] = database line above.)
| # Empty string is falsy but was explicitly provided, so no warning | ||
| # Actually, the wrapper treats None as "not provided", so "" would trigger warning | ||
| # Let me verify the actual behavior: the condition is "if database is None" | ||
| # So "" is NOT None and should not trigger warning |
There was a problem hiding this comment.
Lines 123–126 contain draft reasoning that contradicts itself ("Actually, the wrapper treats None …") and was clearly left over from the authoring process. It hurts readability and could confuse future maintainers. The assertion on line 128 is the ground truth; the comment above should either be replaced with a single clear statement or removed entirely.
| # Empty string is falsy but was explicitly provided, so no warning | |
| # Actually, the wrapper treats None as "not provided", so "" would trigger warning | |
| # Let me verify the actual behavior: the condition is "if database is None" | |
| # So "" is NOT None and should not trigger warning | |
| # Empty string is explicitly provided (not None), so no default warning is injected. |
- Use shallow copy for resolver_kwargs to avoid mutating caller's dict (all current call sites pass fresh dicts, but this prevents surprises for future callers who may store or reuse their kwargs dict) - Clean up self-contradicting comment in test_wrapper_handles_empty_string_database - Remove draft reasoning that contradicts itself - Replace with single clear statement explaining the behavior All 35 tests continue to pass (29 existing + 6 new wrapper tests).
- UP035: Import Callable from collections.abc instead of typing (Python 3.9+ prefers collections.abc for standard library collections) - UP047: Add Generic import to enable proper type parameter handling (required for TypeVar usage with generic function signatures) All tests continue to pass.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- F401: Removed unused Generic import (only TypeVar is needed, Generic is not used) - UP047: Add noqa comment to suppress false positive (UP047 requires PEP 695 syntax which isn't available in Python 3.10; function is already properly generic with TypeVar[T] usage) All tests passing.
- Wrap long description strings in tool decorators - Wrap long use_case strings in lists - Wrap long lambda function calls - Format test dict to multiple lines for readability All tests continue to pass.
- Keep long lambda expressions on single line (ruff preference) - Remove multi-line wrapping of lambdas All tests continue to pass.
|
@Ryjen1 LMK once review comments adressed |
|
Hi @muddlebee All review comments have been addressed! Changes made:
-All 35 tests passing (6 wrapper + 29 existing tools) |
| database = default_db_name | ||
|
|
||
| # Resolve config using the vendor-specific resolver | ||
| # Use shallow copy to avoid mutating caller's dict |
There was a problem hiding this comment.
Comment is inaccurate: {**resolver_kwargs, "database": database} creates a brand-new merged dict — it does not shallow-copy resolver_kwargs. Drop the comment; the spread syntax is self-explanatory.
| "Identifying slow queries that may be causing performance degradation", | ||
| "Analyzing query execution patterns during incident timeframes", | ||
| "Finding poorly optimized queries with high execution times or low cache hit rates", | ||
| ("Finding poorly optimized queries with high execution times or low cache hit rates"), |
There was a problem hiding this comment.
Wrapping a single string in parentheses without a trailing comma is a no-op — it does not create a tuple. Remove the parens to stay consistent with the other two list entries above it.
There was a problem hiding this comment.
the brackets "( )" seems uncessary 🤔 ??
|
@Ryjen1 just small nits above, otherwise good to go 👍 |
- Remove inaccurate comment about 'shallow copy' in sql_wrapper.py
({**resolver_kwargs, 'database': database} creates a brand-new merged dict,
not a shallow copy. The spread syntax is self-explanatory.)
- Remove unnecessary parentheses from PostgreSQL use_case string
(Wrapping single string in parens is a no-op; keep consistent with other entries)
All tests continue to pass.
|
Hi @muddlebee kindly take a look again |
|
Goood work 🎉🍺 |
|
🎯 Bullseye. @Ryjen1 opened a PR, kept the vibes clean, and got it merged. Absolute cinema. 🎬
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
3 participants
Summary
This PR resolves #894 by extracting the repeated SQL-tool wrapper pattern into a single, reusable helper function. The change centralizes the common flow of resolving database config, calling vendor-specific functions, and optionally injecting warnings when a database parameter defaults.
Changes
New Helper
app/tools/utils/sql_wrapper.py: Introducescall_db_tool_with_default_db_warning()to handle the orchestration pattern consistently across all SQL toolsMigrated Tools (No Behavior Change)
AzureSQLCurrentQueriesToolAzureSQLSlowQueriesToolPostgreSQLCurrentQueriesToolPostgreSQLSlowQueriesToolMySQLCurrentProcessesToolMariaDBProcessListToolAll tool names, schemas, and output keys remain unchanged. Vendor-specific config builders and query functions remain in their original locations under
app/integrations/.Test Coverage
tests/tools/test_sql_wrapper.py:NoneCode Reduction
Verification
✅ All 35 tests passing (29 existing + 6 new)
✅ All acceptance criteria from #894 met
✅ Scope strictly limited to six specified tools
✅ No behavioral changes to tool APIs