Back/sc 4525 fix content type conflict in file uploads#28
Back/sc 4525 fix content type conflict in file uploads#28RandomGenericUsername wants to merge 3 commits into
Conversation
…file uploads Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 42 minutes and 59 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThree upload handlers ( ChangesContent-Type stripping for ZIP uploads
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
10ec295 to
616c8e7
Compare
jdavidagudelo
left a comment
There was a problem hiding this comment.
Tests working and no issues with the code.
gajaguar
left a comment
There was a problem hiding this comment.
Los hallazgos son oportunidades de mejora que no bloquean la funcionalidad. Se aprueba el PR.
| } | ||
| client = httpx.Client(follow_redirects=True) | ||
| return client.post(url=url, headers=headers, files=files) | ||
| upload_headers = {k: v for k, v in headers.items() if k.lower() != "content-type"} |
There was a problem hiding this comment.
Encapsulamiento: La misma expresión de filtrado de cabeceras aparece literalmente en cli/functions/handlers.py, cli/functions/pipelines.py y aquí. Extraerla a una pequeña función utilitaria (por ejemplo, strip_content_type(headers: dict) -> dict en un módulo compartido) concentra la regla en un solo lugar. Si algún día se necesita excluir más cabeceras, basta un único cambio en vez de tres ediciones coordinadas.
| assert "multipart/form-data" in content_type | ||
| assert "application/json" not in content_type | ||
|
|
||
| @respx.mock |
There was a problem hiding this comment.
Legibilidad: test_does_not_send_json_content_type y test_preserves_auth_header comparten un setup idéntico — URL, ruta mockeada, project_metadata y diccionario de entrada — duplicado verbatim. Extraer un fixture o helper compartido acorta ambos tests y reduce el riesgo de divergencia si cambia la firma de UploadFileStep.execute.
| assert route.called | ||
| assert route.calls.last.request.headers.get("x-auth-token") == "my-token" | ||
|
|
||
|
|
There was a problem hiding this comment.
Mantenibilidad: Este módulo tiene 1117 líneas y cubre 14 clases de prueba sin relación directa entre sí. A partir de las ~500 líneas, la navegación y el alcance de fallos se vuelven costosos. Considera dividirlo por agrupación funcional — por ejemplo, un archivo por paso o por dominio (test_upload_steps.py, test_invoke_steps.py) — siguiendo la estructura del módulo pipelines.py que testea.
| self.assertTrue(route.called) | ||
| content_type = route.calls.last.request.headers.get("content-type", "") | ||
| self.assertIn("multipart/form-data", content_type, "upload must use multipart/form-data") | ||
| self.assertNotIn("application/json", content_type, "upload must not send Content-Type: application/json") |
There was a problem hiding this comment.
Contrato: Comparar route.calls.last.request.headers contra un diccionario esperado completo elimina la variable intermedia, hace explícito el contrato total de cabeceras que debe enviar el upload, y convierte dos assertIn/assertNotIn parciales en una sola aserción que falla con un diferencial legible.
This PR addresses the issue reported at https://app.shortcut.com/ubidots/story/4525/ubidots-cli-file-upload-requests-fail-with-json-parse-error-due-to-content-type-conflict
Summary by CodeRabbit
Bug Fixes
Tests