fix(validation): Auto-enable raw responses when zero records extracted #128
Conversation
Fixes #126 The auto-enable logic for raw_api_responses was only checking 'not success' but success remained True when API calls succeeded even if dpath extraction resulted in zero records. This prevented developers from getting debugging information for dpath configuration issues. Updated line 438 to also check len(records_data) == 0 to match the documented behavior in the comment above it. Testing: - Verified with wrong dpath manifest that raw responses now auto-enable - All tests pass (97 passed, 2 skipped, 1 xfailed) - Lint and format checks pass Co-Authored-By: AJ Steers <[email protected]>
Original prompt from AJ Steers |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This Branch via MCPTo test the changes in this specific branch with an MCP client like Claude Desktop, use the following configuration: {
"mcpServers": {
"connector-builder-mcp-dev": {
"command": "uvx",
"args": ["--from", "git+https://github.com/airbytehq/connector-builder-mcp.git@devin/1760122860-fix-auto-enable-raw-responses", "connector-builder-mcp"]
}
}
}Testing This Branch via CLIYou can test this version of the MCP Server using the following CLI snippet: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/connector-builder-mcp.git@devin/1760122860-fix-auto-enable-raw-responses#egg=airbyte-connector-builder-mcp' --helpPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
📝 WalkthroughWalkthroughExpanded execute_stream_test_read logic to ensure raw API responses are included when the explicit flag is true, when the operation failed, or when zero records were extracted; also logs a warning and forces inclusion of raw responses when zero records are returned despite overall success. No public interfaces changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Validator as execute_stream_test_read
participant API
Caller->>Validator: invoke(stream_name, config, include_raw_responses_data?)
Validator->>API: request data
API-->>Validator: response (slices, payload)
Note over Validator: extract records_data\ncompute success (based on response)
alt include_raw_responses_data == True\nor success == False\nor len(records_data) == 0
Validator->>Validator: include raw_api_responses (log warning if zero records but success)
else
Validator->>Validator: raw_api_responses = null
end
Validator-->>Caller: return result (records_read, success, raw_api_responses)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
Comment |
PyTest Results (Full)0 tests 0 ✅ 0s ⏱️ Results for commit 3c60dab. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the validation testing logic where raw API responses were not being auto-enabled when zero records were extracted, despite the code comment indicating this should happen. The fix ensures that developers get debugging information when dpath configuration issues cause zero records to be extracted from successful API calls.
- Adds the missing zero records condition to the auto-enable logic for raw responses
- Aligns the actual implementation with the documented behavior in the code comment
- Provides critical debugging support for dpath configuration issues
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…verridden for zero records Update the parameter description for include_raw_responses_data to clearly document that when set to False, raw responses will still be auto-enabled if zero records are extracted. This ensures developers understand the override behavior for debugging scenarios. Related to #126 Co-Authored-By: AJ Steers <[email protected]>
Add explicit warning message to execution logs when a successful API call returns zero records, alerting users to review raw responses to verify the result is correct (e.g., not due to incorrect dpath configuration). The warning message reads: 'Read attempt returned zero records. Please review the included raw responses to ensure the zero-records result is correct.' Related to #126 Co-Authored-By: AJ Steers <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
connector_builder_mcp/validation_testing.py (1)
439-439: Fix correctly addresses the zero-records bug.The extended condition now auto-enables raw responses when API calls succeed but dpath extraction yields zero records, which is exactly the debugging scenario described in issue #126. The logic properly distinguishes between API errors (
not success) and successful-but-empty extractions (len(records_data) == 0).Optional style refinement:
Consider using the more Pythonic empty-check idiom:
- include_raw_responses_data = include_raw_responses_data or not success or len(records_data) == 0 + include_raw_responses_data = include_raw_responses_data or not success or not records_dataBoth are correct;
not records_datais slightly more idiomatic for empty-sequence checks in Python.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
connector_builder_mcp/validation_testing.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (Fast)
- GitHub Check: Test Connector Build (JSONPlaceholder)
- GitHub Check: Test Connector Build (PokemonTGG)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Run Evals (Single Connector)
🔇 Additional comments (2)
connector_builder_mcp/validation_testing.py (2)
331-333: LGTM! Clear documentation of auto-enable behavior.The docstring accurately describes the tri-state behavior and makes the debugging intent explicit. The clarification that False is overridden for zero-records scenarios is particularly helpful.
441-447: LGTM! Helpful debugging guidance.The warning appropriately triggers only when zero records are extracted despite a successful read operation, guiding users to inspect the raw responses for dpath configuration issues. The log format is consistent with the existing codebase.
Refactor the auto-enable logic for raw responses to make it clearer: - Explicitly set include_raw_responses_data=True inside the zero-records warning block - Simplify the final OR expression to only check 'not success' This makes the code more readable and maintainable by separating the zero-records override from the error handling. Addresses PR feedback from @aaronsteers Related to #126 Co-Authored-By: AJ Steers <[email protected]>
aaronsteers
deleted the
devin/1760122860-fix-auto-enable-raw-responses
branch
1 participant
fix(validation): Auto-enable raw responses when zero records extracted
Summary
Fixes a bug in
execute_stream_test_readwhereraw_api_responseswere not being auto-enabled when zero records were extracted due to incorrect dpath configuration, even though the code comment indicated this should happen.The Problem:
not success, missing the zero records casesuccessremainedTrueand raw responses weren't auto-enabledThe Fix:
or len(records_data) == 0to the auto-enable conditionReview & Testing Checklist for Human
field_path: ["results"]for JSONPlaceholder API), callexecute_stream_test_readwithinclude_raw_responses_data=false, and verifyraw_api_responsesis populated (not null)Notes
The fix is minimal and targeted, addressing the exact discrepancy between documented and actual behavior without changing the broader logic flow.
Summary by CodeRabbit
Bug Fixes
Documentation