fix(airplay): carry the HTTP status code in the error instead of re-parsing the message - #49
Open
nikhilshastry2003 wants to merge 1 commit into
Open
fix(airplay): carry the HTTP status code in the error instead of re-parsing the message#49nikhilshastry2003 wants to merge 1 commit into
nikhilshastry2003 wants to merge 1 commit into
Conversation
…arsing the message `post_stream` turned a failed status line into the text of an `AirPlayError::Negotiation`, and `run_session` then parsed the code back out of that text to decide whether to fall back into HAP pairing. #44 made that parse read the first line only, which fixed the header-digit false positives, but the design stayed: the message format was a contract between two functions that nothing enforced. Change the wording of the error and the retry silently stops. Now the code travels as a number. `AirPlayError::HttpStatus { request, code, status_line }` is what a non-success response becomes, and the retry decision (`should_retry_with_pairing`) matches on `code` with the same five statuses as before. A `Negotiation` error whose text happens to quote a status line is no longer a retry — there is no text to mis-read a number out of. The message a user sees is unchanged: `POST /stream failed: HTTP/1.1 404 Not Found`. The crate also had three status-line parsers — `status_is_success`, `status_code` in session.rs and the inline one in `hap_pairing::check_http_status`. There is one now, `http_session::status_code`, and the other two call it. The encrypted `POST /stream` check in `negotiate_with_auth` used `status_line.contains("200")`, the substring pattern #44 removed one layer up; it goes through `status_is_success` too. Tests: the auth-fallback tests keep every case from #44 (the real receiver statuses, the original two, the retryable-code-inside-a-header regressions) but now build the real error through `http_failure`, so they exercise the path production takes. New: a typed error carries the right code and status line, its message reads as before, an unreadable status line stays untyped, and a `Negotiation` error quoting `HTTP/1.1 404` does not retry. Verified: cargo fmt --all --check clean; cargo clippy -p openplay-airplay --all-targets --all-features -D warnings clean; cargo test -p openplay-airplay 71 + 27 passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The follow-up #44 named as worth doing on its own: "the status code is not carried through
AirPlayError::Negotiation, so there is nothing better to match on today."post_streamturned a failed status line into the text of aNegotiationerror, andrun_sessionparsed the code back out of that text to decide whether to fall back into HAP pairing. #44 made that parse read the first line only, which fixed the header-digit false positives, but the design stayed: the message format was a contract between two functions that nothing enforced. Reword the error and the retry silently stops.What changes
AirPlayError::HttpStatus { request, code, status_line }is what a non-success response becomes. The code travels as a number; the status line is kept verbatim for the human reading it. The message is unchanged:POST /stream failed: HTTP/1.1 404 Not Found.should_retry_with_pairing(&AirPlayError)replaceswants_authentication(&str)+status_code(&str)insession.rs. It matches oncode: 401 | 403 | 404 | 470 | 501— the same five statuses fix(airplay): stop refusing receivers by model string, and widen the auth retry #44 established, with its receiver table kept in the doc comment. ANegotiationerror whose text happens to quote a status line is no longer a retry, because there is no text for a number to be mis-read out of.http_session::status_is_success,session::status_code, and the inline one inhap_pairing::check_http_status.http_session::status_codeis now the only one;status_is_success, the newhttp_failureandcheck_http_statusall go through it. Behaviour matches the two production parsers (second whitespace token of the first line).POST /streamcheck innegotiate_with_authusedstatus_line.contains("200")— the substring pattern fix(airplay): stop refusing receivers by model string, and widen the auth retry #44 removed one layer up. It now goes throughstatus_is_successand produces the same typed error.Tests
The auth-fallback tests keep every case from #44 — the real receiver statuses (404 / 470 / 401), the original two (501 / 403), and each retryable-code-inside-a-header regression — but build the real error through
http_failure, so they exercise the path production takes rather than a hand-made string.New: a typed error carries the right code and status line; its
Displayreads as before; an unreadable status line stays an untypedNegotiation; aNegotiationerror quotingHTTP/1.1 404 Not Founddoes not retry;status_codereads only the first line.Verified
cargo fmt --all -- --checkcleancargo clippy -p openplay-airplay --all-targets --all-features -- -D warningscleancargo test -p openplay-airplay— 71 + 27 passed (was 66 + 27)Only
openplay-airplayis touched, so unlike #47 everything here was compiled and run locally; nothing is left to CI alone.