requests icon indicating copy to clipboard operation
requests copied to clipboard

Improve URL fragment handling in redirects

Open tboy1337 opened this issue 3 months ago • 1 comments

Summary

This PR refactors the URL fragment handling logic in SessionRedirectMixin to be more Pythonic and adds comprehensive test coverage for fragment preservation during HTTP redirects, as specified in RFC 7231 Section 7.1.2.

Changes Made

Code Refactoring

  • src/requests/sessions.py: Simplified fragment check from parsed.fragment == "" to not parsed.fragment for more idiomatic Python code
    • This change maintains the same behavior while being more concise and following Python best practices
    • The logic still correctly handles fragment preservation during redirects per RFC 7231 7.1.2

Test Coverage Enhancement

Added 5 new test cases in tests/test_requests.py to ensure robust fragment handling:

  1. test_fragment_preserved_when_redirect_has_no_fragment: Verifies that the original URL fragment is preserved when redirecting to a URL without a fragment
  2. test_fragment_replaced_when_redirect_has_new_fragment: Confirms that fragments are properly maintained through redirect chains
  3. test_fragment_with_no_initial_fragment: Ensures proper behavior when neither the original nor redirect URL contains a fragment
  4. test_fragment_multiple_redirects_preservation: Tests fragment preservation through multiple consecutive redirects (3+ hops)
  5. test_fragment_empty_string_handling: Validates correct handling of URLs ending with # (empty fragment string)

Why These Changes Matter

  • Code Quality: The refactored condition is more Pythonic and easier to read
  • Test Coverage: The new tests provide comprehensive coverage for various fragment scenarios, preventing regressions
  • RFC Compliance: Ensures requests properly implements RFC 7231 Section 7.1.2 for fragment preservation during redirects
  • Edge Cases: Covers important edge cases like empty fragments and multi-hop redirects

Testing

All new tests pass and verify the correct behavior of fragment handling across various redirect scenarios.

tboy1337 avatar Oct 21 '25 12:10 tboy1337

I should add that this issue was found because of I think (I'm not 100% sure because everything was also typed according to mypy rules in the commit) pylint in https://github.com/psf/requests/pull/7054, if it was pylint that found this issue and it is a legitimate issue then it makes a strong case for the project to start using it imo.

tboy1337 avatar Oct 22 '25 12:10 tboy1337