Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughAdds optional Pydantic model support to the framework with conditional imports, validation utilities, and OpenAPI schema integration. Includes lazy-loaded helpers for type checking and validation, integration points in request routing and schema generation, and comprehensive test coverage with new test routes and validation test suites. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant PydanticValidator
participant Handler
participant Response
Client->>Router: POST /endpoint with JSON body
Router->>Router: Detect Pydantic params in handler signature
Router->>PydanticValidator: validate_pydantic_body(model_class, body)
alt Validation Success
PydanticValidator-->>Router: (model_instance, None)
Router->>Handler: Call with validated Pydantic instance
Handler-->>Response: Process and return data
Response-->>Client: 200 OK
else Validation Failure
PydanticValidator-->>Router: (None, error_dict)
Router-->>Client: 422 Unprocessable Entity with error details
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
robyn/pydantic_support.py (2)
140-146: Consider including the full error detail in the exception message.The
PydanticBodyValidationErrorexception only includes the "error" key in its message. For debugging in logs, it might be helpful to include a summary of the detail.♻️ Optional: Include detail count in message
def __init__(self, error_detail: dict): self.error_detail = error_detail - super().__init__(error_detail.get("error", "Validation Error")) + detail = error_detail.get("detail", []) + detail_count = len(detail) if isinstance(detail, list) else 1 + super().__init__(f"{error_detail.get('error', 'Validation Error')} ({detail_count} error(s))")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@robyn/pydantic_support.py` around lines 140 - 146, Update PydanticBodyValidationError.__init__ to include the full error_detail (or a concise summary such as the number of errors plus a short snippet) in the exception message rather than only error_detail.get("error"); use the incoming error_detail dict to build the message (e.g., f"Validation Error: {error_detail}" or f"Validation Error ({len(error_detail.get('errors', []))}): {some_summary}") and pass that string to super().__init__ so logs contain the complete/summary information for easier debugging while still storing error_detail on the instance.
108-117: Edge case:_ValidationErrorcould beNonein except clause.If
_ensure_pydantic()runs but Pydantic isn't installed,_ValidationErrorremainsNone. Theexcept _ValidationError as e:clause would then raiseTypeError: catching classes that do not inherit from BaseException is not allowed.While this is prevented at route registration time by
check_pydantic_installed_for_handler, a defensive check would make the code more robust:🛡️ Suggested defensive check
def validate_pydantic_body(model_class, body: Any) -> Tuple[Any, Optional[dict]]: ... _ensure_pydantic() + if _ValidationError is None: + return None, {"detail": "Pydantic not installed", "error": "Configuration Error"} try: ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@robyn/pydantic_support.py` around lines 108 - 117, The except block assumes _ValidationError is a valid exception class; make it defensive by replacing the two separate except clauses with a single "except Exception as e:" and inside it branch: if _ValidationError is not None and isinstance(e, _ValidationError) return the sanitized validation response (use _sanitize_errors(e.errors())), otherwise return the generic invalid request body response (str(e)). Update the error-handling in the function that calls _ensure_pydantic() (the block containing the current "except _ValidationError as e:" and "except Exception as e:") to use this conditional isinstance check against _ValidationError to avoid catching a None value.integration_tests/test_pydantic.py (2)
5-10: Minor: Unused import and inline imports pattern.The
pydanticimport on line 6 is only used for checking availability. Consider usingimport pydantic as _pydanticto signal it's intentionally unused, or simplify to just the import statement without assignment.Also, the
import requestsstatements insidetest_pydantic_invalid_json,test_pydantic_put, andtest_pydantic_empty_bodycould be hoisted to the module level for consistency with other test files.♻️ Suggested refactor
try: - import pydantic + import pydantic as _pydantic # noqa: F401 _HAS_PYDANTIC = True except ImportError: _HAS_PYDANTIC = FalseAnd add
import requestsat the top of the file alongsidefrom integration_tests.helpers.http_methods_helpers import json_post.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration_tests/test_pydantic.py` around lines 5 - 10, Change the top-level pydantic import to make its unused status explicit (e.g., import pydantic as _pydantic) and keep the feature-detection boolean _HAS_PYDANTIC as-is; then hoist the inline imports of requests from inside test_pydantic_invalid_json, test_pydantic_put, and test_pydantic_empty_body to the module level (add import requests alongside the existing from integration_tests.helpers.http_methods_helpers import json_post) so all imports are consistent and no per-test inline imports remain.
80-82: Nitpick: Redundant.lstrip("/").The endpoint variable from the f-string already starts with
/, so.lstrip("/")works correctly, but the pattern is slightly verbose. Consider usingendpoint[1:]or defining the endpoint without the leading slash initially.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration_tests/test_pydantic.py` around lines 80 - 82, The endpoint string construction uses an unnecessary .lstrip("/") on endpoint; update the code that sets endpoint (the variable named endpoint built from f"/{function_type}/pydantic/user") to avoid the redundant call—either build endpoint without the leading slash (e.g., f"{function_type}/pydantic/user") or replace .lstrip("/") with a simple slice endpoint[1:] where the caller expects no leading slash; adjust the requests.post call to use the resulting endpoint variable as before.robyn/openapi.py (1)
318-337: Potential schema name collision when merging component schemas.The
component_schemas.update()call on line 321 will overwrite existing schemas with the same name. If two different Pydantic models have nested models with identical names but different definitions, the later one will silently overwrite the earlier one.Consider adding a warning or check for schema conflicts:
🛡️ Optional: Warn on schema name collision
if is_pydantic_model(request_body): schema, component_schemas = get_pydantic_openapi_schema(request_body) if component_schemas: + for name in component_schemas: + if name in self.openapi_spec["components"]["schemas"]: + existing = self.openapi_spec["components"]["schemas"][name] + if existing != component_schemas[name]: + import logging + logging.getLogger(__name__).warning( + f"OpenAPI schema '{name}' already exists with different definition" + ) self.openapi_spec["components"]["schemas"].update(component_schemas)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@robyn/openapi.py` around lines 318 - 337, The current merge in get_pydantic_openapi_schema handling overwrites existing component schemas via self.openapi_spec["components"]["schemas"].update(component_schemas), which can silently replace a different schema with the same name; modify the merge so you iterate each key in component_schemas, check if that key already exists in self.openapi_spec["components"]["schemas"], compare the existing definition with the new one, and on difference either log/warn (e.g., using self.logger.warning or warnings.warn) or disambiguate (e.g., rename the new schema key or raise an error) before inserting—change this logic inside the branch where is_pydantic_model(request_body) is handled (around get_pydantic_openapi_schema usage) to prevent silent collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@integration_tests/test_pydantic.py`:
- Around line 5-10: Change the top-level pydantic import to make its unused
status explicit (e.g., import pydantic as _pydantic) and keep the
feature-detection boolean _HAS_PYDANTIC as-is; then hoist the inline imports of
requests from inside test_pydantic_invalid_json, test_pydantic_put, and
test_pydantic_empty_body to the module level (add import requests alongside the
existing from integration_tests.helpers.http_methods_helpers import json_post)
so all imports are consistent and no per-test inline imports remain.
- Around line 80-82: The endpoint string construction uses an unnecessary
.lstrip("/") on endpoint; update the code that sets endpoint (the variable named
endpoint built from f"/{function_type}/pydantic/user") to avoid the redundant
call—either build endpoint without the leading slash (e.g.,
f"{function_type}/pydantic/user") or replace .lstrip("/") with a simple slice
endpoint[1:] where the caller expects no leading slash; adjust the requests.post
call to use the resulting endpoint variable as before.
In `@robyn/openapi.py`:
- Around line 318-337: The current merge in get_pydantic_openapi_schema handling
overwrites existing component schemas via
self.openapi_spec["components"]["schemas"].update(component_schemas), which can
silently replace a different schema with the same name; modify the merge so you
iterate each key in component_schemas, check if that key already exists in
self.openapi_spec["components"]["schemas"], compare the existing definition with
the new one, and on difference either log/warn (e.g., using self.logger.warning
or warnings.warn) or disambiguate (e.g., rename the new schema key or raise an
error) before inserting—change this logic inside the branch where
is_pydantic_model(request_body) is handled (around get_pydantic_openapi_schema
usage) to prevent silent collisions.
In `@robyn/pydantic_support.py`:
- Around line 140-146: Update PydanticBodyValidationError.__init__ to include
the full error_detail (or a concise summary such as the number of errors plus a
short snippet) in the exception message rather than only
error_detail.get("error"); use the incoming error_detail dict to build the
message (e.g., f"Validation Error: {error_detail}" or f"Validation Error
({len(error_detail.get('errors', []))}): {some_summary}") and pass that string
to super().__init__ so logs contain the complete/summary information for easier
debugging while still storing error_detail on the instance.
- Around line 108-117: The except block assumes _ValidationError is a valid
exception class; make it defensive by replacing the two separate except clauses
with a single "except Exception as e:" and inside it branch: if _ValidationError
is not None and isinstance(e, _ValidationError) return the sanitized validation
response (use _sanitize_errors(e.errors())), otherwise return the generic
invalid request body response (str(e)). Update the error-handling in the
function that calls _ensure_pydantic() (the block containing the current "except
_ValidationError as e:" and "except Exception as e:") to use this conditional
isinstance check against _ValidationError to avoid catching a None value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b662b9ab-0ab8-4093-a42d-58f2c9bfe2e9
📒 Files selected for processing (8)
integration_tests/base_routes.pyintegration_tests/test_openapi.pyintegration_tests/test_pydantic.pypyproject.tomlrobyn/openapi.pyrobyn/pydantic_support.pyrobyn/router.pyrobyn/types.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes #
Summary
This PR does....
PR Checklist
Please ensure that:
Pre-Commit Instructions:
Summary by CodeRabbit
Release Notes
New Features
Tests