Reset content-type when sending request - #1241
Conversation
📝 WalkthroughWalkthrough
ChangesHTTP Header Management Fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@meilisearch/_httprequests.py`:
- Around line 50-53: The code mutates shared self.headers; instead create a
per-request copy (e.g., headers = self.headers.copy()) inside the HttpRequests
method that contains this snippet, set or pop "Content-Type" on that local
headers variable instead of self.headers, and pass that local headers into the
underlying HTTP call (requests.request/Session.request) so the global
self.headers is never mutated and concurrent requests cannot race.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d96d39b7-6be1-4f79-95dc-b38eab0a319d
📒 Files selected for processing (1)
meilisearch/_httprequests.py
There was a problem hiding this comment.
Hey @StephaneRob, thanks for your PR - sorry that the issue ended up in the Meilisearch repo instead of here 😅
This looks good; it's just missing finishing touches:
- Update
post_streamwith the same else branch - Add a unit test in
tests/client/test_http_requests.pyproving the header is reset
9fcfb24 to
3780318
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/client/test_http_requests.py (2)
33-43: ⚡ Quick winConsider mocking the HTTP request for better test isolation.
The test makes an actual HTTP request to the health endpoint but discards the response, testing only the side effect on
http.headers. Existing tests in this file are pure unit tests. Mockingrequests.getwould make this a true unit test, avoiding external dependencies and improving speed and reliability.♻️ Proposed refactor using unittest.mock
+import unittest.mock + import requests from meilisearch._httprequests import HttpRequests from meilisearch.config import Configdef test_reset_content_type_header(): """Tests that the content type header is reset when no content type is provided.""" config = Config(BASE_URL, MASTER_KEY, timeout=None) http = HttpRequests(config=config) http.headers["Content-Type"] = "application/json" assert http.headers["Content-Type"] == "application/json" - http.send_request(http_method=requests.get, path="health") + with unittest.mock.patch('requests.get') as mock_get: + mock_get.return_value.status_code = 200 + http.send_request(http_method=requests.get, path="health") assert "Content-Type" not in http.headers🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/client/test_http_requests.py` around lines 33 - 43, The test_reset_content_type_header test is making an actual HTTP request to the health endpoint, which creates an external dependency and reduces test isolation. Mock the requests.get function using unittest.mock.patch before the http.send_request call to prevent the actual HTTP request from being executed. This ensures the test remains a pure unit test focused solely on the header side effect without relying on external services.
33-43: ⚡ Quick winAdd test coverage for
post_streamContent-Type cleanup.Both
send_requestandpost_streamwere fixed to remove the Content-Type header when no content_type is provided. Consider adding a parallel test forpost_streamto validate the cleanup behavior in both methods.♻️ Suggested test for post_stream
def test_reset_content_type_header_post_stream(): """Tests that the content type header is reset in post_stream when content_type=None.""" config = Config(BASE_URL, MASTER_KEY, timeout=None) http = HttpRequests(config=config) http.headers["Content-Type"] = "application/json" assert http.headers["Content-Type"] == "application/json" with unittest.mock.patch('requests.post') as mock_post: mock_post.return_value.status_code = 200 http.post_stream(path="indexes", body={}, content_type=None) assert "Content-Type" not in http.headers🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/client/test_http_requests.py` around lines 33 - 43, Add a new test function to validate that the `post_stream` method properly removes the Content-Type header when no content_type is provided. Create a test that mirrors the existing `test_reset_content_type_header` function but calls `post_stream` instead of `send_request`. The test should set the Content-Type header to a value, call `post_stream` with `content_type=None`, and then assert that the Content-Type header has been removed from the http.headers dictionary. Use unittest.mock.patch to mock the requests.post call to avoid making actual HTTP requests during testing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/client/test_http_requests.py`:
- Around line 33-43: The test_reset_content_type_header test is making an actual
HTTP request to the health endpoint, which creates an external dependency and
reduces test isolation. Mock the requests.get function using unittest.mock.patch
before the http.send_request call to prevent the actual HTTP request from being
executed. This ensures the test remains a pure unit test focused solely on the
header side effect without relying on external services.
- Around line 33-43: Add a new test function to validate that the `post_stream`
method properly removes the Content-Type header when no content_type is
provided. Create a test that mirrors the existing
`test_reset_content_type_header` function but calls `post_stream` instead of
`send_request`. The test should set the Content-Type header to a value, call
`post_stream` with `content_type=None`, and then assert that the Content-Type
header has been removed from the http.headers dictionary. Use
unittest.mock.patch to mock the requests.post call to avoid making actual HTTP
requests during testing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c77bb851-ca6b-4003-bd94-6325fb1b8a02
📒 Files selected for processing (2)
meilisearch/_httprequests.pytests/client/test_http_requests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- meilisearch/_httprequests.py
3780318 to
bfedff8
Compare
bfedff8 to
fe20386
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/client/test_http_requests.py (1)
35-46: ⚡ Quick winAdd a companion regression test for
post_streamheader cleanup.This test correctly guards
send_request, but this PR also changespost_stream. Please add a parallel test to ensure omittedcontent_typeclears staleContent-Typethere too, so both modified paths are protected from regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/client/test_http_requests.py` around lines 35 - 46, Add a new regression test for the `post_stream` method to ensure it clears stale `Content-Type` headers when no content_type is provided, mirroring the existing `test_reset_content_type_header` test. Create a test function that follows the same pattern as `test_reset_content_type_header` (set the Content-Type header, verify it exists, call `http.post_stream` with appropriate parameters and without specifying content_type, then assert the header is removed) to ensure both the `send_request` and `post_stream` code paths are protected from regression.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/client/test_http_requests.py`:
- Around line 35-46: Add a new regression test for the `post_stream` method to
ensure it clears stale `Content-Type` headers when no content_type is provided,
mirroring the existing `test_reset_content_type_header` test. Create a test
function that follows the same pattern as `test_reset_content_type_header` (set
the Content-Type header, verify it exists, call `http.post_stream` with
appropriate parameters and without specifying content_type, then assert the
header is removed) to ensure both the `send_request` and `post_stream` code
paths are protected from regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d159b94-74a3-423e-af4c-78c44a0173a3
📒 Files selected for processing (2)
meilisearch/_httprequests.pytests/client/test_http_requests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- meilisearch/_httprequests.py
Pull Request
Related issue
Fixes meilisearch/meilisearch#6188
What does this PR do?
swap_indexesanddelete_index, the first request set content-type toapplication/jsonand the delete request use it with incorrectnullbody for this type.PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Changes
HttpRequests.send_request()andHttpRequests.post_stream()to remove any existingContent-Typeheader from a reusedHttpRequestsinstance when thecontent_typeparameter is falsy/omitted.test_reset_content_type_headerintests/client/test_http_requests.pyto verify the header is cleared after a request that doesn’t specify a content type.Rationale
When chaining multiple requests with the same HTTP client, a prior request (e.g., one that sets
Content-Type: application/jsonsuch asswap_indexes) could leave the header in place. A subsequent request without a body (e.g., deleting the temporary index) could be sent with a staleContent-Type, leading to server errors.Impact
Content-Typeheaders from being carried across sequential requests#6188where deleting an index right afterswap_indexescould return a 500 on remote Meilisearch instances