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

fix: HTTP 308 Permanent Redirect status code handling #2389

Merged
merged 1 commit into from
May 17, 2023

Conversation

aucampia
Copy link
Member

Summary of changes

Change the handling of HTTP status code 308 to behave more like urllib.request.HTTPRedirectHandler, most critically, the new 308 handling will create a new urllib.request.Request object with the new URL, which will prevent state from being carried over from the original request.

One case where this is important is when the domain name changes, for example, when the original URL is http://www.w3.org/ns/adms.ttl and the redirect URL is https://uri.semic.eu/w3c/ns/adms.ttl. With the previous behaviour, the redirect would contain a Host header with the value www.w3.org instead of uri.semic.eu, because the Host header is placed in Request.unredirected_hdrs and takes precedence over the Host header in Request.headers.

Other changes:

  • Only handle HTTP status code 308 on Python versions before 3.11 as Python 3.11 will handle 308 by default [ref].

  • Move code which uses http://www.w3.org/ns/adms.ttl and http://www.w3.org/ns/adms.rdf out of test_guess_format_for_parse into a separate parameterized test which instead uses the embedded http server.

    This allows the test to fully control the Content-Type header in the response instead of relying on the value that the server is sending.

    This is needed because the server is sending Content-Type: text/plain for the adms.ttl file, which is not a valid RDF format, and the test is expecting Content-Type: text/turtle.

Fixes:

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
  • Added tests for any changes that have a runtime impact.
  • Checked that all tests and type checking passes.
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

Change the handling of HTTP status code 308 to behave more like
`urllib.request.HTTPRedirectHandler`, most critically, the new 308 handling will
create a new `urllib.request.Request` object with the new URL, which will
prevent state from being carried over from the original request.

One case where this is important is when the domain name changes, for example,
when the original URL is `http://www.w3.org/ns/adms.ttl` and the redirect URL is
`https://uri.semic.eu/w3c/ns/adms.ttl`. With the previous behaviour, the redirect
would contain a `Host` header with the value `www.w3.org` instead of
`uri.semic.eu`, because the `Host` header is placed in
`Request.unredirected_hdrs` and takes precedence over the `Host` header in
`Request.headers`.

Other changes:
- Only handle HTTP status code 308 on Python versions before 3.11 as Python 3.11
  will handle 308 by default [[ref](https://docs.python.org/3.11/whatsnew/changelog.html#id128)].
- Move code which uses `http://www.w3.org/ns/adms.ttl` and
  `http://www.w3.org/ns/adms.rdf` out of `test_guess_format_for_parse` into a
  separate parameterized test which instead uses the embedded http server.

  This allows the test to fully control the `Content-Type` header in the
  response instead of relying on the value that the server is sending.

  This is needed because the server is sending `Content-Type: text/plain` for
  the `adms.ttl` file, which is not a valid RDF format, and the test is
  expecting `Content-Type: text/turtle`.

Fixes:
- <RDFLib#2382>.
@aucampia aucampia marked this pull request as ready for review May 17, 2023 16:41
@coveralls
Copy link

Coverage Status

Coverage: 90.854% (+0.02%) from 90.83% when pulling 90071df on aucampia:iwana-20230512T1005-fix_test into cbd6151 on RDFLib:main.

@aucampia aucampia requested a review from a team May 17, 2023 16:41
@aucampia aucampia added review wanted This indicates that the PR is ready for review ready to merge The PR will be merged soon if no further feedback is provided. labels May 17, 2023
@aucampia
Copy link
Member Author

Merging this without reviews, as it is quite well-tested code, and the problem is breaking the CI.

Happy for post-merge feedback if any is forthcoming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge The PR will be merged soon if no further feedback is provided. review wanted This indicates that the PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants