ads-client: alphabetical ordering pass + fix an OHTTP preflight panic - #7578
Open
Almaju wants to merge 5 commits into
Open
ads-client: alphabetical ordering pass + fix an OHTTP preflight panic#7578Almaju wants to merge 5 commits into
Almaju wants to merge 5 commits into
Conversation
Mechanical reordering across the component: struct fields, enum variants, derive lists, trait items and inherent/trait impl items are now in alphabetical order, and rustfmt was re-run over every file touched. Derive lists are alphabetical with one exception - a derive follows the trait it extends, so the lists read in dependency order: `PartialEq, Eq`, `PartialOrd, Ord`, `Clone, Copy`. Path-qualified derives sort on their last segment, so `uniffi::Enum` sorts under `E`. No behavioural change: every moved item keeps its attributes and its leading comments, and no serde or uniffi wire format depends on declaration order.
The same ordering applied to struct literals, which the previous commit
left alone: reordering initializers is only safe when none of them
observes another. Checked by hand and reordered field-for-field at 17
sites:
- `HttpCacheBuilderError::{InvalidMaxSize,InvalidTtl}` in `validate`
- `HttpCacheStore` in `new` and `new_with_test_clock`
- `EffectiveTtl` in `NetworkFirst::apply` and its unit tests
- `viaduct::{Request,Response}` in the http_cache and mars::error tests
None of these initializers observes another, so this is a no-op at
runtime.
`From<PreflightResponse> for Headers` used `.expect("valid header")` on
`geo_location` and `normalized_ua`, both echoed verbatim out of the MARS
`/v1/ads-preflight` response body. `Headers::insert` rejects any value that
is not printable ASCII, so a preflight response carrying a non-ASCII geo
location — or CRLF — panicked inside the caller's process instead of
failing the request. Every OHTTP ad request and OHTTP click/impression/report
callback goes through this conversion.
It is now `TryFrom<PreflightResponse> for Headers` with
`Error = viaduct::ViaductError`. Both call sites in `MARSClient` already
return an error type that converts from `ViaductError` (`FetchAdsError` and
`CallbackRequestError`), so the failure propagates with `?` and surfaces to
the caller as a request error. Four unit tests cover the happy path, the
omitted-empty-UA path, non-ASCII, and CRLF injection.
The remaining `unwrap`/`expect`/`panic!` sites in this component were
audited at the same time. Three are `#[cfg(test)]`-gated and one — the
`path_segments_mut()` call in `Environment::into_url` — cannot fail because
every `base_url()` arm is an `https` URL, which `url` guarantees is
hierarchical. Only that last one is non-obvious from the code, so only it
gets a comment; no other code changes.
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
1 participant
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.
Alphabetical ordering pass over
components/ads-client, plus one bug it turned up.Bug:
From<PreflightResponse> for Headerscalled.expect()ongeo_locationandnormalized_ua, both echoed from the MARS/v1/ads-preflightresponse body. Header values must be printable ASCII, so a non-ASCII geo location or a CRLF panicked in the caller's process instead of failing the request — on every OHTTP ad request and click/impression/report callback. NowTryFrom<..., Error = viaduct::ViaductError>, propagating with?at both call sites.Ordering: fields, variants, derives, trait and impl items. Derives keep dependency order (
PartialEq, Eq). Stops at the FFI boundary: MARS wire shapes,uniffi::Recordfields and the telemetry callback interface are untouched.Pull Request checklist
From→TryFromis on a privatemars::preflighttype.fmtandclippy --all-targetsclean. Fork CI built NSS with the realviaduct/ohttpfeature and passed all 9 integration tests, includingtest_contract_tile_ohttp_prod— the path this changes.mars::preflight— happy path, empty UA, non-ASCII, CRLF.### Ads-Clientin v157.0.