From e8737c09aeb2aaa428c94757095fd4d91f537407 Mon Sep 17 00:00:00 2001 From: Florian Eich Date: Sun, 12 Apr 2026 20:28:09 +0200 Subject: [PATCH 1/2] fix(weather): avoid percent-encoding commas in place name geocoding The OpenWeatherMap geo API expects literal commas in the `q` parameter (e.g. "London,UK"), but `query_pairs_mut().append_pair()` percent-encodes them into "%2C", causing the request to 404. Build the query string with `format!` and `Url::join` instead, which preserves the commas. Also add a trailing slash to `GEO_URL` so that `join` resolves relative paths correctly. Adds an `#[ignore]`d integration test that verifies place-based geocoding against the live API. --- src/blocks/weather/open_weather_map.rs | 46 ++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/src/blocks/weather/open_weather_map.rs b/src/blocks/weather/open_weather_map.rs index 51980c890..d166e5dd4 100644 --- a/src/blocks/weather/open_weather_map.rs +++ b/src/blocks/weather/open_weather_map.rs @@ -3,7 +3,7 @@ use chrono::{DateTime, Utc}; use reqwest::Url; use serde::{Deserializer, de}; -pub(super) const GEO_URL: &str = "https://api.openweathermap.org/geo/1.0"; +pub(super) const GEO_URL: &str = "https://api.openweathermap.org/geo/1.0/"; pub(super) const CURRENT_URL: &str = "https://api.openweathermap.org/data/2.5/weather"; pub(super) const FORECAST_URL: &str = "https://api.openweathermap.org/data/2.5/forecast"; pub(super) const API_KEY_ENV: &str = "OPENWEATHERMAP_API_KEY"; @@ -142,16 +142,18 @@ impl<'a> Service<'a> { )?)); } - let geo_url = - Url::parse(GEO_URL).error("Failed to parse the hard-coded constant GEO_URL")?; + let geo_url = Url::parse(GEO_URL).error("Failed to parse the hard-coded GEO_URL")?; // Try by place name if let Some(place) = config.place.as_ref() { // "{GEO_URL}/direct?q={place}&appid={api_key}" - let mut url = geo_url.join("direct").error("Failed to join geo_url")?; - url.query_pairs_mut() - .append_pair("q", place) - .append_pair("appid", api_key); + // + // Cannot use reqwest's `query_pairs_mut().append_pair()` because it percent-encodes + // the comma in the place name (ie., it turns "London,UK" into "London%2CUK"), which + // causes the request to 404. + let url = geo_url + .join(&format!("direct?q={place}&appid={api_key}")) + .error("Failed to join geo_url")?; let city: Option = REQWEST_CLIENT .get(url) @@ -194,12 +196,15 @@ impl<'a> Service<'a> { fn getenv_openweathermap_api_key() -> Option { std::env::var(API_KEY_ENV).ok() } + fn getenv_openweathermap_city_id() -> Option { std::env::var(CITY_ID_ENV).ok() } + fn getenv_openweathermap_place() -> Option { std::env::var(PLACE_ENV).ok() } + fn getenv_openweathermap_zip() -> Option { std::env::var(ZIP_ENV).ok() } @@ -416,3 +421,30 @@ fn weather_to_icon(weather: &str, is_night: bool) -> WeatherIcon { _ => WeatherIcon::Default, } } + +#[cfg(test)] +mod tests { + use super::*; + + #[ignore] + #[tokio::test] + async fn using_place_resolves_correctly() -> Result<()> { + let api_key = getenv_openweathermap_api_key(); + let config = Config { + api_key, + place: Some("Zurich,CH".to_string()), + ..Default::default() + }; + + let Some(LocationSpecifier::CityCoord(CityCoord { lat, lon })) = + Service::get_location_query(false, &api_key.unwrap(), &config).await? + else { + panic!("no location specifier found (eg., OpenWeatherMap returned empty result)"); + }; + + assert_eq!(&format!("{lat:.1}"), "47.4"); + assert_eq!(&format!("{lon:.1}"), "8.5"); + + Ok(()) + } +} From 7d6d3c331342fb27f143dccd6af2f14dbba6528c Mon Sep 17 00:00:00 2001 From: Florian Eich Date: Sun, 12 Apr 2026 20:28:57 +0200 Subject: [PATCH 2/2] refactor(weather): extract `get_location_query` into a free function Move `Service::get_location_query` out of the impl block into a standalone async function. Use `api_key` from provided `Config`, instead of needlessly taking it as a parameter. --- src/blocks/weather/open_weather_map.rs | 156 +++++++++++++------------ 1 file changed, 79 insertions(+), 77 deletions(-) diff --git a/src/blocks/weather/open_weather_map.rs b/src/blocks/weather/open_weather_map.rs index d166e5dd4..baef2ee21 100644 --- a/src/blocks/weather/open_weather_map.rs +++ b/src/blocks/weather/open_weather_map.rs @@ -114,83 +114,10 @@ impl<'a> Service<'a> { api_key, units: &config.units, lang: &config.lang, - location_query: Service::get_location_query(autolocate, api_key, config).await?, + location_query: get_location_query(autolocate, config).await?, forecast_hours: config.forecast_hours, }) } - - async fn get_location_query( - autolocate: bool, - api_key: &str, - config: &Config, - ) -> Result> { - if autolocate { - return Ok(None); - } - - // Try by coordinates from config - if let Some((lat, lon)) = config.coordinates.as_ref() { - return Ok(Some(LocationSpecifier::try_from((lat, lon)).error( - "Invalid coordinates: failed to parse latitude or longitude from string to f64", - )?)); - } - - // Try by city ID from config - if let Some(id) = config.city_id.as_ref() { - return Ok(Some(LocationSpecifier::try_from(id).error( - "Invalid city id: failed to parse it from string to u32", - )?)); - } - - let geo_url = Url::parse(GEO_URL).error("Failed to parse the hard-coded GEO_URL")?; - - // Try by place name - if let Some(place) = config.place.as_ref() { - // "{GEO_URL}/direct?q={place}&appid={api_key}" - // - // Cannot use reqwest's `query_pairs_mut().append_pair()` because it percent-encodes - // the comma in the place name (ie., it turns "London,UK" into "London%2CUK"), which - // causes the request to 404. - let url = geo_url - .join(&format!("direct?q={place}&appid={api_key}")) - .error("Failed to join geo_url")?; - - let city: Option = REQWEST_CLIENT - .get(url) - .send() - .await - .error("Geo request failed")? - .json::>() - .await - .error("Geo failed to parse JSON")? - .first() - .map(|city| LocationSpecifier::CityCoord(*city)); - - return Ok(city); - } - - // Try by zip code - if let Some(zip) = config.zip.as_ref() { - // "{GEO_URL}/zip?zip={zip}&appid={api_key}" - let mut url = geo_url.join("zip").error("Failed to join geo_url")?; - url.query_pairs_mut() - .append_pair("zip", zip) - .append_pair("appid", api_key); - - let city: CityCoord = REQWEST_CLIENT - .get(url) - .send() - .await - .error("Geo request failed")? - .json() - .await - .error("Geo failed to parse JSON")?; - - return Ok(Some(LocationSpecifier::CityCoord(city))); - } - - Ok(None) - } } fn getenv_openweathermap_api_key() -> Option { @@ -209,6 +136,82 @@ fn getenv_openweathermap_zip() -> Option { std::env::var(ZIP_ENV).ok() } +async fn get_location_query( + autolocate: bool, + config: &Config, +) -> Result> { + if autolocate { + return Ok(None); + } + + // Try by coordinates from config + if let Some((lat, lon)) = config.coordinates.as_ref() { + return Ok(Some(LocationSpecifier::try_from((lat, lon)).error( + "Invalid coordinates: failed to parse latitude or longitude from string to f64", + )?)); + } + + // Try by city ID from config + if let Some(id) = config.city_id.as_ref() { + return Ok(Some(LocationSpecifier::try_from(id).error( + "Invalid city id: failed to parse it from string to u32", + )?)); + } + + let geo_url = Url::parse(GEO_URL).error("Failed to parse the hard-coded GEO_URL")?; + let api_key = config + .api_key + .as_ref() + .error("API key not provided (eg. via API_KEY_ENV environment variable)")?; + + // Try by place name + if let Some(place) = config.place.as_ref() { + // "{GEO_URL}/direct?q={place}&appid={api_key}" + // + // Cannot use reqwest's `query_pairs_mut().append_pair()` because it percent-encodes + // the comma in the place name (ie., it turns "London,UK" into "London%2CUK"), which + // causes the request to 404. + let url = geo_url + .join(&format!("direct?q={place}&appid={api_key}")) + .error("Failed to join geo_url")?; + + let city: Option = REQWEST_CLIENT + .get(url) + .send() + .await + .error("Geo request failed")? + .json::>() + .await + .error("Geo failed to parse JSON")? + .first() + .map(|city| LocationSpecifier::CityCoord(*city)); + + return Ok(city); + } + + // Try by zip code + if let Some(zip) = config.zip.as_ref() { + // "{GEO_URL}/zip?zip={zip}&appid={api_key}" + let mut url = geo_url.join("zip").error("Failed to join geo_url")?; + url.query_pairs_mut() + .append_pair("zip", zip) + .append_pair("appid", api_key); + + let city: CityCoord = REQWEST_CLIENT + .get(url) + .send() + .await + .error("Geo request failed")? + .json() + .await + .error("Geo failed to parse JSON")?; + + return Ok(Some(LocationSpecifier::CityCoord(city))); + } + + Ok(None) +} + #[derive(Deserialize, Debug)] struct ApiForecastResponse { list: Vec, @@ -429,15 +432,14 @@ mod tests { #[ignore] #[tokio::test] async fn using_place_resolves_correctly() -> Result<()> { - let api_key = getenv_openweathermap_api_key(); let config = Config { - api_key, + api_key: getenv_openweathermap_api_key(), place: Some("Zurich,CH".to_string()), ..Default::default() }; let Some(LocationSpecifier::CityCoord(CityCoord { lat, lon })) = - Service::get_location_query(false, &api_key.unwrap(), &config).await? + get_location_query(false, &config).await? else { panic!("no location specifier found (eg., OpenWeatherMap returned empty result)"); };