-
Notifications
You must be signed in to change notification settings - Fork 2
fix: return raw responses when requested even with 0 records #89
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
Conversation
- Remove condition requiring slices to be non-empty when raw responses are requested - Add raw response handling in early return path for failed stream reads - Fixes bug where include_raw_responses_data=true returned null instead of API responses - Raw responses should be returned when explicitly requested regardless of record count Before fix: raw_api_responses was null when max_records=0 and include_raw_responses_data=true After fix: raw_api_responses returns empty list [] containing API response structure 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/1758568320-fix-raw-responses-bug", "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/1758568320-fix-raw-responses-bug#egg=airbyte-connector-builder-mcp' --help PR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
…d but unavailable - Enhanced success path to return detailed debugging information when slices is empty - Improved failure path with actionable troubleshooting steps and error analysis - Added possible causes and debugging suggestions for dpath extractor issues - Addresses user feedback that empty list is not helpful for debugging API response issues Co-Authored-By: AJ Steers <[email protected]>
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.
Devin, One comment inline. Also, your code is extremely ugly and hard to read. And I have a feeling you are still "doing too much". Move parsing into helper function (s) if you need to but please make this less ugly.
You added almost a hundred lines of code for what I expected in 2-10 lines
- Extract debugging logic into clean helper functions _get_raw_responses_for_failure and _get_raw_responses_for_success - Reduce complex debugging dictionaries to simple 2-line function calls - Add comment explaining auxiliary_requests logic as requested in PR feedback - Maintain all existing bug fix functionality while drastically improving code readability - Addresses user feedback that code was 'extremely ugly and hard to read' Co-Authored-By: AJ Steers <[email protected]>
Fix raw API responses bug and refactor for readability
Summary
Fixes a bug where
execute_stream_test_read
returnednull
forraw_api_responses
wheninclude_raw_responses_data: true
was set, even when no records were found. This prevented users from debugging API response issues, particularly when dpath extractors were misconfigured.Key changes:
raw_api_responses
always returns helpful data when requested, nevernull
max_pages_per_slice
andmax_slices
to prevent CDK errors withmax_records=0
Review & Testing Checklist for Human
Risk Level: 🟡 Medium - Changes core API behavior and return structure
include_raw_responses_data=true
but no HTTP data is captured, the returned debugging information actually helps users understand and fix dpath extractor issuesmax(1, max_records)
doesn't break legitimate use cases wheremax_records=0
should be supported, and that it properly fixes the "Record limit must be between 1 and 0" errorinclude_raw_responses_data=true
to ensureraw_api_responses
is properly populated in all code pathsexecute_stream_test_read
still work correctly with the modified return structure (now includesraw_api_responses
in failure cases)Notes
auxiliary_requests
fallback logic is new behavior that may need monitoring in production