Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure 304 status code does not throw, but returns None instead #460

Merged
merged 12 commits into from
Jan 29, 2025

Conversation

svrooij
Copy link
Contributor

@svrooij svrooij commented Jan 26, 2025

Fixes #363

Add handling for 304 responses without a location header in request adapter.

  • Modify packages/http/httpx/kiota_http/httpx_request_adapter.py to include a check for 304 status code in _should_return_none method.
    • Return True if the status code is 304 and there is no location header.
  • Add a unit test in packages/http/httpx/tests/test_httpx_request_adapter.py to verify that the request adapter returns null and does not throw for a 304 response without a location header.
    • Create a mock 304 response without a location header.
    • Ensure the unit test checks that the request adapter returns null for the 304 response.

For more details, open the Copilot Workspace session.

Fixes microsoft#363

Add handling for 304 responses without a location header in request adapter.

* Modify `packages/http/httpx/kiota_http/httpx_request_adapter.py` to include a check for 304 status code in `_should_return_none` method.
  * Return True if the status code is 304 and there is no location header.
* Add a unit test in `packages/http/httpx/tests/test_httpx_request_adapter.py` to verify that the request adapter returns null and does not throw for a 304 response without a location header.
  * Create a mock 304 response without a location header.
  * Ensure the unit test checks that the request adapter returns null for the 304 response.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/microsoft/kiota-python/issues/363?shareId=XXXX-XXXX-XXXX-XXXX).
@svrooij svrooij requested a review from a team as a code owner January 26, 2025 13:41

This comment was marked as outdated.

@baywet baywet mentioned this pull request Jan 28, 2025
Copy link
Contributor

Conflicts have been resolved. A maintainer will take a look shortly.

baywet
baywet previously approved these changes Jan 29, 2025
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making the changes!

Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

geee... finally. Python linting is quite strict.

@baywet baywet merged commit 1b0d8ac into microsoft:main Jan 29, 2025
53 checks passed
@svrooij
Copy link
Contributor Author

svrooij commented Jan 29, 2025

geee... finally. Python linting is quite strict.

Which is why I added the tasks to the dev container so you can run them locally 😄

@svrooij svrooij deleted the fix-3xx-responses branch January 29, 2025 13:13
@baywet
Copy link
Member

baywet commented Jan 29, 2025

geee... finally. Python linting is quite strict.

Which is why I added the tasks to the dev container so you can run them locally 😄

yeah I was too lazy for that, run the tool, push, let the CI tell me what's wrong while I do something else. A bit noisy for others though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done ✔️
Development

Successfully merging this pull request may close these issues.

ensure 3XX responses without location header do not throw
2 participants