Set binary format in openapi spec#806
Conversation
Reviewer's GuideAdds OpenAPI schema support for binary file uploads to the artifact upload endpoint and introduces an integration-style test that exercises the full HTTP multipart upload path through Connexion validation to ensure artifacts are stored correctly. Sequence diagram for multipart artifact upload with OpenAPI binary formatsequenceDiagram
actor Client
participant ConnexionApp
participant ArtifactController
participant ArtifactStorage
Client->>ConnexionApp: POST /api/artifact/upload
activate ConnexionApp
ConnexionApp->>ConnexionApp: Validate request via OpenAPI schema
Note over ConnexionApp: field file type string<br/>format binary
ConnexionApp->>ArtifactController: Forward validated multipart data
deactivate ConnexionApp
activate ArtifactController
ArtifactController->>ArtifactStorage: Store file and metadata
ArtifactStorage-->>ArtifactController: Storage result
deactivate ArtifactController
ArtifactController-->>Client: 201 Created with artifact info
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
test_upload_artifact_via_http,response.textandresponse.json()rely on Flask test client internals; consider usingresponse.get_data(as_text=True)for debug output andresponse.get_json()for parsing to avoid attribute issues and keep it consistent with other tests. - The new HTTP upload test duplicates some setup logic from existing artifact upload tests (e.g., building headers, file content, and metadata); consider extracting a small helper or fixture to reduce repetition and make future changes to the upload contract easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_upload_artifact_via_http`, `response.text` and `response.json()` rely on Flask test client internals; consider using `response.get_data(as_text=True)` for debug output and `response.get_json()` for parsing to avoid attribute issues and keep it consistent with other tests.
- The new HTTP upload test duplicates some setup logic from existing artifact upload tests (e.g., building headers, file content, and metadata); consider extracting a small helper or fixture to reduce repetition and make future changes to the upload contract easier.
## Individual Comments
### Comment 1
<location> `backend/tests/controllers/test_artifact_controller.py:491-492` </location>
<code_context>
+ )
+
+ # Verify successful upload
+ assert response.status_code == 201, f"Response body: {response.text}"
+ response_data = response.json()
+ assert response_data["filename"] == "http-test.log"
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `response.text` in the failure message may not be reliable with the Flask test client
With the Flask test client, `response` often has `data`/`get_data()` but no `.text`, so this assertion can raise `AttributeError` when the status isn’t 201, obscuring the real failure. Use something like `f"Response body: {response.data!r}"` or `response.get_data(as_text=True)` instead so the failure message works reliably across environments.
```suggestion
# Verify successful upload
assert response.status_code == 201, f"Response body: {response.get_data(as_text=True)}"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR fixes the OpenAPI specification for file uploads in the artifact endpoint by adding the format: binary field specification. This ensures that the Connexion validation layer properly handles multipart file uploads.
Changes:
- Added
format: binaryto the file field in the/api/artifactPOST endpoint OpenAPI specification - Added a new test
test_upload_artifact_via_httpthat validates the file upload through actual HTTP requests, ensuring the OpenAPI schema is correct
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| backend/ibutsu_server/openapi/openapi.yaml | Added format: binary to the file field specification in the artifact upload endpoint to properly handle binary file uploads in multipart/form-data requests |
| backend/tests/controllers/test_artifact_controller.py | Added comprehensive HTTP-based test for artifact upload that validates the OpenAPI spec through Connexion's validation layer, complementing the existing test_request_context-based test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (73.40%) is below the target coverage (85.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #806 +/- ##
=======================================
Coverage 73.40% 73.40%
=======================================
Files 154 154
Lines 7557 7557
Branches 660 660
=======================================
Hits 5547 5547
Misses 1790 1790
Partials 220 220 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
07a6c45 to
8b73da0
Compare
8b73da0 to
09aa68a
Compare
Summary by Sourcery
Ensure artifact file uploads are correctly described and validated as binary data in the OpenAPI schema and covered by an end-to-end HTTP upload test.
Bug Fixes:
Tests: