Forbid unknown attributes for request payload#479
Forbid unknown attributes for request payload#479tisnik merged 2 commits intolightspeed-core:mainfrom
Conversation
WalkthroughAdded strict validation to three request models by forbidding unknown fields and adjusting a UUID validator to accept None. Mirrored these constraints in OpenAPI by setting additionalProperties: false for corresponding schemas. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as API Endpoint
participant Model as Pydantic Model
participant Validator as Field Validators
Client->>API: POST FeedbackRequest
API->>Model: Instantiate with payload
Model->>Model: Enforce extra="forbid"
alt Unknown fields present
Model-->>API: ValidationError (extra fields)
API-->>Client: 422 Unprocessable Entity
else Fields known
Model->>Validator: check_uuid(value)
alt value is None
Note right of Validator: Skip UUID check
Validator-->>Model: OK
else Invalid UUID
Validator-->>Model: Raise error (quoted value)
Model-->>API: ValidationError
API-->>Client: 422 Unprocessable Entity
end
Model-->>API: Valid model
API-->>Client: 200/201 Success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/openapi.json (1)
1369-1473: Add upgrade note and negative test for forbidden extra fieldsConfirmed
additionalProperties=falseonAttachment,FeedbackRequest, andFeedbackStatusUpdateRequestindocs/openapi.json; this is a breaking change. Add an upgrade note to the release notes and a test asserting that sending an extra property onFeedbackRequestyields HTTP 422.
🧹 Nitpick comments (7)
docs/openapi.json (1)
1410-1415: Add minLength: 1 for user_feedback to match docs.Your code comment states “1-4096”, but schema only has maxLength. Consider enforcing minLength=1 via model Field (then regenerate OpenAPI).
src/models/requests.py (6)
16-17: Nit: fix grammar in user-facing docs.“can be send” → “can be sent”.
- """Model representing an attachment that can be send from the UI as part of query. + """Model representing an attachment that can be sent from the UI as part of a query.
288-294: Enforce 1–4096 length as documented.Currently only max_length=4096; allow empty strings. Add min_length=1.
- user_feedback: Optional[str] = Field( - default=None, - max_length=4096, + user_feedback: Optional[str] = Field( + default=None, + min_length=1, + max_length=4096, description="Feedback on the LLM response.", examples=["I'm not satisfied with the response because it is too vague."], )
341-345: Unify UUID error message style with QueryRequest.QueryRequest quotes the value in the error; this one doesn’t. Quote here for consistency and clearer logs.
- if not suid.check_suid(value): - raise ValueError(f"Improper conversation ID {value}") + if not suid.check_suid(value): + raise ValueError(f"Improper conversation ID '{value}'")
393-399: Fix example class name in docstring.Example shows FeedbackRequest inside FeedbackStatusUpdateRequest docs.
- feedback_request = FeedbackRequest( + feedback_request = FeedbackStatusUpdateRequest( status=false )
34-45: Optional: cap attachment size to prevent abuse.Consider a reasonable max_length on content (e.g., 1_000_000 chars) to avoid large inlined payloads.
- content: str = Field( + content: str = Field( + max_length=1_000_000, description="The actual attachment content", examples=["warning: quota exceeded"], )
241-337: Add tests for “extra fields not permitted” errors.Recommend unit tests for Attachment, FeedbackRequest, and FeedbackStatusUpdateRequest ensuring extra keys raise ValidationError, plus FastAPI route tests that return 422.
I can draft minimal pytest cases if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
docs/openapi.json(3 hunks)src/models/requests.py(6 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (7)
docs/openapi.json (3)
821-822: OpenAPI: Attachment now disallows unknown fields (good).This aligns the spec with Pydantic extra="forbid" and prevents silent acceptance of typos in payloads.
1448-1449: OpenAPI: FeedbackRequest now disallows unknown fields (good).Spec matches runtime validation and reduces client error ambiguity.
1519-1521: OpenAPI: FeedbackStatusUpdateRequest now disallows unknown fields (good).Consistent with model_config and expected 422 behavior on extra keys.
src/models/requests.py (4)
49-49: Runtime enforcement: extra="forbid" on Attachment (good).Prevents stray keys inside attachments; matches the OpenAPI change.
308-308: Runtime enforcement: extra="forbid" on FeedbackRequest (good).This closes a common source of payload typos.
407-409: Runtime enforcement: extra="forbid" on FeedbackStatusUpdateRequest (good).Makes status updates strict and predictable.
1-413: Strictness enforced on all Request models All*Requestclasses (QueryRequest, FeedbackRequest, FeedbackStatusUpdateRequest) definemodel_config.extra='forbid', so no request payload model accepts extra fields.
|
@CodeRabbit generate unit tests |
|
An unexpected error occurred during Unit Test Generation. |
✅ Actions performedUnit Test Generation started... |
Description
Forbid unknown attributes for request payload
Type of change
Summary by CodeRabbit
Bug Fixes
Documentation