Remove hardcoded rule for handling erroneous status codes differently

[thomas-zahner]

Closes #1661

[thomas-zahner]

So I tracked down the code that is responsible for the current behaviour. Turns out it has nothing to do with clap, default values or range parsing. Instead it seems like a design decision made in the past. Also it's really not much code that is responsible for that.

Basically, if a HTTP code is not explicitly accepted (via --accept or its default value) we use error_for_status_ref to handle erroneous status codes in the code.

Removing this explicit handling would also imply that we remove the Status::Redirected and Status::UnknownStatusCode variants.

Also I wasn't sure at first if the new variant should be added to the Status or ErrorKind struct. But since I think it should behave like a "normal" error I've added it to ErrorKind.

This means it's not counted like unknowns as a new category:

🔍 1 Total (in 0s) ✅ 0 OK 🚫 0 Errors ❓ 1 Unknown

But instead like normal errors:

$ echo 'https://httpstat.us/200' | cargo run - --accept 201 --verbose

   [ERROR] https://httpstat.us/200 | Rejected status code: 200 OK (this depends on your "accept" configuration)

Issues found in 1 input. Find details below.

[stdin]:
   [ERROR] https://httpstat.us/200 | Rejected status code: 200 OK (this depends on your "accept" configuration)

🔍 1 Total (in 0s) ✅ 0 OK 🚫 1 Error

@mre what are your thoughts on this. Is this approach okay with you? If so, we should of course update the documentation accordingly and mention the breaking changes in the release notes.

[mre]

I agree.

The rule should be that Status is strictly set by the checking logic and should directly relate to a website's status (as in "status code"). It's nothing that lychee should temper with.
The error part is mostly for expressing how we handle the status that comes back from the website. So, yeah, if a user defines that they don't want to accept a certain status code as "valid", that should surface as an error.

[thomas-zahner]

@mre The change has more implications than I initially expected. What do you think about the PR?

[thomas-zahner]

And not quite sure if this the optimal approach

[mre]

Yeah, not sure about this. Doesn't that mean: "if a user decided to reject a 429 Too Many Requests status code, you should still retry"? To me, that sounds like the user should be in control here; if they decided to consider 429s as hard errors, so be it. 😉 That should be the final answer, and we shouldn't retry. So I'd remove that condition and see where it breaks. (Unless I'm misunderstanding.)

[thomas-zahner]

So actually I think we should leave it like this. Otherwise it means that lychee will never retry requests when a 429 occurs. The reason I noticed it in the first place was because without these lines there are test failures. If we don't keep these two lines that would mean that --max-retries would now only apply to the above and below cases. (octocrab and reqwest errors) The reason for this is again the removal of response.error_for_status_ref as explained in my other comment. So previously we entered the if let Some(r) = self.reqwest_error() case.

[mre]

Sounds good. Can we add a comment to explain that behavior. I'm sure I'll forget why it's there otherwise. 😅

[mre]

Added some remarks. Overall, I like the approach and would go forward with this.

[mre]

Yeah, not sure about this. Doesn't that mean: "if a user decided to reject a 429 Too Many Requests status code, you should still retry"? To me, that sounds like the user should be in control here; if they decided to consider 429s as hard errors, so be it. 😉 That should be the final answer, and we shouldn't retry. So I'd remove that condition and see where it breaks. (Unless I'm misunderstanding.)

[mre]

Looks good. Can we just add one more cli test to ensure that --accept overrides the defaults? Like so?

#[tokio::test]
async fn test_accept_overrides_defaults_not_additive() -> Result<()> {
    let mock_server_200 = mock_server!(StatusCode::OK);
    
    let mut cmd = main_command();
    cmd.arg("--accept")
        .arg("404")  // ONLY accept 404 - should reject normally-accepted 200
        .arg("-")
        .write_stdin(mock_server_200.uri())
        .assert()
        .failure()
        .code(2)
        .stdout(contains(format!(
            r#"[200] {}/ | Rejected status code (this depends on your "accept" configuration): OK"#,
            mock_server_200.uri()
        )));

    Ok(())
}

[thomas-zahner]

Thanks for the test. For some reason I forgot about that, this makes a lot of sense!