fix(redshift-mcp-server): handle missing DBName field in discover_clusters#2685
Conversation
…sters
The describe_clusters API may return cluster metadata without a DBName
field in certain scenarios. Previously, direct key access caused a
KeyError when this field was missing.
Fix: Use dict.get('DBName') for safe access, returning None when the
field is absent instead of raising an exception.
Includes regression test that verifies the function handles clusters
without DBName gracefully.
Fixes: awslabs#2331
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: abhu85 <60182103+abhu85@users.noreply.github.com>
Per maintainer @grayhemp's feedback in issue awslabs#2331, default to 'dev' (Redshift's default database name) instead of None when DBName field is missing from the cluster metadata. This matches the behavior of the Serverless workgroup discovery code which already defaults to 'dev'. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks @grayhemp for the guidance! I've updated the PR to default to 'database_name': cluster.get('DBName', 'dev'),This matches the existing Serverless workgroup behavior (lines 454-456) which already defaults to Regarding Serverless - I checked and it already handles the missing database name case correctly via: 'database_name': workgroup_detail.get('configParameters', [{}])[0].get('parameterValue', 'dev'),So only the provisioned cluster path needed this fix. |
Indeed, awesome. Thank you very much. |
|
Additionally, could you please run E2E tests and post the results, similarly to how it is done here #1927 (see the description)? You might need to modify the prompts to exclude the PRs specific part as it's not merged yet. Thanks again. |
Per maintainer feedback, modified test_discover_clusters_provisioned() to
return 2 mock entries instead of a separate test method:
- Entry 1: Full cluster data (extends entry 2)
- Entry 2: Minimal data with DBName omitted
This tests the .get('DBName', 'dev') fix within the existing test coverage,
avoiding code duplication and extra test methods.
|
@grayhemp Thanks for the feedback! I've refactored the test as suggested:
All tests pass locally: Regarding E2E tests - I don't have a Redshift cluster available for testing. Could you run E2E tests on your end, or is there a test cluster I could use? |
kiro-cli chat \
--model claude-opus-4.6-1m \
--no-interactive \
--trust-tools "@{awslabs.redshift-mcp-server}/*,execute_bash,fs_read" \
'Use Redshift MCP Server to run end-to-end (integration) tests covering the scenarios for the changes introduced in the current branch only. Get the test scenario ideas from the unit tests under the project directory. Provide a short testing summary, one line per scenario.'Change Specific Integration Test Summary
All 7 scenarios passed. The .get('DBName', 'dev') fix correctly defaults database_name to |
kiro-cli chat \
--model claude-opus-4.6-1m \
--no-interactive \
--trust-tools "@{awslabs.redshift-mcp-server}/*,execute_bash,fs_read" \
'Use Redshift MCP Server to run a complete set of end-to-end (integration) tests covering all its tools. Check both provisioned and serverless clusters, including the database schema exploration in both. Check the SQL read-only protection, transaction breaker protection, failed user SQL behavior. Get the test scenario ideas from the unit tests under the project directory. Provide a short testing summary, one line per tool.'Complete Integration Test Summary
All 19 test scenarios passed across both provisioned and serverless cluster types. The three protection layers (read-only transactions, transaction breaker regex, graceful error handling with END cleanup) all function correctly in production. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2685 +/- ##
==========================================
- Coverage 91.30% 91.29% -0.02%
==========================================
Files 1001 1001
Lines 73725 73725
Branches 11880 11880
==========================================
- Hits 67316 67305 -11
- Misses 3959 3966 +7
- Partials 2450 2454 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@abhu85 Thank you. I ran E2E tests and they all look good. Let me get the second reviewer/approval. |
…sters (awslabs#2685) * fix(redshift-mcp-server): handle missing DBName field in discover_clusters The describe_clusters API may return cluster metadata without a DBName field in certain scenarios. Previously, direct key access caused a KeyError when this field was missing. Fix: Use dict.get('DBName') for safe access, returning None when the field is absent instead of raising an exception. Includes regression test that verifies the function handles clusters without DBName gracefully. Fixes: awslabs#2331 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: abhu85 <60182103+abhu85@users.noreply.github.com> * fix: default DBName to 'dev' per maintainer feedback Per maintainer @grayhemp's feedback in issue awslabs#2331, default to 'dev' (Redshift's default database name) instead of None when DBName field is missing from the cluster metadata. This matches the behavior of the Serverless workgroup discovery code which already defaults to 'dev'. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: merge DBName default test into existing provisioned test Per maintainer feedback, modified test_discover_clusters_provisioned() to return 2 mock entries instead of a separate test method: - Entry 1: Full cluster data (extends entry 2) - Entry 2: Minimal data with DBName omitted This tests the .get('DBName', 'dev') fix within the existing test coverage, avoiding code duplication and extra test methods. --------- Signed-off-by: abhu85 <60182103+abhu85@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
KeyError: 'DBName'whendiscover_clusters()processes clusters without theDBNamefield present.get('DBName', 'dev')with default to 'dev' (Redshift's default database name)Problem
The
describe_clustersAPI may return cluster metadata without aDBNamefield in certain scenarios (e.g., clusters created via certain methods, or clusters in specific states). The current implementation used direct dictionary accesscluster['DBName']which raisesKeyErrorwhen the field is missing.Per AWS API Reference,
DBNameis marked as "Required: No".Root Cause
Line 417 in
redshift.py:Other fields in the same block already used safe access patterns (
.get()), butDBNamewas missed.Fix
Changed to safe dictionary access with default value:
This matches the existing Serverless workgroup behavior which already defaults to 'dev'.
Tests Run
All 13 discover tests pass, including new test for missing DBName scenario.
Fixes #2331
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.