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

Response code 304 should not be treated as a redirect #219

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mattnowzari
Copy link
Contributor

@mattnowzari mattnowzari commented Feb 21, 2025

Closes https://github.com/elastic/search-team/issues/9216

Response code 304 should be ignored (even thought it technically is a redirect) as it does not contain a location field in its response.

Some background around 304 response codes:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/304

Checklists

Pre-Review Checklist

  • This PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check crawler.yml.example and elasticsearch.yml.example)
  • This PR has a meaningful title
  • This PR links to all relevant GitHub issues that it fixes or partially addresses
    • If there is no GitHub issue, please create it. Each PR should have a link to an issue
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v0.1.0)

@navarone-feekery
Copy link
Collaborator

@mattnowzari what happens if the response is a 304 now?

@mattnowzari
Copy link
Contributor Author

@mattnowzari what happens if the response is a 304 now?

My hope is that it isn't getting treated as a redirect any longer, but I would like to do more concrete testing of this before saying this with certainty.

Truth be told, I feel like half the battle was understanding that a 304 doesn't have a location field in its response, but how to handle them is something I am not 100% confident on. 304 responses are technically redirects, so it somehow feels wrong to make an exception for them.

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

Successfully merging this pull request may close these issues.

2 participants