diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9f16e9a..14b2196 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,18 +6,28 @@ on: pull_request: workflow_dispatch: +# Least privilege by default. The workflow runs on pull_request, and +# consumer-smoke plus the FetchContent builds clone and execute third-party +# code, so no job should inherit a writable token. A job that needs more grants +# itself a narrower block. +permissions: + contents: read + jobs: build-linux: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v7.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install dependencies run: | sudo apt-get update sudo apt-get install -y --no-install-recommends \ build-essential cmake pkg-config clang-format \ - libcurl4-openssl-dev + libcurl4-openssl-dev locales + # The LocaleIndependence tests need a comma-decimal locale; without + # one they GTEST_SKIP and the regression they guard goes unchecked. + sudo locale-gen de_DE.UTF-8 - name: Build run: make build CMAKE_ARGS=-DSPC_WARNINGS_AS_ERRORS=ON @@ -34,7 +44,7 @@ jobs: build-macos: runs-on: macos-latest steps: - - uses: actions/checkout@v7.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install dependencies run: | @@ -53,18 +63,19 @@ jobs: markdown-lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7.0.1 - - uses: DavidAnson/markdownlint-cli2-action@v23 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: DavidAnson/markdownlint-cli2-action@ded1f9488f68a970bc66ea5619e13e9b52e601cd # v23 sanitizer: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v7.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install dependencies run: | sudo apt-get update sudo apt-get install -y --no-install-recommends \ - build-essential cmake libcurl4-openssl-dev + build-essential cmake libcurl4-openssl-dev locales + sudo locale-gen de_DE.UTF-8 - name: Build with address and undefined behavior sanitizers run: >- cmake -S . -B build-sanitized @@ -78,12 +89,13 @@ jobs: thread-sanitizer: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v7.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install dependencies run: | sudo apt-get update sudo apt-get install -y --no-install-recommends \ - build-essential cmake libcurl4-openssl-dev + build-essential cmake libcurl4-openssl-dev locales + sudo locale-gen de_DE.UTF-8 - name: Build with ThreadSanitizer run: >- cmake -S . -B build-thread-sanitized @@ -97,7 +109,7 @@ jobs: clang-tidy: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v7.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install dependencies run: | sudo apt-get update @@ -120,10 +132,11 @@ jobs: consumer-smoke: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v7.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install dependencies run: | sudo apt-get update sudo apt-get install -y --no-install-recommends \ - build-essential cmake libcurl4-openssl-dev + build-essential cmake libcurl4-openssl-dev locales + sudo locale-gen de_DE.UTF-8 - run: ./tools/test_consumers.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a9dfd1..f65fb7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,141 @@ uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Changed + +- **HTTP 404 now says which kind of 404 it was.** `ErrorCode::FeedUnavailable` + ("no active outlook — clear the rows") is produced only by + `StaticFeedClient`, the one feed where SPC uses 404 that way. A 404 from the + ArcGIS MapServer or from IEM — a retired product, a renamed service path, a + wrong base URL — is now `ErrorCode::NotFound`, which was previously + unreachable. A logical ArcGIS `{"error":{"code":404}}` envelope, which is how + a renamed MapServer path is actually reported (over HTTP 200), maps to + `NotFound` as well. Consumers that branch on `is_feed_unavailable()` for + `ArcGISClient` or `ArchiveClient` results should treat `NotFound` as a fault + and alert on it instead of clearing rows. +- `Error::from_response` takes a trailing `Feed404` argument stating what a 404 + means for the feed that answered. It defaults to `Feed404::NotFound`, so + existing calls keep compiling and get the safe reading. + +### Fixed + +- **ArcGIS paging advanced `resultOffset` by the requested page size, not by + the records the server returned.** ArcGIS clamps `resultRecordCount` to the + layer's own `maxRecordCount`, so a truncated page can be shorter than the + 2000 requested; the offset then skipped the gap and the caller got a + successful result with a silent hole. `ArcGISPager::advance()` now takes the + returned record count. +- **`HttpClient` reached the local filesystem.** `is_absolute_url()` only + recognises `http://` and `https://`, so `file:///etc/hosts` was classified as + relative, appended to the empty default `base_url`, and handed to libcurl, + which read the file and returned it as the response body. The transport now + restricts libcurl to `http` and `https` on the request and on redirects, and + caps redirects at 10. +- `ClientConfig::max_response_bytes` (64 MB default) bounds a single response + body. An IEM archive window is caller-chosen and unbounded, and the body was + buffered whole, then parsed into a full JSON AST, then into the payload. +- Retry honours a `Retry-After` header on 429/503 (delta-seconds form, clamped + to `max_delay`) instead of retrying a server that asked for 60 s after + 200 ms. The header was already captured and then discarded. +- Retry jitter is applied before the `max_delay` clamp, not after, so a delay + can no longer exceed the documented ceiling by `jitter_factor`. A + `max_attempts` of 0 now performs the request once instead of returning a + fabricated "Max retry attempts exceeded" for a request never made. +- The default `User-Agent` is generated from `PROJECT_VERSION` instead of a + hard-coded literal, and a test fails if the two ever disagree. +- **Numeric-as-string probabilities were locale-dependent.** `std::stod` + delegates to `strtod`, which honours the process `LC_NUMERIC`. On a + comma-decimal host — any application that calls `setlocale(LC_ALL, "")` on a + de_DE / fr_FR / pt_BR desktop, as Qt and GTK apps do — `strtod("0.15")` + consumed only `"0"` and returned 0 without throwing. The live Day 4-8 static + feed carries its probability only as the string `"LABEL": "0.15"`, so the + feature was dropped by the `probability > 0.0` gate and + `StaticFeedClient::day4_8()` returned a successful, silently empty payload. + The same parse gated fire weather's no-risk sentinel filter, so a + `"Probability Too Low"` polygon shipped as a real band. Both now go through + a locale-independent `spc::detail::parse_double`, which uses + `std::from_chars` where the standard library provides it for `double` and a + classic-locale stream elsewhere (libc++ only implements floating-point + `from_chars` from version 20; the project's clang-tidy job builds against + libc++ 18). Output is unchanged in the C locale for every value in the + fixture corpus; the parse is narrower than `std::stod` only in rejecting + leading whitespace and a leading `+`, neither of which any SPC payload uses. +- **`RateLimiter` crashed on a zero `refill_interval` (SIGFPE).** + `RateLimiter::Config` is a public aggregate with no validation, so + `RateLimiter{{.refill_interval = 0ms}}` reached an integer division by zero + in `refill()` on the first `try_acquire()`. The constructor now clamps a + non-positive interval to the 1000 ms default, and clamps `initial_tokens` to + `max_tokens`. +- **`RateLimiter::acquire()` hung forever once a `daily_limit` was spent.** + With no `Config::max_wait` it polls `try_acquire()` every 10 ms, and + `try_acquire()` returns false permanently until the next UTC-midnight reset, + so the caller's thread spun until then. It now returns false immediately + when the daily quota is exhausted, since waiting cannot help. +- `ArchiveClient` interpolated `ts`, `sts`, `ets` and `wfo` into IEM query URLs + with no percent-encoding, while the ArcGIS path in the same file encoded + every value. `api.hpp` documents the timestamps as ISO 8601, which permits a + `+HH:MM` offset, and a raw `+` decodes server-side as a space — so an + offset-bearing timestamp silently queried a different window. An `&` in any + of the four injected extra query parameters. All four are now encoded. +- `ArchiveClient` now bounds its rate-limit wait (5 s) instead of blocking the + caller's thread indefinitely, which also makes the documented + `ErrorCode::RateLimited` result reachable, and acquires a token per retry + attempt rather than per call — `with_retry` re-issues up to 4 requests, and + retries precisely on 429/503, so one token was buying up to four requests + exactly when IEM was asking for less traffic. +- ArcGIS paging is bounded. A page that reports truncation while carrying no + records, and a server that never stops reporting truncation, now fail with + `ErrorCode::ServerError` after at most `ArcGISPager::max_pages()` (100) + requests instead of looping forever and growing memory without bound. + `ArcGISPager::offset()` is a `std::int64_t`, so the arithmetic cannot + overflow. + +### Removed + +- The two-argument `parse_fire_weather(body, day)` overload. It silently + assumed `FireWeatherLayer::Outlook`, so a dry-thunderstorm body decoded + `dn=5` as `"ELEV"` (severity 1) instead of `"IDRT"` (severity 0) — the label + confusion 0.2.0 fixed, still reachable through the public API. The captured + day-1 and day-2 payloads carry no LABEL at all, only the shared numeric + `dn`, so nothing in a body says which layer produced it. Pass the layer + explicitly: `parse_fire_weather(body, day, FireWeatherLayer::Outlook)` + restores the old behaviour where that was in fact the right layer. + +- Release builds no longer default to `-march=x86-64-v3`. The probe only + proved the *compiler* accepted the flag, never that the run host has + AVX2/BMI2/FMA — and this is an installable SDK, so the build host and the run + host are routinely different. `-mtune=generic` is the default; set + `SPC_TUNE_X86_64_V3=ON` to opt in to the non-portable artifact. +- The Esri-vs-GeoJSON parity gate now reads every captured fixture pair (three + categorical, four probabilistic). The test named for probabilistic parity + only ever opened the GeoJSON side, so `parse_esri_rings` was pinned by one + categorical layer. +- The Linux CI jobs generate `de_DE.UTF-8`, so the locale regression tests run + there instead of skipping. +- `ci.yml` declares `permissions: contents: read` at the top level — it runs on + `pull_request` and executes third-party build scripts — and pins both actions + to full commit SHAs instead of mutable tags. +- `CLAUDE.md` and `CONTRIBUTING.md` list the `fixtures-check` and `lint-md` + gates that CI enforces, `CONTRIBUTING.md` names all seven CI jobs, `make help` + lists every target, and the README documents `src/core/`, `query_layer`, and + the `JSON library: Glaze (divergence note)` heading the CHANGELOG points at. + +### Deprecated + +- `ArcGISClient::query_storm_reports()`. The SPC MapServer has no Local Storm + Report layer, so the method always failed without touching the network — it + now carries the attribute and doc comment its sibling + `query_active_watches()` already had. Use `ArchiveClient::storm_reports()`. + +### Added + +- `ArcGISClient::query_fire_weather()` documents its all-or-nothing contract: + it merges two layers, and a failure on either discards both. +- `Error::from_arcgis`, for ArcGIS logical failure envelopes. It keeps the + ArcGIS code in `Error::http_status` only while that code is HTTP-shaped + (100..599) and records it in `Error::detail`, so a code such as 1000 can no + longer masquerade as an HTTP status. + ## [0.2.0] - 2026-09-03 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index dee2185..c0b3c0f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,6 +6,8 @@ make build make test make lint +make fixtures-check # fixture-provenance gate; CI fails on a stale SHA256SUMS +make lint-md # markdown-lint gate over **/*.md make test-consumers python3 tools/verify_arcgis_metadata.py # live, opt-in ``` @@ -47,3 +49,11 @@ Glaze 8.3 parses loose SPC JSON. GoogleTest 1.18 runs the unit suite. The tag must match `project(spc-cpp VERSION ...)`. A `vX.Y.Z` tag triggers the release workflow, which reads the matching `CHANGELOG.md` section. + +The default `ClientConfig::user_agent` is generated from `PROJECT_VERSION` via +`SPC_VERSION_STRING`, so a bump carries automatically for anything linking the +targets, and `HttpClientLifecycle.DefaultUserAgentCarriesTheProjectVersion` +fails if that stops being true. Also bump the `#ifndef SPC_VERSION_STRING` +fallback literal in `include/spc/http_client.hpp`: it only applies to a +consumer that includes the header without linking the target, so no test +covers it. diff --git a/CMakeLists.txt b/CMakeLists.txt index 821bab5..c39f590 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -14,6 +14,8 @@ option(SPC_BUILD_TESTS "Build tests" ON) option(SPC_BUILD_EXAMPLES "Build examples" ON) option(SPC_ENABLE_LTO "Enable Link Time Optimization" ON) option(SPC_NATIVE_ARCH "Use -march=native for CPU-specific tuning" OFF) +option(SPC_TUNE_X86_64_V3 + "Build Release with -march=x86-64-v3 (AVX2/BMI2/FMA). Makes the artifact non-portable." OFF) option(SPC_ENABLE_CLANG_TIDY "Run clang-tidy while compiling project targets" OFF) option(SPC_WARNINGS_AS_ERRORS "Treat project warnings as errors" OFF) @@ -62,14 +64,24 @@ if(NOT MSVC) if(SPC_NATIVE_ARCH) set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -march=native") message(STATUS "Using -march=native for CPU-specific optimizations") - else() + elseif(SPC_TUNE_X86_64_V3) + # Opt-in only. check_cxx_compiler_flag proves the *compiler* accepts + # the flag, never that the *run* host has AVX2/BMI2/FMA -- and this is + # an installable SDK (install(EXPORT) + find_package(spc)), so the + # build host and the run host are routinely different. Defaulting this + # on made every stock Release build SIGILL on pre-Haswell / pre-Zen + # hardware and inside VMs that do not expose AVX2. include(CheckCXXCompilerFlag) check_cxx_compiler_flag("-march=x86-64-v3" COMPILER_SUPPORTS_X86_64_V3) if(COMPILER_SUPPORTS_X86_64_V3) set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -march=x86-64-v3") + message(STATUS "Release builds target x86-64-v3; the artifact is not portable") else() + message(WARNING "SPC_TUNE_X86_64_V3 requested but the compiler rejects the flag") set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -mtune=generic") endif() + else() + set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -mtune=generic") endif() endif() diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 600903d..a3db55c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,11 +15,15 @@ cd spc-cpp sudo apt install -y build-essential cmake clang-format \ libcurl4-openssl-dev -make build # CMake configure + Release build -make test # Run unit tests (ctest) -make lint # clang-format --dry-run + cpp_auto_audit +make build # CMake configure + Release build +make test # Run unit tests (ctest) +make lint # clang-format --dry-run + cpp_auto_audit +make fixtures-check # fixture-provenance gate (stale SHA256SUMS fails CI) +make lint-md # markdown-lint gate over **/*.md ``` +`make help` lists every target. + ## Code style - `.clang-format` (LLVM base, **tabs**, width 4, 100-col). `make @@ -45,8 +49,9 @@ their own product-specific severity mappers. ## PRs -- Branch, push, open a PR against `main`. CI (linux + macos + - markdown-lint) must be green. +- Branch, push, open a PR against `main`. Every CI job must be green: + `build-linux`, `build-macos`, `markdown-lint`, `sanitizer`, + `thread-sanitizer`, `clang-tidy` and `consumer-smoke`. - Conventional-commit subject lines. - Update `CHANGELOG.md` under `[Unreleased]`. - A maintainer merges with a **merge commit** (history reachability diff --git a/Makefile b/Makefile index 877d86b..5741ba8 100644 --- a/Makefile +++ b/Makefile @@ -108,9 +108,15 @@ help: @echo " make build - Configure and build the SDK (Release)" @echo " make debug - Configure and build the SDK (Debug)" @echo " make test - Run tests" + @echo " make test-consumers - Installed-package + FetchContent consumer checks" @echo " make lint - Check formatting + cpp_auto_audit" + @echo " make lint-md - Markdown lint (CI gate)" @echo " make format - Format code in place" + @echo " make format-md - Fix markdown lint findings in place" + @echo " make fixtures-check - Verify fixture provenance (CI gate)" @echo " make coverage - Generate code coverage report (requires lcov)" + @echo " make install-hooks - Install the pre-commit hook" + @echo " make pre-commit - Format, then lint" @echo " make clean - Remove build artifacts" @echo " make help - Show this help" @echo "" diff --git a/README.md b/README.md index 49dec2b..1c074c0 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,14 @@ publishes CAP and WFO polygons, but it omits the SPC parameters represented by `ArcGISClient::query_active_watches()` method is deprecated and returns `InvalidRequest` instead of fabricating incomplete watches. +Storm reports are the same story: the SPC MapServer publishes no Local Storm +Report layer, so `ArcGISClient::query_storm_reports()` is deprecated and always +fails. Use `ArchiveClient::storm_reports()`. + +For anything the typed methods do not cover, +`ArcGISClient::query_layer(service, layer_id, params)` runs a raw paged query +against any of the three NOAA MapServers and returns the page bodies. + ### Custom networking `HttpClient` is the default GET transport. Implement `HttpTransport` and pass @@ -94,6 +102,7 @@ target_link_libraries(myapp PRIVATE spc::spc) | --- | --- | | `include/spc/` | Public clients, models, errors, and geometry helpers | | `src/api/` | Static feed, ArcGIS, and IEM routing | +| `src/core/` | Errors, geometry, and the rate limiter | | `src/models/` | Glaze-backed GeoJSON and Esri parsers | | `src/http/` | libcurl transport | | `tests/` | Public client tests and captured parser fixtures | @@ -117,6 +126,8 @@ The normal unit suite does not depend on NOAA availability. The metadata command compares the live ArcGIS layer IDs and names with the checked 2026-09-03 contract, then queries all 39 feature layers. +### JSON library: Glaze (divergence note) + SPC payloads vary in key case, numeric representation, and geometry type. The parsers use Glaze 8.3's generic JSON tree to handle those shapes. The original convective parser stays aligned with the downstream `spc-data` service; new diff --git a/include/spc/api.hpp b/include/spc/api.hpp index 6f67f28..45e4945 100644 --- a/include/spc/api.hpp +++ b/include/spc/api.hpp @@ -93,6 +93,11 @@ class ArcGISClient { [[nodiscard]] Result query_conditional_intensity(std::int32_t day, const std::string& hazard); [[nodiscard]] Result query_day4_8(std::int32_t day); + /// Merges both published fire-weather layers for `day`. All-or-nothing: if + /// either layer fails, nothing is returned, including the features already + /// parsed from the other. `FireWeatherPayload` cannot represent a partial + /// result, so a caller that needs one layer independently should use + /// `query_layer(ArcGISService::FireWeather, ...)`. [[nodiscard]] Result query_fire_weather(std::int32_t day); /// NOAA's WWA polygons do not contain the SPC watch parameters represented /// by WatchPayload. Use ArchiveClient::watches() for active SPC watches. @@ -100,7 +105,11 @@ class ArcGISClient { "use ArchiveClient::watches() for SPC watch data")]] [[nodiscard]] Result query_active_watches(); [[nodiscard]] Result query_active_md(); - [[nodiscard]] Result query_storm_reports(); + /// The SPC MapServer publishes no Local Storm Report layer, so this always + /// fails without touching the network. Use `ArchiveClient::storm_reports()`. + [[deprecated("use ArchiveClient::storm_reports(); the SPC MapServer has no LSR " + "layer")]] [[nodiscard]] Result + query_storm_reports(); /// Raw paged query against one of the documented NOAA MapServers. [[nodiscard]] Result> diff --git a/include/spc/error.hpp b/include/spc/error.hpp index e693e46..5d9058a 100644 --- a/include/spc/error.hpp +++ b/include/spc/error.hpp @@ -13,11 +13,16 @@ enum class ErrorCode { NetworkError, RateLimited, ServerError, + /// A genuine fault: a bad URL, a retired product, a renamed MapServer + /// path, or a wrong ArcGIS layer id. Every 404 that is not an SPC static + /// feed lands here — including a logical ArcGIS `{"error":{"code":404}}` + /// envelope, which is how a retired MapServer path is reported. NotFound, /// SPC returns HTTP 404 for "no active outlook" (e.g. overnight day-1 /// probabilistic). This is a normal, expected state — distinct from a /// genuine `NotFound` (bad URL / retired product). Consumers treat it as - /// "clear the rows for this day/hazard", not as an error. + /// "clear the rows for this day/hazard", not as an error. Only + /// `StaticFeedClient` produces it: see `Feed404`. FeedUnavailable, InvalidRequest, ParseError, @@ -49,10 +54,28 @@ enum class ErrorCode { return "Unknown"; } +/// How a transport-level HTTP 404 is to be read for the feed that answered. +/// +/// `Error::from_response` cannot tell a "no active outlook" 404 from a +/// retired endpoint by looking at the status alone, so the caller — which +/// knows which host it addressed — states the semantics explicitly. +enum class Feed404 : std::uint8_t { + /// A 404 is a fault (bad URL / retired product) -> `ErrorCode::NotFound`. + /// The safe default: every feed except the SPC static products. + NotFound, + /// SPC's static `.nolyr.geojson` products answer 404 with an HTML page + /// when nothing is issued -> `ErrorCode::FeedUnavailable`. + NoActiveOutlook, +}; + /// Error information returned by SDK operations. struct Error { ErrorCode code; std::string message; + /// Transport HTTP status. For a logical ArcGIS failure (reported over + /// HTTP 200) this carries the ArcGIS error code when that code is in the + /// 100..599 HTTP range, and 0 otherwise — ArcGIS also uses codes such as + /// 1000 that are not HTTP statuses. See `from_arcgis`. int http_status{0}; std::string detail; @@ -94,9 +117,19 @@ struct Error { return {ErrorCode::InvalidRequest, std::move(msg), 0, ""}; } - /// Create an Error from an HTTP response status code and body. SPC's 404 - /// "no active outlook" maps to `FeedUnavailable` (not `NotFound`). - [[nodiscard]] static Error from_response(int status, const std::string& body); + /// Create an Error from an HTTP response status code and body. + /// + /// `semantics` decides what a 404 means for the feed that answered: + /// `Feed404::NoActiveOutlook` (SPC static products only) yields + /// `FeedUnavailable`; the default `Feed404::NotFound` yields `NotFound`. + [[nodiscard]] static Error from_response(int status, const std::string& body, + Feed404 semantics = Feed404::NotFound); + + /// Create an Error from an ArcGIS logical failure envelope + /// (`{"error":{"code":...,"message":...}}`), which the MapServer reports + /// over HTTP 200. A code of 404 — how a renamed or retired service path + /// is reported — is a genuine `NotFound`, never `FeedUnavailable`. + [[nodiscard]] static Error from_arcgis(int arcgis_code, const std::string& body); }; /// Result type for SDK operations. diff --git a/include/spc/http_client.hpp b/include/spc/http_client.hpp index 3e07998..55c677d 100644 --- a/include/spc/http_client.hpp +++ b/include/spc/http_client.hpp @@ -2,7 +2,14 @@ #include "spc/error.hpp" +/// Set from `PROJECT_VERSION` by the build. The fallback keeps the header +/// usable when it is read outside the project's own CMake targets. +#ifndef SPC_VERSION_STRING +#define SPC_VERSION_STRING "0.2.0" +#endif + #include +#include #include #include #include @@ -26,9 +33,14 @@ struct ClientConfig { /// URLs — one client then serves spc.noaa.gov, the ArcGIS MapServer, /// and the IEM archive interchangeably (spc-data's fetcher behavior). std::string base_url; - std::string user_agent{"spc-cpp/0.2.0 (contact@predictioncast.ai)"}; + std::string user_agent{"spc-cpp/" SPC_VERSION_STRING " (contact@predictioncast.ai)"}; std::chrono::seconds timeout{15}; bool verify_ssl{true}; + /// Hard ceiling on a single response body. A wide IEM archive window is + /// unbounded by construction — the caller chooses the date range — and the + /// body is buffered whole, then parsed into a full JSON AST, then into the + /// payload. Exceeding this aborts the transfer with a network error. + std::size_t max_response_bytes{64UL * 1024UL * 1024UL}; }; /// GET transport boundary used by the high-level clients. @@ -58,8 +70,11 @@ class HttpClient final : public HttpTransport { HttpClient(const HttpClient&) = delete; HttpClient& operator=(const HttpClient&) = delete; - /// GET `path`. If `path` is an absolute URL (starts with http) it is - /// used verbatim; otherwise it is appended to `config().base_url`. + /// GET `path`. If `path` is an absolute `http://` or `https://` URL it is + /// used verbatim; otherwise it is appended to `config().base_url`. Only + /// those two schemes are accepted — every other scheme (`file://`, + /// `dict://`, `scp://`, ...) is refused by the transport, on the request + /// and on any redirect. [[nodiscard]] Result get(std::string_view path) const override; [[nodiscard]] const ClientConfig& config() const noexcept; diff --git a/include/spc/models/common.hpp b/include/spc/models/common.hpp index 58b5034..99649c4 100644 --- a/include/spc/models/common.hpp +++ b/include/spc/models/common.hpp @@ -15,6 +15,7 @@ #include "spc/types.hpp" +#include #include #include #include @@ -34,6 +35,26 @@ const Json* lookup(const Json& obj, const char* key); /// String value of `obj[key]`, or "" if absent / null / non-string. std::string json_string(const Json& obj, const char* key); +/// Outcome of `parse_double`. +struct ParsedNumber { + double value = 0.0; ///< meaningful only when `ok` + std::size_t consumed = 0; ///< characters the number occupied + bool ok = false; +}; + +/// Parse the leading decimal number of `text` **locale-independently**, and +/// report how many characters it consumed. +/// +/// `std::stod` delegates to `strtod`, which honours the process `LC_NUMERIC`: +/// on a comma-decimal host (any app that calls `setlocale(LC_ALL, "")` on a +/// de_DE / fr_FR / pt_BR desktop) `strtod("0.15")` stops at the '.' and +/// returns 0. SPC publishes probabilities as numeric strings, so that turned +/// a live outlook into a successful, silently empty payload. +/// +/// Narrower than `std::stod` by design: leading whitespace and a leading '+' +/// are rejected. No SPC payload uses either form. +ParsedNumber parse_double(std::string_view text); + /// SPC ships `LABEL` as either a string ("SLGT", "5") or a number (5). /// Always returns a numeric view; non-numeric / missing yields 0. double json_number_or_numeric_string(const Json& obj, const char* key); diff --git a/include/spc/models/fire_weather.hpp b/include/spc/models/fire_weather.hpp index 45b49d6..96bf495 100644 --- a/include/spc/models/fire_weather.hpp +++ b/include/spc/models/fire_weather.hpp @@ -44,13 +44,14 @@ struct FireWeatherPayload { /// (IDRT/SDRT) and anything else map to 0 (kept by label, no severity). [[nodiscard]] std::uint8_t fire_severity_from_label(std::string_view label) noexcept; -/// Parse a fire-weather outlook (ArcGIS Esri or static GeoJSON). Throws +/// Parse one known ArcGIS fire-weather layer (Esri or GeoJSON body). Throws /// std::runtime_error on malformed JSON. -[[nodiscard]] FireWeatherPayload parse_fire_weather(std::string_view body, std::int32_t day); - -/// Parse one known ArcGIS fire-weather layer. Day 1 and 2 use the layer kind -/// to disambiguate numeric `dn` codes shared by categorical and dry-thunder -/// products. Day 3 through 8 decode LABEL/label/dn as probabilities. +/// +/// `layer` is required and is not inferable from the body: NOAA's day-1 and +/// day-2 payloads carry no LABEL at all, only a numeric `dn` band index that +/// the outlook (5=ELEV, 8=CRIT, 10=EXTM) and dry-thunderstorm (5=IDRT, +/// 8=SDRT) products both use with different meanings. Day 3 through 8 decode +/// LABEL/label/dn as probabilities. [[nodiscard]] FireWeatherPayload parse_fire_weather(std::string_view body, std::int32_t day, FireWeatherLayer layer); diff --git a/include/spc/pagination.hpp b/include/spc/pagination.hpp index 75df3c2..c8bdc13 100644 --- a/include/spc/pagination.hpp +++ b/include/spc/pagination.hpp @@ -9,7 +9,13 @@ namespace spc { /// An ArcGIS layer query caps the result set (commonly 1000-2000 records) /// and signals truncation with `"exceededTransferLimit": true` in the /// response envelope. To page, re-issue with `resultOffset` advanced by the -/// page size until the server stops reporting truncation. +/// number of records the server actually returned until it stops reporting +/// truncation. +/// +/// The requested `resultRecordCount` is only a hint: ArcGIS clamps it to the +/// layer's own `maxRecordCount`, so a truncated page can be shorter than the +/// page size. Advancing by the request size would then skip every record in +/// the gap, which is why `advance()` requires the returned record count. /// /// SPC outlook layers are tiny (single-digit feature counts), so paging /// rarely triggers — but storm-report / LSR layers can exceed the cap, and @@ -18,36 +24,57 @@ namespace spc { class ArcGISPager { public: /// `page_size` is the `resultRecordCount` passed per request. ArcGIS's - /// hard server max is typically 2000; that is the default. - explicit ArcGISPager(std::int32_t page_size = 2000) - : page_size_(page_size > 0 ? page_size : 2000) {} + /// hard server max is typically 2000; that is the default. `max_pages` + /// bounds a server (or caching proxy) that keeps reporting truncation + /// without advancing — see `page_limit_reached()`. + // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) + explicit ArcGISPager(std::int32_t page_size = 2000, std::int32_t max_pages = 100) + : page_size_(page_size > 0 ? page_size : 2000), + max_pages_(max_pages > 0 ? max_pages : 100) {} [[nodiscard]] std::int32_t page_size() const noexcept { return page_size_; } + /// Hard ceiling on the number of pages fetched for one query. + [[nodiscard]] std::int32_t max_pages() const noexcept { return max_pages_; } + /// Current `resultOffset` to send on the next request. - [[nodiscard]] std::int32_t offset() const noexcept { return offset_; } + [[nodiscard]] std::int64_t offset() const noexcept { return offset_; } /// Whether another page should be fetched. Seeded `true`; the caller /// passes the response's `exceededTransferLimit` after each page. [[nodiscard]] bool has_more() const noexcept { return has_more_; } - /// Record the just-fetched page: advance the offset by `page_size` and - /// keep going only while the server still reports truncation. - void advance(bool exceeded_transfer_limit) noexcept { - offset_ += page_size_; - has_more_ = exceeded_transfer_limit; + /// True when paging stopped at `max_pages` while the server was still + /// reporting truncation — a non-converging server, not a complete result. + [[nodiscard]] bool page_limit_reached() const noexcept { + return server_truncated_ && pages_fetched_ >= max_pages_; + } + + /// Record the just-fetched page: advance the offset by the records the + /// server returned, and keep going only while it still reports truncation + /// and the page ceiling has not been reached. + void advance(std::int32_t records_returned, bool exceeded_transfer_limit) noexcept { + offset_ += records_returned > 0 ? records_returned : 0; + ++pages_fetched_; + server_truncated_ = exceeded_transfer_limit; + has_more_ = exceeded_transfer_limit && pages_fetched_ < max_pages_; } /// Reset to the first page. void reset() noexcept { offset_ = 0; + pages_fetched_ = 0; has_more_ = true; + server_truncated_ = false; } private: std::int32_t page_size_; - std::int32_t offset_{0}; + std::int32_t max_pages_; + std::int64_t offset_{0}; + std::int32_t pages_fetched_{0}; bool has_more_{true}; + bool server_truncated_{false}; }; } // namespace spc diff --git a/include/spc/rate_limit.hpp b/include/spc/rate_limit.hpp index ded2213..6227051 100644 --- a/include/spc/rate_limit.hpp +++ b/include/spc/rate_limit.hpp @@ -18,9 +18,11 @@ namespace spc { class RateLimiter { public: struct Config { - std::uint16_t max_tokens = 2; ///< default: gentle on IEM - std::chrono::milliseconds refill_interval{1000}; ///< 1 token / sec - std::uint16_t initial_tokens = 2; + std::uint16_t max_tokens = 2; ///< default: gentle on IEM + /// Time per token. Clamped to 1000 ms if set to zero or less. + std::chrono::milliseconds refill_interval{1000}; + std::uint16_t initial_tokens = 2; ///< clamped to `max_tokens` + /// Longest `acquire()` may block. Unset means block indefinitely. std::optional max_wait; std::int32_t daily_limit{0}; ///< 0 = no daily cap }; @@ -28,6 +30,13 @@ class RateLimiter { explicit RateLimiter(Config config); [[nodiscard]] bool try_acquire() noexcept; + + /// Block until a token is available. With `Config::max_wait` set this is + /// `acquire_for(*max_wait)`; without it the wait is unbounded, so a + /// synchronous caller can stall for as long as the bucket stays empty. + /// Returns false only on a bounded wait that expired, or when a + /// configured `daily_limit` is spent — no amount of waiting clears that + /// before the next UTC day, so it fails immediately rather than spinning. [[nodiscard]] bool acquire(); [[nodiscard]] bool acquire_for(std::chrono::milliseconds max_wait); [[nodiscard]] std::uint16_t available_tokens() const noexcept; @@ -38,6 +47,7 @@ class RateLimiter { private: void refill() noexcept; void check_daily_reset() noexcept; + [[nodiscard]] bool daily_quota_exhausted() noexcept; Config config_; mutable std::mutex mutex_; diff --git a/include/spc/retry.hpp b/include/spc/retry.hpp index 6ecc881..915c862 100644 --- a/include/spc/retry.hpp +++ b/include/spc/retry.hpp @@ -4,10 +4,15 @@ #include "spc/http_client.hpp" #include +#include #include #include #include +#include +#include +#include #include +#include namespace spc { @@ -58,8 +63,6 @@ struct RetryResult { delay_ms *= policy.backoff_multiplier; } - delay_ms = std::min(delay_ms, static_cast(policy.max_delay.count())); - if (policy.jitter_factor > 0) { static thread_local std::mt19937 rng{std::random_device{}()}; std::uniform_real_distribution dist(1.0 - policy.jitter_factor, @@ -67,20 +70,70 @@ struct RetryResult { delay_ms *= dist(rng); } + // Clamp AFTER the jitter: clamping first let jitter carry the delay back + // above the documented ceiling by up to jitter_factor. + delay_ms = std::min(delay_ms, static_cast(policy.max_delay.count())); + delay_ms = std::max(delay_ms, 0.0); + return std::chrono::milliseconds{static_cast(delay_ms)}; } +/// The `Retry-After` delay a 429/503 asked for, or zero. +/// +/// Only the delta-seconds form is parsed; the HTTP-date form falls back to the +/// computed backoff. 429 and 503 are exactly the responses `should_retry` +/// fires on and exactly the ones that carry this header, and `HttpResponse` +/// already captures every header. +[[nodiscard]] inline std::chrono::milliseconds retry_after(const HttpResponse& response) { + for (const std::pair& header : response.headers) { + const std::string& name = header.first; + if (name.size() != 11) { + continue; + } + bool matches = true; + const std::string_view target{"retry-after"}; + for (std::size_t i = 0; i < name.size(); ++i) { + const char lowered = + name[i] >= 'A' && name[i] <= 'Z' ? static_cast(name[i] - 'A' + 'a') : name[i]; + if (lowered != target[i]) { + matches = false; + break; + } + } + if (!matches) { + continue; + } + std::int64_t seconds = 0; + const std::from_chars_result parsed = std::from_chars( + header.second.data(), header.second.data() + header.second.size(), seconds); + if (parsed.ec == std::errc{} && seconds > 0) { + return std::chrono::milliseconds{seconds * 1000}; + } + return std::chrono::milliseconds{0}; + } + return std::chrono::milliseconds{0}; +} + /// Execute an HTTP operation with exponential-backoff retry. template [[nodiscard]] Result with_retry(Operation&& operation, const RetryPolicy& policy) { std::chrono::milliseconds total_delay{0}; + // max_attempts == 0 would otherwise skip the operation entirely and report + // a fabricated network error for a request that was never made. Iterate a + // wider counter so the top of the uint8 range cannot wrap. + const std::uint16_t attempts = std::max(1, policy.max_attempts); - for (std::uint8_t attempt = 1; attempt <= policy.max_attempts; ++attempt) { + for (std::uint16_t attempt = 1; attempt <= attempts; ++attempt) { Result result = operation(); + const std::uint8_t attempt_number = static_cast(attempt); if (result.has_value()) { - if (should_retry(*result, policy) && attempt < policy.max_attempts) { - std::chrono::milliseconds delay = calculate_retry_delay(attempt, policy); + if (should_retry(*result, policy) && attempt < attempts) { + // Respect a server that told us how long to wait. + const std::chrono::milliseconds requested = retry_after(*result); + const std::chrono::milliseconds delay = + std::min(std::max(calculate_retry_delay(attempt_number, policy), requested), + policy.max_delay); total_delay += delay; std::this_thread::sleep_for(delay); continue; @@ -88,8 +141,8 @@ template return result; } - if (should_retry(result.error(), policy) && attempt < policy.max_attempts) { - std::chrono::milliseconds delay = calculate_retry_delay(attempt, policy); + if (should_retry(result.error(), policy) && attempt < attempts) { + std::chrono::milliseconds delay = calculate_retry_delay(attempt_number, policy); total_delay += delay; std::this_thread::sleep_for(delay); continue; diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index a33412c..dc24afe 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -22,6 +22,9 @@ add_library(spc_http STATIC ) spc_configure_target(spc_http) target_link_libraries(spc_http PUBLIC spc_core CURL::libcurl) +# The default User-Agent lives in an installed public header, so the version +# has to reach consumers too. PUBLIC exports it through spcTargets.cmake. +target_compile_definitions(spc_http PUBLIC SPC_VERSION_STRING="${PROJECT_VERSION}") target_include_directories(spc_http PUBLIC $ $ diff --git a/src/api/client.cpp b/src/api/client.cpp index d5155b5..95139f0 100644 --- a/src/api/client.cpp +++ b/src/api/client.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -126,6 +127,15 @@ std::string percent_encode(std::string_view value) { return encoded; } +/// IEM is a courtesy third party, so the bucket stays small — but the wait +/// must be bounded, or `RateLimiter::acquire()` blocks the caller's thread +/// forever and the documented `ErrorCode::RateLimited` result is unreachable. +RateLimiter::Config archive_rate_limit() { + RateLimiter::Config config; + config.max_wait = std::chrono::seconds{5}; + return config; +} + const char* service_base(ArcGISService service) { switch (service) { case ArcGISService::Outlooks: @@ -138,7 +148,19 @@ const char* service_base(ArcGISService service) { return kArcGisOutlks; } -Result inspect_arcgis_envelope(const std::string& body) { +/// What `paged()` needs from one ArcGIS response envelope: whether the server +/// truncated the result, and how many records it actually returned. +struct ArcGISEnvelope { + std::int32_t feature_count{0}; + bool exceeded_transfer_limit{false}; +}; + +bool envelope_flag(const Json& root, const char* key) { + const Json* flag = detail::lookup(root, key); + return flag != nullptr && flag->is_boolean() && flag->get(); +} + +Result inspect_arcgis_envelope(const std::string& body) { const glz::expected root = detail::parse_root(body); if (!root) { return std::unexpected(Error::parse(root.error())); @@ -147,22 +169,42 @@ Result inspect_arcgis_envelope(const std::string& body) { if (error != nullptr && error->is_object()) { const double raw_code = detail::json_number_or_numeric_string(*error, "code"); const int code = raw_code > 0.0 ? static_cast(raw_code) : 400; - return std::unexpected(Error::from_response(code, body)); - } - const Json* exceeded = detail::lookup(*root, "exceededTransferLimit"); - return exceeded != nullptr && exceeded->is_boolean() && exceeded->get(); + // A logical ArcGIS failure travels over HTTP 200, so it must not go + // through the HTTP status mapper: an ArcGIS code 404 (renamed or + // retired service path) is a genuine NotFound, and a code like 1000 + // is not an HTTP status at all. + return std::unexpected(Error::from_arcgis(code, body)); + } + ArcGISEnvelope envelope; + // Verified live against SPC_wx_outlks layer 1 with resultRecordCount=1: + // `f=json` carries the flag at the root, `f=geojson` carries it at the + // root AND under `properties`. Read both so either shape is honoured. + envelope.exceeded_transfer_limit = envelope_flag(*root, "exceededTransferLimit"); + if (!envelope.exceeded_transfer_limit) { + const Json* properties = detail::lookup(*root, "properties"); + envelope.exceeded_transfer_limit = + properties != nullptr && envelope_flag(*properties, "exceededTransferLimit"); + } + // Esri (`f=json`) and GeoJSON (`f=geojson`) both name the array `features`. + const Json* features = detail::lookup(*root, "features"); + if (features != nullptr && features->is_array()) { + envelope.feature_count = static_cast(features->get_array().size()); + } + return envelope; } -/// SPC 404 == "no active outlook" (FeedUnavailable). Map HTTP status to the -/// right error; only a real body is handed to the parser. -Result body_or_error(Result r) { +/// Map HTTP status to the right error; only a real body is handed to the +/// parser. `semantics` is the trust boundary: only the SPC static feeds may +/// read a 404 as "no active outlook" (FeedUnavailable). For every other host +/// a 404 is a retired or wrong URL, i.e. NotFound. +Result body_or_error(Result r, Feed404 semantics) { if (!r) { return std::unexpected(r.error()); } if (r->status_code == 200) { return std::move(r->body); } - return std::unexpected(Error::from_response(r->status_code, r->body)); + return std::unexpected(Error::from_response(r->status_code, r->body, semantics)); } } // namespace @@ -191,8 +233,8 @@ Result StaticFeedClient::day_categorical(std::int32_t Error::invalid_request("categorical outlook day must be 1, 2, or 3")); } const std::string url = std::format("{}day{}otlk_cat.nolyr.geojson", kStaticBase, day); - Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry)); + Result body = body_or_error( + with_retry([&] { return impl_->http->get(url); }, impl_->retry), Feed404::NoActiveOutlook); if (!body) { return std::unexpected(body.error()); } @@ -219,8 +261,8 @@ Result StaticFeedClient::day_probabilistic(std::int32_t day, const std::string filename = day == 3 ? "day3otlk_prob.nolyr.geojson" : std::format("day{}otlk_{}.nolyr.geojson", day, tag); const std::string url = std::string{kStaticBase} + filename; - Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry)); + Result body = body_or_error( + with_retry([&] { return impl_->http->get(url); }, impl_->retry), Feed404::NoActiveOutlook); if (!body) { return std::unexpected(body.error()); } @@ -237,8 +279,8 @@ Result StaticFeedClient::day4_8(std::int32_t day) { Error::invalid_request("extended outlook day must be between 4 and 8")); } const std::string url = std::format("{}day{}prob.nolyr.geojson", kStaticDay48Base, day); - Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry)); + Result body = body_or_error( + with_retry([&] { return impl_->http->get(url); }, impl_->retry), Feed404::NoActiveOutlook); if (!body) { return std::unexpected(body.error()); } @@ -281,16 +323,25 @@ struct ArcGISClient::Impl { url += "&outSR=" + percent_encode(out_spatial_reference); } Result body = - body_or_error(with_retry([&] { return http->get(url); }, retry)); + body_or_error(with_retry([&] { return http->get(url); }, retry), Feed404::NotFound); if (!body) { return std::unexpected(body.error()); } - const Result exceeded = inspect_arcgis_envelope(*body); - if (!exceeded) { - return std::unexpected(exceeded.error()); + const Result envelope = inspect_arcgis_envelope(*body); + if (!envelope) { + return std::unexpected(envelope.error()); + } + if (envelope->exceeded_transfer_limit && envelope->feature_count == 0) { + // The offset would never move: paging cannot converge. + return std::unexpected( + Error::server("ArcGIS reported a truncated page containing no records")); } pages.push_back(std::move(*body)); - pager.advance(*exceeded); + pager.advance(envelope->feature_count, envelope->exceeded_transfer_limit); + } + if (pager.page_limit_reached()) { + return std::unexpected(Error::server( + std::format("ArcGIS paging did not converge within {} pages", pager.max_pages()))); } return pages; } @@ -516,7 +567,7 @@ struct ArchiveClient::Impl { r.initial_delay = std::chrono::milliseconds{500}; return r; }()), - limiter(RateLimiter::Config{}) {} + limiter(archive_rate_limit()) {} explicit Impl(std::shared_ptr transport) : http(usable_transport(std::move(transport))), retry([] { @@ -525,7 +576,22 @@ struct ArchiveClient::Impl { policy.initial_delay = std::chrono::milliseconds{500}; return policy; }()), - limiter(RateLimiter::Config{}) {} + limiter(archive_rate_limit()) {} + + /// Pay a token per *attempt*, not per call: `with_retry` re-issues the + /// request up to `max_attempts` times, and it retries precisely on + /// 429/503 — the responses in which IEM is asking for less traffic. + /// Acquiring once outside the retry loop undercounted the budget by 4x. + Result rate_limited_get(const std::string& url) { + return with_retry( + [&]() -> Result { + if (!limiter.acquire()) { + return std::unexpected(Error::rate_limited("IEM rate limit")); + } + return http->get(url); + }, + retry); + } }; ArchiveClient::ArchiveClient(ClientConfig config) @@ -537,15 +603,14 @@ ArchiveClient::ArchiveClient(ArchiveClient&&) noexcept = default; ArchiveClient& ArchiveClient::operator=(ArchiveClient&&) noexcept = default; Result ArchiveClient::watches(const std::string& ts) { - if (!impl_->limiter.acquire()) { - return std::unexpected(Error::rate_limited("IEM rate limit")); - } + // `ts` is caller-supplied and reaches a query string, so it is encoded + // like every ArcGIS query value: an unencoded '&' would inject a + // parameter and an unencoded '+' would decode server-side as a space. std::string url = std::format("{}json/spcwatch.py", kIemBase); if (!ts.empty()) { - url += std::format("?ts={}", ts); + url += "?ts=" + percent_encode(ts); } - Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry)); + Result body = body_or_error(impl_->rate_limited_get(url), Feed404::NotFound); if (!body) { return std::unexpected(body.error()); } @@ -560,16 +625,14 @@ Result ArchiveClient::watches(const std::string& ts) { Result ArchiveClient::storm_reports(const std::string& start_iso, const std::string& end_iso, const std::string& wfo) { - if (!impl_->limiter.acquire()) { - return std::unexpected(Error::rate_limited("IEM rate limit")); - } - std::string url = - std::format("{}geojson/lsr.geojson?sts={}&ets={}", kIemBase, start_iso, end_iso); + // api.hpp documents these as ISO 8601, which permits a "+HH:MM" offset; a + // raw '+' decodes server-side as a space and silently shifts the window. + std::string url = std::format("{}geojson/lsr.geojson?sts={}&ets={}", kIemBase, + percent_encode(start_iso), percent_encode(end_iso)); if (!wfo.empty()) { - url += std::format("&wfo={}", wfo); + url += "&wfo=" + percent_encode(wfo); } - Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry)); + Result body = body_or_error(impl_->rate_limited_get(url), Feed404::NotFound); if (!body) { return std::unexpected(body.error()); } diff --git a/src/core/error.cpp b/src/core/error.cpp index 9bc0c0a..94c50c3 100644 --- a/src/core/error.cpp +++ b/src/core/error.cpp @@ -1,34 +1,18 @@ #include "spc/error.hpp" +#include #include #include #include namespace spc { -Error Error::from_response(int status, const std::string& body) { - Error err; - err.http_status = status; - - // Determine error code from HTTP status. SPC serves a static HTML 404 - // page when a product has no active outlook (the documented normal - // overnight state for e.g. day-1 probabilistic); we surface that as - // FeedUnavailable so callers clear rows rather than treat it as a fault. - if (status == 404) { - err.code = ErrorCode::FeedUnavailable; - } else if (status == 400) { - err.code = ErrorCode::InvalidRequest; - } else if (status == 429 || status == 503) { - err.code = ErrorCode::RateLimited; - } else if (status >= 500) { - err.code = ErrorCode::ServerError; - } else { - err.code = ErrorCode::Unknown; - } +namespace { - // Best-effort: ArcGIS returns JSON `{"error":{"code":...,"message":...}}` - // on logical failures even with HTTP 200. Parse via glz::generic (no - // schema to reject on); fall back to the raw body for SPC's HTML pages. +/// Best-effort: ArcGIS returns JSON `{"error":{"code":...,"message":...}}` +/// on logical failures even with HTTP 200. Parse via glz::generic (no schema +/// to reject on); fall back to the raw body for SPC's HTML pages. +void fill_message_from_body(Error& err, const std::string& body) { glz::generic root{}; glz::error_ctx ec = glz::read_json(root, body); if (!ec && root.is_object()) { @@ -49,6 +33,34 @@ Error Error::from_response(int status, const std::string& body) { // Not valid JSON (SPC's HTML 404) — keep a short raw snippet. err.message = body.substr(0, 256); } +} + +} // namespace + +Error Error::from_response(int status, const std::string& body, Feed404 semantics) { + Error err; + err.http_status = status; + + // Determine error code from HTTP status. SPC serves a static HTML 404 + // page when a product has no active outlook (the documented normal + // overnight state for e.g. day-1 probabilistic); only a caller that + // addressed those feeds may ask for that reading. Every other 404 — a + // renamed MapServer path, a retired IEM endpoint, a typo in a base URL — + // is a genuine fault, so the default is NotFound. + if (status == 404) { + err.code = semantics == Feed404::NoActiveOutlook ? ErrorCode::FeedUnavailable + : ErrorCode::NotFound; + } else if (status == 400) { + err.code = ErrorCode::InvalidRequest; + } else if (status == 429 || status == 503) { + err.code = ErrorCode::RateLimited; + } else if (status >= 500) { + err.code = ErrorCode::ServerError; + } else { + err.code = ErrorCode::Unknown; + } + + fill_message_from_body(err, body); if (err.message.empty()) { err.message = "HTTP " + std::to_string(status); @@ -57,4 +69,38 @@ Error Error::from_response(int status, const std::string& body) { return err; } +Error Error::from_arcgis(int arcgis_code, const std::string& body) { + Error err; + + // ArcGIS codes overlap the HTTP status space but are not HTTP statuses: + // the transport answered 200. Keep the code in http_status only while it + // is HTTP-shaped, so a code such as 1000 cannot masquerade as a status. + err.http_status = arcgis_code >= 100 && arcgis_code <= 599 ? arcgis_code : 0; + + if (arcgis_code == 404) { + // A renamed or retired MapServer path. Never "no active outlook": + // ArcGIS signals an empty product as HTTP 200 with no features. + err.code = ErrorCode::NotFound; + } else if (arcgis_code == 429) { + err.code = ErrorCode::RateLimited; + } else if (arcgis_code >= 500 && arcgis_code <= 599) { + err.code = ErrorCode::ServerError; + } else { + // 400, 403, 498/499 (token), 1000+ (operation failures): the request + // as posed cannot be served. + err.code = ErrorCode::InvalidRequest; + } + + fill_message_from_body(err, body); + + if (err.detail.empty()) { + err.detail = std::format("arcgisCode={}", arcgis_code); + } + if (err.message.empty()) { + err.message = std::format("ArcGIS error {}", arcgis_code); + } + + return err; +} + } // namespace spc diff --git a/src/core/rate_limit.cpp b/src/core/rate_limit.cpp index a4bff49..4784441 100644 --- a/src/core/rate_limit.cpp +++ b/src/core/rate_limit.cpp @@ -4,14 +4,37 @@ #include "spc/rate_limit.hpp" #include +#include namespace spc { +namespace { + +/// Config is a public aggregate with no validation, so clamp the two fields +/// that can otherwise make the limiter misbehave: a non-positive refill +/// interval divides by zero in refill(), and initial_tokens above max_tokens +/// hands out a burst the bucket was never sized for. +RateLimiter::Config sanitized(RateLimiter::Config config) { + if (config.refill_interval <= std::chrono::milliseconds::zero()) { + config.refill_interval = std::chrono::milliseconds{1000}; + } + config.initial_tokens = std::min(config.initial_tokens, config.max_tokens); + return config; +} + +} // namespace + RateLimiter::RateLimiter(Config config) - : config_(config), tokens_(config_.initial_tokens), + : config_(sanitized(config)), tokens_(config_.initial_tokens), last_refill_(std::chrono::steady_clock::now()), day_start_(std::chrono::floor(std::chrono::system_clock::now())) {} +bool RateLimiter::daily_quota_exhausted() noexcept { + std::lock_guard lock(mutex_); + check_daily_reset(); + return config_.daily_limit > 0 && daily_requests_used_ >= config_.daily_limit; +} + void RateLimiter::check_daily_reset() noexcept { std::chrono::system_clock::time_point now = std::chrono::system_clock::now(); std::chrono::system_clock::time_point current_day = std::chrono::floor(now); @@ -65,6 +88,11 @@ bool RateLimiter::acquire() { if (try_acquire()) { return true; } + // Waiting cannot help: only a UTC-day rollover clears the quota, and + // spinning toward midnight is never what the caller wanted. + if (daily_quota_exhausted()) { + return false; + } std::this_thread::sleep_for(std::chrono::milliseconds(10)); } } @@ -76,6 +104,9 @@ bool RateLimiter::acquire_for(std::chrono::milliseconds max_wait) { if (try_acquire()) { return true; } + if (daily_quota_exhausted()) { + return false; + } std::this_thread::sleep_for(std::chrono::milliseconds(10)); } return false; diff --git a/src/http/client.cpp b/src/http/client.cpp index aea9513..fa8c967 100644 --- a/src/http/client.cpp +++ b/src/http/client.cpp @@ -8,9 +8,23 @@ namespace spc { namespace { +/// Accumulates the body, refusing to grow past a ceiling. Returning a short +/// count makes libcurl abort the transfer with CURLE_WRITE_ERROR. +struct BodySink { + std::string body; + std::size_t limit{0}; + bool overflowed{false}; +}; + std::size_t write_cb(char* ptr, std::size_t size, std::size_t nmemb, void* user) { - static_cast(user)->append(ptr, size * nmemb); - return size * nmemb; + BodySink* sink = static_cast(user); + const std::size_t chunk = size * nmemb; + if (sink->limit > 0 && sink->body.size() + chunk > sink->limit) { + sink->overflowed = true; + return 0; + } + sink->body.append(ptr, chunk); + return chunk; } std::size_t header_cb(char* buffer, std::size_t size, std::size_t nitems, void* userdata) { @@ -101,16 +115,23 @@ Result HttpClient::get(std::string_view path) const { CURL* curl = impl_->curl; const std::string url = is_absolute_url(path) ? std::string{path} : impl_->config.base_url + std::string{path}; - std::string body; + BodySink sink; + sink.limit = impl_->config.max_response_bytes; std::vector> response_headers; curl_easy_reset(curl); curl_easy_setopt(curl, CURLOPT_URL, url.c_str()); curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, &write_cb); - curl_easy_setopt(curl, CURLOPT_WRITEDATA, &body); + curl_easy_setopt(curl, CURLOPT_WRITEDATA, &sink); curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, &header_cb); curl_easy_setopt(curl, CURLOPT_HEADERDATA, &response_headers); curl_easy_setopt(curl, CURLOPT_TIMEOUT, static_cast(impl_->config.timeout.count())); + // This SDK speaks to public HTTP(S) endpoints only. Without this libcurl + // happily honours file://, dict://, scp:// and friends, and a path built + // from user input becomes a local-file read. + curl_easy_setopt(curl, CURLOPT_PROTOCOLS_STR, "http,https"); + curl_easy_setopt(curl, CURLOPT_REDIR_PROTOCOLS_STR, "http,https"); + curl_easy_setopt(curl, CURLOPT_MAXREDIRS, 10L); // Parity with spc-data/src/fetcher.cpp: curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L); curl_easy_setopt(curl, CURLOPT_NOSIGNAL, 1L); @@ -121,6 +142,10 @@ Result HttpClient::get(std::string_view path) const { CURLcode rc = curl_easy_perform(curl); if (rc != CURLE_OK) { + if (sink.overflowed) { + return std::unexpected( + Error::network("response exceeded ClientConfig::max_response_bytes")); + } return std::unexpected(Error::network(curl_easy_strerror(rc))); } @@ -129,7 +154,7 @@ Result HttpClient::get(std::string_view path) const { return HttpResponse{ static_cast(http_code), - std::move(body), + std::move(sink.body), std::move(response_headers), }; } diff --git a/src/models/common.cpp b/src/models/common.cpp index 5776418..af1368b 100644 --- a/src/models/common.cpp +++ b/src/models/common.cpp @@ -10,6 +10,31 @@ #include +// Floating-point std::from_chars is the locale-independent parse the standard +// intends, but libc++ only implements it from version 20 (the project's own +// clang-tidy job builds against libc++ 18, where the overload is deleted). +// libstdc++ and MSVC advertise it through __cpp_lib_to_chars; libc++ does not +// define that macro at all, so fall back to its version. +// Definable on the command line to exercise the fallback on a toolchain that +// has from_chars. +#ifndef SPC_HAS_FP_FROM_CHARS +#if defined(__cpp_lib_to_chars) && __cpp_lib_to_chars >= 201611L +#define SPC_HAS_FP_FROM_CHARS 1 +#elif defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 200000 +#define SPC_HAS_FP_FROM_CHARS 1 +#else +#define SPC_HAS_FP_FROM_CHARS 0 +#endif +#endif + +#if SPC_HAS_FP_FROM_CHARS +#include +#include +#else +#include +#include +#endif + namespace spc { namespace detail { @@ -38,6 +63,50 @@ std::string json_string(const Json& obj, const char* key) { return {}; } +ParsedNumber parse_double(std::string_view text) { + ParsedNumber parsed; + if (text.empty()) { + return parsed; + } + // Gate the leading character so both implementations below agree on what + // they accept: from_chars takes '-', a digit, '.', or inf/nan, and never + // leading whitespace or '+'. + const char first = text.front(); + const bool leading_ok = first == '-' || first == '.' || (first >= '0' && first <= '9') || + first == 'i' || first == 'I' || first == 'n' || first == 'N'; + if (!leading_ok) { + return parsed; + } + +#if SPC_HAS_FP_FROM_CHARS + const std::from_chars_result result = + std::from_chars(text.data(), text.data() + text.size(), parsed.value); + if (result.ec != std::errc{}) { + return ParsedNumber{}; + } + parsed.consumed = static_cast(result.ptr - text.data()); + parsed.ok = true; + return parsed; +#else + std::istringstream stream{std::string{text}}; + stream.imbue(std::locale::classic()); + stream >> parsed.value; + if (stream.fail()) { + return ParsedNumber{}; + } + // tellg() reports -1 once the whole buffer was consumed. + parsed.consumed = text.size(); + if (!stream.eof()) { + const std::streamoff position = stream.tellg(); + if (position >= 0) { + parsed.consumed = static_cast(position); + } + } + parsed.ok = true; + return parsed; +#endif +} + /// SPC ships `LABEL` as either a string ("SLGT", "5") or a number (5). Always /// returns a numeric view; non-numeric / missing yields 0. double json_number_or_numeric_string(const Json& obj, const char* key) { @@ -49,12 +118,11 @@ double json_number_or_numeric_string(const Json& obj, const char* key) { return v->get(); } if (v->is_string()) { - const std::string s = v->get(); - try { - return std::stod(s); - } catch (...) { - return 0.0; - } + // Was std::stod, whose strtod honours LC_NUMERIC; see parse_double. + // Byte-identical to a C-locale stod for every value in the fixture + // corpus, which is what the spc-data byte-identity gate covers. + const ParsedNumber parsed = parse_double(v->get()); + return parsed.ok ? parsed.value : 0.0; } return 0.0; } diff --git a/src/models/fire_weather.cpp b/src/models/fire_weather.cpp index df05fc9..52b5d85 100644 --- a/src/models/fire_weather.cpp +++ b/src/models/fire_weather.cpp @@ -63,13 +63,11 @@ bool has_zero_dn(const Json& props) { if (!dn->is_string()) { return false; } + // Locale-independent: a comma-decimal strtod stops at the '.' of "0.0", + // the full-consume check fails, and the no-risk sentinel ships as a band. const std::string text = dn->get(); - std::size_t consumed = 0; - try { - return std::stod(text, &consumed) == 0.0 && consumed == text.size(); - } catch (...) { - return false; - } + const detail::ParsedNumber parsed = detail::parse_double(text); + return parsed.ok && parsed.value == 0.0 && parsed.consumed == text.size(); } std::string label_from_dn(const Json& props, FireWeatherLayer layer) { @@ -112,10 +110,6 @@ std::uint8_t fire_severity_from_label(std::string_view label) noexcept { return 0; // dry-thunderstorm bands (IDRT/SDRT) and unknowns: label-only } -FireWeatherPayload parse_fire_weather(std::string_view body, std::int32_t day) { - return parse_fire_weather(body, day, FireWeatherLayer::Outlook); -} - FireWeatherPayload parse_fire_weather(std::string_view body, std::int32_t day, FireWeatherLayer layer) { const Json root = parse_root_or_throw(body); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index db765f1..ab1a891 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -4,6 +4,9 @@ add_executable(spc_tests test_parser.cpp test_corpus.cpp test_arcgis.cpp + test_rate_limit.cpp + test_locale.cpp + test_retry.cpp ) target_link_libraries(spc_tests PRIVATE spc_api @@ -11,6 +14,7 @@ target_link_libraries(spc_tests PRIVATE ) target_compile_definitions(spc_tests PRIVATE SPC_FIXTURES_DIR="${CMAKE_CURRENT_SOURCE_DIR}/fixtures" + SPC_PROJECT_VERSION="${PROJECT_VERSION}" ) target_compile_options(spc_tests PRIVATE -Wall -Wextra -Wpedantic) include(GoogleTest) diff --git a/tests/test_arcgis.cpp b/tests/test_arcgis.cpp index db9490f..4fba947 100644 --- a/tests/test_arcgis.cpp +++ b/tests/test_arcgis.cpp @@ -19,11 +19,16 @@ #include "spc/models/storm_report.hpp" #include "spc/models/watch.hpp" +#include +#include #include +#include #include #include #include #include +#include +#include namespace { @@ -180,14 +185,132 @@ TEST(ArcGISParity, EsriRingsMatchGeoJsonForDay1Categorical) { "genuinely diverged from the verbatim GeoJSON path (not source quantization)"; } -TEST(ArcGISParity, EsriProbTornadoMatchesGeoJsonProb) { - const ProbOutlookPayload gj = - parse_probabilistic(slurp("arcgis_day1_prob_tornado.geojson"), 1, "tornado"); - ASSERT_GT(gj.features.size(), 0u); - for (const ProbOutlookFeature& f : gj.features) { - EXPECT_GT(f.probability, 0.0); - EXPECT_LT(f.probability, 1.0); - EXPECT_FALSE(f.rings.empty()); +// Grid-probe membership agreement between two ring sets, on the same terms as +// the day-1 categorical gate above: probes within ~5 m of either boundary are +// skipped, everything else must classify identically. +struct MembershipProbe { + std::size_t compared = 0; + std::size_t agreed = 0; +}; + +void probe_membership(const std::vector& reference, const std::vector& candidate, + MembershipProbe& probe) { + constexpr double kSkip = 5.0e-5; + constexpr int kN = 40; + + double minx = 1e18; + double miny = 1e18; + double maxx = -1e18; + double maxy = -1e18; + for (const Polygon& r : reference) { + for (const LonLat& p : r) { + minx = std::min(minx, p.lon); + maxx = std::max(maxx, p.lon); + miny = std::min(miny, p.lat); + maxy = std::max(maxy, p.lat); + } + } + const double pad = 0.5; + minx -= pad; + maxx += pad; + miny -= pad; + maxy += pad; + + for (int ix = 0; ix <= kN; ++ix) { + for (int iy = 0; iy <= kN; ++iy) { + const double px = minx + (maxx - minx) * (static_cast(ix) / kN); + const double py = miny + (maxy - miny) * (static_cast(iy) / kN); + if (dist_to_boundary(px, py, reference) < kSkip || + dist_to_boundary(px, py, candidate) < kSkip) { + continue; + } + ++probe.compared; + if (inside_any(px, py, reference) == inside_any(px, py, candidate)) { + ++probe.agreed; + } + } + } +} + +// Every captured Esri/GeoJSON fixture pair, not just the day-1 categorical +// one. The probabilistic pairs previously had no reader at all: the test named +// for Esri-vs-GeoJSON probabilistic parity only ever opened the GeoJSON side, +// so parse_esri_rings' hole-dropping rule was pinned by one categorical layer. +TEST(ArcGISParity, EveryCapturedEsriFixtureMatchesItsGeoJsonTwin) { + struct Pair { + std::string stem; + std::int32_t day; + std::string hazard; ///< empty for the categorical layers + }; + const std::vector pairs = { + {"arcgis_day1_categorical", 1, ""}, {"arcgis_day2_categorical", 2, ""}, + {"arcgis_day3_categorical", 3, ""}, {"arcgis_day1_prob_tornado", 1, "tornado"}, + {"arcgis_day1_prob_hail", 1, "hail"}, {"arcgis_day1_prob_wind", 1, "wind"}, + {"arcgis_day2_prob_wind", 2, "wind"}, + }; + + for (const Pair& pair : pairs) { + // Label -> rings, from the verbatim GeoJSON walker. + std::vector>> reference; + if (pair.hazard.empty()) { + const CategoricalOutlookPayload gj = + parse_categorical(slurp(pair.stem + ".geojson"), pair.day); + for (const OutlookFeature& f : gj.features) { + reference.emplace_back(f.label, f.rings); + } + } else { + const ProbOutlookPayload gj = + parse_probabilistic(slurp(pair.stem + ".geojson"), pair.day, pair.hazard); + for (const ProbOutlookFeature& f : gj.features) { + EXPECT_GT(f.probability, 0.0) << pair.stem; + EXPECT_LT(f.probability, 1.0) << pair.stem; + // ProbOutlookFeature keeps no label; the isopleth value is the + // band identity, and both sides derive it from the same string. + reference.emplace_back(std::format("{:.6f}", f.probability), f.rings); + } + } + ASSERT_GT(reference.size(), 0u) << pair.stem; + + const glz::expected root = + detail::parse_root(slurp(pair.stem + ".esri.json")); + ASSERT_TRUE(root.has_value()) << pair.stem; + const Json* feats = detail::lookup(*root, "features"); + ASSERT_NE(feats, nullptr) << pair.stem; + ASSERT_TRUE(feats->is_array()) << pair.stem; + + MembershipProbe probe; + std::size_t matched = 0; + for (const glz::generic& feat : feats->get_array()) { + const Json* attrs = detail::lookup(feat, "attributes"); + const Json* geom = detail::lookup(feat, "geometry"); + if (attrs == nullptr || geom == nullptr) { + continue; + } + const std::string label = + pair.hazard.empty() ? detail::json_string(*attrs, "label") + : std::format("{:.6f}", detail::normalized_probability(*attrs)); + const std::vector* expected = nullptr; + for (const std::pair>& band : reference) { + if (band.first == label) { + expected = &band.second; + break; + } + } + if (expected == nullptr) { + continue; + } + const std::vector esri_rings = detail::parse_esri_rings(*geom); + ASSERT_FALSE(esri_rings.empty()) << pair.stem << " band " << label; + ++matched; + probe_membership(*expected, esri_rings, probe); + } + + EXPECT_EQ(matched, reference.size()) + << pair.stem << ": Esri and GeoJSON disagree on the band set"; + EXPECT_GT(probe.compared, 100u) << pair.stem << ": too few clear-of-boundary probes"; + EXPECT_EQ(probe.agreed, probe.compared) + << pair.stem << ": Esri vs GeoJSON disagreed on " << (probe.compared - probe.agreed) + << "/" << probe.compared << " unambiguous interior/exterior probes"; } } @@ -230,7 +353,8 @@ TEST(NetNewModels, FireWeatherOwnSeverityMapper) { EXPECT_EQ(fire_severity_from_label("CRIT"), 2); EXPECT_EQ(fire_severity_from_label("EXTM"), 3); EXPECT_EQ(fire_severity_from_label("SLGT"), 0); // not categorical - const FireWeatherPayload p = parse_fire_weather(slurp("arcgis_day1_fire_weather.esri.json"), 1); + const FireWeatherPayload p = parse_fire_weather(slurp("arcgis_day1_fire_weather.esri.json"), 1, + FireWeatherLayer::Outlook); EXPECT_EQ(p.day, 1); ASSERT_EQ(p.features.size(), 3u); EXPECT_EQ(p.features[0].label, "ELEV"); @@ -244,6 +368,39 @@ TEST(NetNewModels, FireWeatherOwnSeverityMapper) { } } +TEST(NetNewModels, FireWeatherLayerIsTheOnlyThingThatDisambiguatesDayOneAndTwoDn) { + // The captured day-1 and day-2 payloads carry no LABEL at all, only the + // numeric dn band index (5/8/10) that the Outlook and DryThunderstorm + // layers both use with different meanings. Nothing in the body says which + // layer it came from, so the caller must say — there is no safe default. + for (const std::string& name : {std::string{"arcgis_day1_fire_weather.esri.json"}, + std::string{"arcgis_day2_fire_weather.esri.json"}}) { + const std::string body = slurp(name); + EXPECT_EQ(body.find("LABEL"), std::string::npos) << name; + EXPECT_EQ(body.find("\"label\""), std::string::npos) << name; + + const std::int32_t day = name.find("day1") != std::string::npos ? 1 : 2; + const FireWeatherPayload outlook = parse_fire_weather(body, day, FireWeatherLayer::Outlook); + const FireWeatherPayload dry = + parse_fire_weather(body, day, FireWeatherLayer::DryThunderstorm); + + ASSERT_EQ(outlook.features.size(), 3u) << name; + ASSERT_EQ(dry.features.size(), 3u) << name; + EXPECT_EQ(outlook.features[0].label, "ELEV") << name; + EXPECT_EQ(outlook.features[1].label, "CRIT") << name; + EXPECT_EQ(outlook.features[2].label, "EXTM") << name; + EXPECT_EQ(dry.features[0].label, "IDRT") << name; + EXPECT_EQ(dry.features[1].label, "SDRT") << name; + for (const FireWeatherFeature& f : dry.features) { + EXPECT_EQ(f.layer, FireWeatherLayer::DryThunderstorm) << name; + EXPECT_EQ(f.severity, 0) << name; + EXPECT_NE(f.label, "ELEV") << name; + EXPECT_NE(f.label, "CRIT") << name; + EXPECT_NE(f.label, "EXTM") << name; + } + } +} + TEST(NetNewModels, FireWeatherDryThunderstormCodesUseTheirOwnLabels) { const std::string body = R"({"features":[ {"attributes":{"dn":5},"geometry":{"rings":[[[0,1],[1,1],[1,0],[0,0],[0,1]]]}}, diff --git a/tests/test_client.cpp b/tests/test_client.cpp index e7242b9..a09b798 100644 --- a/tests/test_client.cpp +++ b/tests/test_client.cpp @@ -1,5 +1,7 @@ #include "spc/api.hpp" +#include "spc/pagination.hpp" +#include #include #include #include @@ -30,6 +32,32 @@ HttpResponse empty_feature_collection() { return {200, R"({"type":"FeatureCollection","features":[],"exceededTransferLimit":false})", {}}; } +/// A FeatureCollection of `count` placeholder features that still reports +/// truncation — the shape ArcGIS returns when a layer's own maxRecordCount is +/// below the requested resultRecordCount. +std::string truncated_page(std::int32_t count) { + std::string body = R"({"type":"FeatureCollection","features":[)"; + for (std::int32_t i = 0; i < count; ++i) { + if (i > 0) { + body += ","; + } + body += R"({"type":"Feature","properties":{},"geometry":null})"; + } + body += R"(],"exceededTransferLimit":true})"; + return body; +} + +/// Answers every request with the same truncated page, forever. +class AlwaysTruncatingTransport final : public HttpTransport { +public: + mutable std::int32_t calls = 0; + + [[nodiscard]] Result get(std::string_view /*path*/) const override { + ++calls; + return HttpResponse{200, truncated_page(1), {}}; + } +}; + #if defined(__GNUC__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated-declarations" @@ -187,6 +215,27 @@ TEST(ArcGISClientRouting, CoversEveryPublishedFireWeatherFeatureLayer) { } } +TEST(ArcGISClientRouting, FireWeatherIsAllOrNothingAcrossItsTwoMergedLayers) { + // query_fire_weather merges two layers per day. If the second fails, the + // features already parsed from the first are discarded and the caller gets + // an error with no indication that half the product was retrieved. + // FireWeatherPayload cannot represent the partial state, so the contract + // is all-or-nothing; this pins it. + std::shared_ptr transport = std::make_shared(); + const std::string body = R"({"features":[ + {"attributes":{"dn":5},"geometry":{"rings":[[[0,1],[1,1],[1,0],[0,0],[0,1]]]}} + ],"exceededTransferLimit":false})"; + // A 400 is not retryable, so the second layer fails on its first request. + transport->responses = {{200, body, {}}, {400, "bad request", {}}}; + ArcGISClient client{transport}; + + const Result result = client.query_fire_weather(1); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::InvalidRequest); + EXPECT_EQ(transport->requests.size(), 2u); +} + TEST(ArcGISClientRouting, RejectsUnsupportedProductsBeforeNetworkAccess) { std::shared_ptr transport = std::make_shared(); ArcGISClient client{transport}; @@ -207,7 +256,7 @@ TEST(ArcGISClientRouting, RejectsUnsupportedProductsBeforeNetworkAccess) { TEST(ArcGISClientPaging, EncodesParametersAndFetchesEveryPage) { std::shared_ptr transport = std::make_shared(); transport->responses = { - {200, R"({"features":[],"exceededTransferLimit":true})", {}}, + {200, truncated_page(2000), {}}, {200, R"({"features":[],"exceededTransferLimit":false})", {}}, }; ArcGISClient client{transport}; @@ -231,6 +280,56 @@ TEST(ArcGISClientPaging, EncodesParametersAndFetchesEveryPage) { EXPECT_NE(transport->requests[1].find("resultOffset=2000"), std::string::npos); } +TEST(ArcGISClientPaging, AdvancesTheOffsetByTheRecordsTheServerActuallyReturned) { + // ArcGIS clamps resultRecordCount to the layer's own maxRecordCount, so a + // truncated page can be shorter than the 2000 requested. Advancing by the + // request size would skip the records in between. + std::shared_ptr transport = std::make_shared(); + transport->responses = { + {200, truncated_page(3), {}}, + {200, truncated_page(2), {}}, + {200, R"({"features":[],"exceededTransferLimit":false})", {}}, + }; + ArcGISClient client{transport}; + + const Result> result = + client.query_layer(ArcGISService::Outlooks, 1, {}); + + ASSERT_TRUE(result); + ASSERT_EQ(transport->requests.size(), 3u); + EXPECT_NE(transport->requests[0].find("resultOffset=0"), std::string::npos); + EXPECT_NE(transport->requests[1].find("resultOffset=3"), std::string::npos); + EXPECT_NE(transport->requests[2].find("resultOffset=5"), std::string::npos); +} + +TEST(ArcGISClientPaging, FailsWhenATruncatedPageCarriesNoRecords) { + // Zero records plus exceededTransferLimit:true cannot converge — the + // offset would never move. Fail instead of looping. + std::shared_ptr transport = std::make_shared(); + transport->responses = {{200, R"({"features":[],"exceededTransferLimit":true})", {}}}; + ArcGISClient client{transport}; + + const Result> result = + client.query_layer(ArcGISService::Outlooks, 1, {}); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::ServerError); + EXPECT_EQ(transport->requests.size(), 1u); +} + +TEST(ArcGISClientPaging, StopsAndFailsWhenTheServerNeverStopsReportingTruncation) { + std::shared_ptr transport = + std::make_shared(); + ArcGISClient client{transport}; + + const Result> result = + client.query_layer(ArcGISService::Outlooks, 1, {}); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::ServerError); + EXPECT_EQ(transport->calls, ArcGISPager{}.max_pages()); +} + TEST(ArcGISClientPaging, ReturnsLogicalArcGISErrorsReportedWithHttp200) { std::shared_ptr transport = std::make_shared(); transport->responses = { @@ -249,6 +348,87 @@ TEST(ArcGISClientPaging, ReturnsLogicalArcGISErrorsReportedWithHttp200) { EXPECT_EQ(result.error().message, "Invalid or missing input parameters."); } +// ===== HTTP 404 trust boundary ===== +// +// SPC's static products answer 404 with an HTML page when there is no active +// outlook (verified live: a retired www.spc.noaa.gov outlook path returns +// HTTP 404 text/html). Every other feed's 404 is a genuine fault. The ArcGIS +// MapServer reports a retired service path as HTTP 200 with a logical +// `{"error":{"code":404}}` envelope (verified live against a renamed service). + +TEST(Feed404Semantics, StaticFeedReadsSpcHtml404AsNoActiveOutlook) { + std::shared_ptr transport = std::make_shared(); + transport->responses = {{404, "404 Not Found", {}}}; + StaticFeedClient client{transport}; + + const Result result = client.day_categorical(1); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::FeedUnavailable); + EXPECT_TRUE(result.error().is_feed_unavailable()); + EXPECT_EQ(result.error().http_status, 404); +} + +TEST(Feed404Semantics, ArcGisTransport404IsAGenuineNotFound) { + std::shared_ptr transport = std::make_shared(); + transport->responses = {{404, "not found", {}}}; + ArcGISClient client{transport}; + + const Result result = client.query_categorical(1); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::NotFound); + EXPECT_FALSE(result.error().is_feed_unavailable()); +} + +TEST(Feed404Semantics, ArcGisLogicalNotFoundIsAGenuineNotFound) { + std::shared_ptr transport = std::make_shared(); + // Live shape for a renamed / retired MapServer path. + transport->responses = { + {200, + R"({"error":{"code":404,"message":"Service outlooks/SPC_wx_outlks_RETIRED/MapServer not found ","details":[]}})", + {}}, + }; + ArcGISClient client{transport}; + + const Result result = client.query_categorical(1); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::NotFound); + EXPECT_FALSE(result.error().is_feed_unavailable()); + EXPECT_NE(result.error().message.find("not found"), std::string::npos); +} + +TEST(Feed404Semantics, ArcGisCodeThatIsNotAnHttpStatusStaysOutOfHttpStatus) { + std::shared_ptr transport = std::make_shared(); + transport->responses = { + {200, + R"({"error":{"code":1000,"message":"Unable to complete operation.","details":[]}})", + {}}, + }; + ArcGISClient client{transport}; + + const Result> result = + client.query_layer(ArcGISService::Outlooks, 1, {}); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::InvalidRequest); + EXPECT_EQ(result.error().http_status, 0); + EXPECT_NE(result.error().detail.find("1000"), std::string::npos); +} + +TEST(Feed404Semantics, ArchiveTransport404IsAGenuineNotFound) { + std::shared_ptr transport = std::make_shared(); + transport->responses = {{404, "gone", {}}}; + ArchiveClient client{transport}; + + const Result result = client.watches(); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::NotFound); + EXPECT_FALSE(result.error().is_feed_unavailable()); +} + TEST(ArcGISClientRouting, ActiveWatchesDirectCallersToTheIemClient) { std::shared_ptr transport = std::make_shared(); ArcGISClient client{transport}; @@ -261,6 +441,64 @@ TEST(ArcGISClientRouting, ActiveWatchesDirectCallersToTheIemClient) { EXPECT_TRUE(transport->requests.empty()); } +// ===== ArchiveClient (IEM) query construction ===== +// +// api.hpp documents start_iso/end_iso as ISO 8601, which permits a "+HH:MM" +// UTC offset. A raw '+' in a query string decodes server-side as a space, so +// an unencoded offset silently queries a different window; a raw '&' in any +// value injects extra parameters. + +TEST(ArchiveClientRouting, PercentEncodesTheWatchTimestamp) { + std::shared_ptr transport = std::make_shared(); + transport->responses = {{200, R"({"features":[]})", {}}}; + ArchiveClient client{transport}; + + ASSERT_TRUE(client.watches("202605191200&x=1")); + + ASSERT_EQ(transport->requests.size(), 1u); + EXPECT_EQ(transport->requests[0], + "https://mesonet.agron.iastate.edu/json/spcwatch.py?ts=202605191200%26x%3D1"); +} + +TEST(ArchiveClientRouting, PercentEncodesIsoOffsetsAndTheWfoFilter) { + std::shared_ptr transport = std::make_shared(); + transport->responses = {{200, R"({"features":[]})", {}}}; + ArchiveClient client{transport}; + + ASSERT_TRUE(client.storm_reports("2026-05-19T12:00:00+00:00", "2026-05-20T12:00:00+00:00", + "ICT&sts=1900-01-01")); + + ASSERT_EQ(transport->requests.size(), 1u); + EXPECT_EQ(transport->requests[0], + "https://mesonet.agron.iastate.edu/geojson/lsr.geojson" + "?sts=2026-05-19T12%3A00%3A00%2B00%3A00&ets=2026-05-20T12%3A00%3A00%2B00%3A00" + "&wfo=ICT%26sts%3D1900-01-01"); +} + +TEST(HttpClientLifecycle, DefaultUserAgentCarriesTheProjectVersion) { + // The UA string is a literal in an installed header; nothing tied it to + // project(spc-cpp VERSION ...), so a release bump left every outbound + // request to NOAA and IEM announcing the previous version. + const ClientConfig config; + + EXPECT_NE(config.user_agent.find(SPC_PROJECT_VERSION), std::string::npos) + << "user_agent \"" << config.user_agent << "\" does not carry version " + << SPC_PROJECT_VERSION; +} + +TEST(HttpClientLifecycle, RefusesEverySchemeOtherThanHttpAndHttps) { + // is_absolute_url() only recognises http:// and https://, so a file:// URL + // was treated as relative, concatenated onto the empty default base_url, + // and handed to libcurl — which read the local file and returned it as the + // response body. Nothing in this SDK's scope should reach the filesystem. + const HttpClient client; + + const Result result = client.get("file:///etc/hosts"); + + ASSERT_FALSE(result) << "file:// must not be fetched"; + EXPECT_EQ(result.error().code, ErrorCode::NetworkError); +} + TEST(HttpClientLifecycle, ConcurrentClientsShareProcessWideCurlState) { std::vector workers; workers.reserve(16); diff --git a/tests/test_locale.cpp b/tests/test_locale.cpp new file mode 100644 index 0000000..df50a42 --- /dev/null +++ b/tests/test_locale.cpp @@ -0,0 +1,103 @@ +/// @file test_locale.cpp +/// @brief SPC publishes probabilities as numeric strings ("0.15"). Decoding +/// them must not depend on the host application's LC_NUMERIC: a host that +/// calls setlocale(LC_ALL, "") on a comma-decimal desktop (as Qt/GTK apps do) +/// would otherwise get a successful but silently empty outlook. + +#include "spc/models/convective.hpp" +#include "spc/models/fire_weather.hpp" +#include "spc/models/outlook.hpp" + +#include +#include +#include +#include +#include +#include + +namespace { + +using namespace spc; + +std::string slurp(const std::string& name) { + std::ifstream f(std::filesystem::path(SPC_FIXTURES_DIR) / name, std::ios::binary); + EXPECT_TRUE(f.is_open()) << "missing fixture: " << name; + std::stringstream buf; + buf << f.rdbuf(); + return buf.str(); +} + +/// setlocale is process-global, so restore it on every exit path. +class ScopedNumericLocale { +public: + explicit ScopedNumericLocale(const char* name) { + const char* previous = std::setlocale(LC_NUMERIC, nullptr); + saved_ = previous != nullptr ? previous : "C"; + applied_ = std::setlocale(LC_NUMERIC, name) != nullptr; + } + ~ScopedNumericLocale() { (void)std::setlocale(LC_NUMERIC, saved_.c_str()); } + ScopedNumericLocale(const ScopedNumericLocale&) = delete; + ScopedNumericLocale& operator=(const ScopedNumericLocale&) = delete; + ScopedNumericLocale(ScopedNumericLocale&&) = delete; + ScopedNumericLocale& operator=(ScopedNumericLocale&&) = delete; + + [[nodiscard]] bool applied() const noexcept { return applied_; } + +private: + std::string saved_; + bool applied_{false}; +}; + +TEST(LocaleIndependence, Day48StaticFeedKeepsItsProbabilityUnderACommaDecimalLocale) { + // The live Day 4-8 feed carries exactly one probability field and it is a + // string: {"DN": 15, "LABEL": "0.15"}. Under a comma-decimal locale + // strtod("0.15") consumes only "0", so the feature was dropped by the + // probability > 0.0 gate and the whole product came back empty. + const ScopedNumericLocale locale{"de_DE.UTF-8"}; + if (!locale.applied()) { + GTEST_SKIP() << "de_DE.UTF-8 is not installed on this host"; + } + + const Day48OutlookPayload payload = parse_day4_8(slurp("day4prob.nolyr.geojson"), 4); + + ASSERT_EQ(payload.features.size(), 1u); + EXPECT_DOUBLE_EQ(payload.features[0].probability, 0.15); +} + +TEST(LocaleIndependence, ProbabilisticOutlookKeepsItsIsoplethsUnderACommaDecimalLocale) { + const ScopedNumericLocale locale{"de_DE.UTF-8"}; + if (!locale.applied()) { + GTEST_SKIP() << "de_DE.UTF-8 is not installed on this host"; + } + + const ProbOutlookPayload payload = + parse_probabilistic(slurp("arcgis_day1_prob_tornado.geojson"), 1, "tornado"); + + ASSERT_GT(payload.features.size(), 0u); + for (const ProbOutlookFeature& f : payload.features) { + EXPECT_GE(f.probability, 0.01); + EXPECT_LE(f.probability, 1.0); + } +} + +TEST(LocaleIndependence, FireWeatherNoRiskSentinelIsStillDroppedUnderACommaDecimalLocale) { + // has_zero_dn requires the whole string to parse; a comma-decimal strtod + // stops at the '.' of "0.0", the full-consume check fails, and the no-risk + // sentinel polygon ships as a real band. + const ScopedNumericLocale locale{"de_DE.UTF-8"}; + if (!locale.applied()) { + GTEST_SKIP() << "de_DE.UTF-8 is not installed on this host"; + } + const std::string body = R"({"features":[ + {"attributes":{"dn":"0.0"},"geometry":{"rings":[[[0,1],[1,1],[1,0],[0,0],[0,1]]]}}, + {"attributes":{"dn":"5"},"geometry":{"rings":[[[2,1],[3,1],[3,0],[2,0],[2,1]]]}} + ]})"; + + const FireWeatherPayload payload = + parse_fire_weather(body, 1, FireWeatherLayer::DryThunderstorm); + + ASSERT_EQ(payload.features.size(), 1u); + EXPECT_EQ(payload.features[0].label, "IDRT"); +} + +} // namespace diff --git a/tests/test_rate_limit.cpp b/tests/test_rate_limit.cpp new file mode 100644 index 0000000..34911f8 --- /dev/null +++ b/tests/test_rate_limit.cpp @@ -0,0 +1,52 @@ +/// @file test_rate_limit.cpp +/// @brief RateLimiter contract: a caller-settable Config must not be able to +/// crash or hang the limiter. + +#include "spc/rate_limit.hpp" + +#include +#include + +namespace { + +using namespace spc; + +TEST(RateLimit, AZeroRefillIntervalIsClampedInsteadOfDividingByZero) { + // Config is a public aggregate with no validation, so a zero interval + // reached `elapsed / refill_interval` and raised SIGFPE on first use. + RateLimiter::Config config; + config.refill_interval = std::chrono::milliseconds{0}; + RateLimiter limiter{config}; + + EXPECT_GT(limiter.config().refill_interval.count(), 0); + EXPECT_TRUE(limiter.try_acquire()); +} + +TEST(RateLimit, InitialTokensAreClampedToTheBucketSize) { + RateLimiter::Config config; + config.max_tokens = 2; + config.initial_tokens = 9; + RateLimiter limiter{config}; + + EXPECT_EQ(limiter.available_tokens(), 2); +} + +TEST(RateLimit, AnExhaustedDailyQuotaFailsFastInsteadOfWaitingForMidnight) { + // try_acquire() returns false permanently once the daily quota is spent, + // and only a UTC-midnight rollover clears it. acquire() with no max_wait + // used to busy-wait toward that rollover; it must give up at once. + RateLimiter::Config config; + config.daily_limit = 1; + RateLimiter limiter{config}; + + ASSERT_TRUE(limiter.acquire()); + + const std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now(); + EXPECT_FALSE(limiter.acquire()); + EXPECT_FALSE(limiter.acquire_for(std::chrono::milliseconds{5000})); + const std::chrono::milliseconds waited = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start); + EXPECT_LT(waited.count(), 1000); +} + +} // namespace diff --git a/tests/test_retry.cpp b/tests/test_retry.cpp new file mode 100644 index 0000000..ce7892a --- /dev/null +++ b/tests/test_retry.cpp @@ -0,0 +1,121 @@ +/// @file test_retry.cpp +/// @brief Retry arithmetic and the Retry-After contract. + +#include "spc/http_client.hpp" +#include "spc/retry.hpp" + +#include +#include +#include +#include + +namespace { + +using namespace spc; + +TEST(Retry, JitterCannotPushTheDelayPastMaxDelay) { + // The clamp used to run before the jitter multiply, so a delay pinned at + // max_delay came back up to jitter_factor above the documented ceiling. + RetryPolicy policy; + policy.initial_delay = std::chrono::milliseconds{10000}; + policy.max_delay = std::chrono::milliseconds{30000}; + policy.jitter_factor = 0.5; + + for (std::uint8_t attempt = 1; attempt <= 8; ++attempt) { + for (int sample = 0; sample < 64; ++sample) { + const std::chrono::milliseconds delay = calculate_retry_delay(attempt, policy); + EXPECT_LE(delay.count(), policy.max_delay.count()); + EXPECT_GE(delay.count(), 0); + } + } +} + +TEST(Retry, ZeroMaxAttemptsStillPerformsTheRequestOnce) { + // The loop `for (attempt = 1; attempt <= max_attempts; ...)` never ran, so + // with_retry reported a network error for a request never made. + RetryPolicy policy; + policy.max_attempts = 0; + int calls = 0; + + const Result result = with_retry( + [&]() -> Result { + ++calls; + return HttpResponse{200, "{}", {}}; + }, + policy); + + EXPECT_EQ(calls, 1); + ASSERT_TRUE(result); + EXPECT_EQ(result->status_code, 200); +} + +TEST(Retry, MaxAttemptsAtTheTopOfTheRangeTerminates) { + // The review claimed `++attempt` wrapping 255 -> 0 made this loop run + // forever. It does not: the `attempt < max_attempts` guard returns the + // result at attempt 255 before the counter can wrap. Pinned so the wider + // loop counter keeps that true. + RetryPolicy policy; + policy.max_attempts = 255; + policy.initial_delay = std::chrono::milliseconds{0}; + policy.max_delay = std::chrono::milliseconds{0}; + policy.jitter_factor = 0.0; + int calls = 0; + + const Result result = with_retry( + [&]() -> Result { + ++calls; + return std::unexpected(Error::network("boom")); + }, + policy); + + EXPECT_EQ(calls, 255); + ASSERT_FALSE(result); +} + +TEST(Retry, HonoursRetryAfterInsteadOfTheComputedBackoff) { + // 429 and 503 are exactly the responses that carry Retry-After, and the + // SDK already captures every header. A server asking for 60 s was being + // retried after 200 ms. + RetryPolicy policy; + policy.max_attempts = 2; + policy.initial_delay = std::chrono::milliseconds{1}; + policy.max_delay = std::chrono::milliseconds{150}; + policy.jitter_factor = 0.0; + + const std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now(); + const Result result = with_retry( + []() -> Result { + return HttpResponse{429, "slow down", {{"Retry-After", "60"}}}; + }, + policy); + const std::chrono::milliseconds waited = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start); + + ASSERT_TRUE(result); + EXPECT_EQ(result->status_code, 429); + // 60 s clamped to max_delay, not the 1 ms the backoff would have chosen. + EXPECT_GE(waited.count(), 100); +} + +TEST(Retry, IgnoresAnUnparseableRetryAfterAndFallsBackToTheBackoff) { + RetryPolicy policy; + policy.max_attempts = 2; + policy.initial_delay = std::chrono::milliseconds{1}; + policy.max_delay = std::chrono::milliseconds{5000}; + policy.jitter_factor = 0.0; + + const std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now(); + const Result result = with_retry( + []() -> Result { + // The HTTP-date form, which the SDK does not parse. + return HttpResponse{503, "", {{"retry-after", "Wed, 21 Oct 2026 07:28:00 GMT"}}}; + }, + policy); + const std::chrono::milliseconds waited = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start); + + ASSERT_TRUE(result); + EXPECT_LT(waited.count(), 1000); +} + +} // namespace