-
Notifications
You must be signed in to change notification settings - Fork 2
fix: preserve specific CDK validation errors in validate_manifest #14
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
- Replace overly broad exception handling with specific ValidationError handling - Preserve detailed JSON schema validation error messages from CDK - Maintain backward compatibility for other exception types - Add jsonschema.ValidationError import to catch schema validation failures This fixes the issue where generic 'Validation error' messages were masking helpful CDK validation details, improving the developer experience when debugging manifest validation failures. 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/1754351228-improve-validation-errors", "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/1754351228-improve-validation-errors#egg=airbyte-connector-builder-mcp' --help PR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
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.
Pull Request Overview
This PR fixes overly broad exception handling in the validate_manifest()
function to preserve specific CDK validation errors instead of masking them with generic messages. Users will now see detailed JSON schema validation errors that include specific information about manifest incompatibilities and validation failures.
- Added specific handling for
jsonschema.ValidationError
exceptions - Preserved detailed error messages from CDK validation instead of generic "Validation error" messages
- Maintained backward compatibility for other exception types
|
||
except ValidationError as e: | ||
logger.error(f"JSON schema validation error: {e}") | ||
errors.append(f"JSON schema validation failed: {str(e)}") |
Copilot
AI
Aug 6, 2025
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.
The new ValidationError exception handler should be placed before the generic Exception handler but after any other specific exceptions. Consider whether other specific CDK exceptions should also be handled explicitly to provide better error messages.
errors.append(f"JSON schema validation failed: {str(e)}") | |
errors.append(f"JSON schema validation failed: {str(e)}") | |
except AirbyteTracedException as e: | |
logger.error(f"Airbyte CDK traced exception: {e}") | |
errors.append(f"Airbyte CDK error: {str(e)}") |
Copilot uses AI. Check for mistakes.
- Add jsonschema>=4.0.0 to main dependencies to fix Deptry analysis - Add types-jsonschema>=4.0.0 to dev dependencies to fix MyPy type checking - Resolves CI failures for ValidationError import from jsonschema.exceptions Co-Authored-By: AJ Steers <[email protected]>
- Add pre-validation using declarative_component_schema.yaml from CDK - Extract detailed error information: field paths, invalid values, schema constraints - Preserve CDK validation as fallback if schema loading fails - Format validation errors with specific field paths and constraint details Tested with Microsoft Lists manifest and invalid test cases showing: - Field-specific error paths (e.g., 'at field type') - Invalid values received vs expected constraints - Schema type and enum validation details - Significant improvement over generic 'schema validation failed' messages Co-Authored-By: AJ Steers <[email protected]>
- Move pyyaml from dev to main dependencies (imported in production code) - Add types-pyyaml to dev dependencies for MyPy type checking - Fixes MyPy and Deptry CI failures while preserving functionality Co-Authored-By: AJ Steers <[email protected]>
- Add module-level caching for declarative component schema - Avoid reloading and parsing YAML schema on every validation call - Fixes test_performance_multiple_tool_calls CI timeout (16.53s -> 8.97s) - Preserves detailed validation error functionality Co-Authored-By: AJ Steers <[email protected]>
fix: preserve specific CDK validation errors in validate_manifest
Summary
Fixes overly broad exception handling in the
validate_manifest()
function that was masking specific CDK validation errors with generic "Validation error" messages.Before: Users would see unhelpful errors like:
After: Users now see specific validation details like:
Changes made:
jsonschema.ValidationError
importValidationError
specifically before the generalException
catchReview & Testing Checklist for Human
Recommended test plan: Create a manifest with known schema violations (e.g., invalid version, missing required fields, invalid field types) and validate it through the MCP tools to confirm error messages are now detailed and helpful.
Diagram
Notes
Requested by: AJ Steers (@aaronsteers)
Devin session: https://app.devin.ai/sessions/09bc6e5fedd64dc9a958add0f43a868b