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

[#25] Handle 304 redirect #264

Merged
merged 1 commit into from
Jan 18, 2023
Merged

[#25] Handle 304 redirect #264

merged 1 commit into from
Jan 18, 2023

Conversation

aeqz
Copy link
Contributor

@aeqz aeqz commented Jan 10, 2023

Description

Problem: after recent work on Xrefcheck redirect behavior, it remained to discuss how to handle 304 redirects.

Solution: with the current default configuration, 304 redirects are considered as valid, which seems appropriate taking into account that 304 responses usually mean that you previously received a successful response for that same request and there is no need to retransmit the resource again. We add a test case for this default config and update the FAQ section.

It is also related to conditional (e.g. If-None-Match and If-Modified-Since) and cache request headers. As Xrefcheck does not send them when performing HTTP requests, usually it should not receive 304 redirects.

Related issue(s)

Fixes #25

Related changes

  • Tests

    • If I added new functionality, I added tests covering it.
    • If I fixed a bug, I added a regression test to prevent the bug from
      silently reappearing again.
  • Documentation

    • I checked whether I should update the docs and did so if necessary:
  • Public contracts

    • Any modifications of public contracts comply with the Evolution
      of Public Contracts
      policy.
    • I added an entry to the changelog if my changes are visible to the users
      and
    • provided a migration guide for breaking changes if possible

Stylistic guide

@aeqz aeqz requested a review from Martoon-00 January 12, 2023 20:14
Copy link
Member

@Martoon-00 Martoon-00 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@aeqz aeqz force-pushed the aeqz/#25-304-redirect branch from 49ec69a to d7a7d90 Compare January 16, 2023 17:16
@aeqz aeqz changed the title [#25] Handle 304 redirects [#25] Handle 304 redirect Jan 16, 2023
Problem: After recent work on Xrefcheck redirect behavior, it remained
to discuss how to handle 304 redirects.

Solution: With the current default configuration, 304 redirects are
considered as valid, which seems appropriate taking into account that
304 responses usually mean that you previously received a successful
response for that same request and there is no need to retransmit the
resource again. We add a test case for this default config and update
the FAQ section.
@aeqz aeqz force-pushed the aeqz/#25-304-redirect branch from d7a7d90 to eccb6d0 Compare January 16, 2023 17:52
@aeqz aeqz merged commit 5d182c0 into master Jan 18, 2023
@aeqz aeqz deleted the aeqz/#25-304-redirect branch January 18, 2023 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report redirects as broken links
2 participants