fix: cover api edge cases
This commit is contained in:
@@ -2,11 +2,14 @@ use crate::{
|
||||
clients::{funtranslations::TranslationClient, pokeapi::PokeApiClient},
|
||||
domain::{PokemonInfo, TranslationKind},
|
||||
error::AppError,
|
||||
telemetry::AppMetrics,
|
||||
telemetry::{AppMetrics, UpstreamRequestOutcome},
|
||||
};
|
||||
use std::time::Instant;
|
||||
use tracing::{debug, instrument};
|
||||
|
||||
// The application service depends on concrete clients to keep this challenge
|
||||
// concise. In a larger enterprise codebase these would become trait-based ports
|
||||
// so adapters could be swapped without touching business logic.
|
||||
#[derive(Clone)]
|
||||
pub struct PokedexService {
|
||||
pokemon: PokeApiClient,
|
||||
@@ -46,7 +49,11 @@ impl PokedexService {
|
||||
self.metrics.record_upstream_request(
|
||||
"funtranslations",
|
||||
translation_operation(translation_kind),
|
||||
translation.is_ok(),
|
||||
if translation.is_ok() {
|
||||
UpstreamRequestOutcome::Success
|
||||
} else {
|
||||
UpstreamRequestOutcome::Error
|
||||
},
|
||||
translation_start.elapsed(),
|
||||
);
|
||||
|
||||
@@ -61,16 +68,34 @@ impl PokedexService {
|
||||
async fn fetch_pokemon(&self, name: &str) -> Result<PokemonInfo, AppError> {
|
||||
let start = Instant::now();
|
||||
let pokemon = self.pokemon.get_pokemon_info(name).await;
|
||||
self.metrics.record_upstream_request(
|
||||
"pokeapi",
|
||||
"pokemon_species",
|
||||
pokemon.is_ok(),
|
||||
start.elapsed(),
|
||||
);
|
||||
if !matches!(pokemon, Err(AppError::BadRequest(_))) {
|
||||
self.metrics.record_upstream_request(
|
||||
"pokeapi",
|
||||
"pokemon_species",
|
||||
upstream_outcome_for_pokemon(&pokemon),
|
||||
start.elapsed(),
|
||||
);
|
||||
}
|
||||
pokemon
|
||||
}
|
||||
}
|
||||
|
||||
const fn upstream_outcome_for_pokemon(
|
||||
result: &Result<PokemonInfo, AppError>,
|
||||
) -> UpstreamRequestOutcome {
|
||||
match result {
|
||||
Ok(_) => UpstreamRequestOutcome::Success,
|
||||
Err(AppError::NotFound) => UpstreamRequestOutcome::NotFound,
|
||||
Err(
|
||||
AppError::BadRequest(_)
|
||||
| AppError::InvalidUpstreamData(_)
|
||||
| AppError::Upstream(_)
|
||||
| AppError::Timeout
|
||||
| AppError::Internal,
|
||||
) => UpstreamRequestOutcome::Error,
|
||||
}
|
||||
}
|
||||
|
||||
const fn translation_operation(kind: TranslationKind) -> &'static str {
|
||||
match kind {
|
||||
TranslationKind::Shakespeare => "shakespeare",
|
||||
|
||||
51
src/clients/body.rs
Normal file
51
src/clients/body.rs
Normal file
@@ -0,0 +1,51 @@
|
||||
use crate::error::AppError;
|
||||
use serde::de::DeserializeOwned;
|
||||
|
||||
const MAX_UPSTREAM_BODY_BYTES: usize = 1024 * 1024;
|
||||
|
||||
pub async fn read_json<T>(
|
||||
mut response: reqwest::Response,
|
||||
service: &'static str,
|
||||
) -> Result<T, AppError>
|
||||
where
|
||||
T: DeserializeOwned,
|
||||
{
|
||||
if response
|
||||
.content_length()
|
||||
.is_some_and(|length| length > MAX_UPSTREAM_BODY_BYTES as u64)
|
||||
{
|
||||
return Err(AppError::Upstream(format!(
|
||||
"{service} response body exceeded {MAX_UPSTREAM_BODY_BYTES} bytes"
|
||||
)));
|
||||
}
|
||||
|
||||
let mut body = Vec::new();
|
||||
while let Some(chunk) = response
|
||||
.chunk()
|
||||
.await
|
||||
.map_err(|error| map_reqwest_error(&error))?
|
||||
{
|
||||
let next_len = body
|
||||
.len()
|
||||
.checked_add(chunk.len())
|
||||
.ok_or(AppError::Internal)?;
|
||||
if next_len > MAX_UPSTREAM_BODY_BYTES {
|
||||
return Err(AppError::Upstream(format!(
|
||||
"{service} response body exceeded {MAX_UPSTREAM_BODY_BYTES} bytes"
|
||||
)));
|
||||
}
|
||||
body.extend_from_slice(&chunk);
|
||||
}
|
||||
|
||||
serde_json::from_slice(&body).map_err(|error| {
|
||||
AppError::InvalidUpstreamData(format!("{service} returned invalid JSON: {error}"))
|
||||
})
|
||||
}
|
||||
|
||||
fn map_reqwest_error(error: &reqwest::Error) -> AppError {
|
||||
if error.is_timeout() {
|
||||
AppError::Timeout
|
||||
} else {
|
||||
AppError::Upstream(error.to_string())
|
||||
}
|
||||
}
|
||||
@@ -1,4 +1,4 @@
|
||||
use crate::{domain::TranslationKind, error::AppError};
|
||||
use crate::{clients::body, domain::TranslationKind, error::AppError};
|
||||
use serde::Deserialize;
|
||||
use std::time::Duration;
|
||||
use tracing::instrument;
|
||||
@@ -40,11 +40,14 @@ impl TranslationClient {
|
||||
)));
|
||||
}
|
||||
|
||||
response
|
||||
.json::<TranslationResponse>()
|
||||
.await
|
||||
.map(|payload| payload.contents.translated)
|
||||
.map_err(|error| map_reqwest_error(&error))
|
||||
let payload = body::read_json::<TranslationResponse>(response, "FunTranslations").await?;
|
||||
if payload.contents.translated.trim().is_empty() {
|
||||
Err(AppError::InvalidUpstreamData(
|
||||
"FunTranslations returned an empty translation".to_owned(),
|
||||
))
|
||||
} else {
|
||||
Ok(payload.contents.translated)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -112,6 +115,26 @@ mod tests {
|
||||
assert_eq!(translated, "Created, it was.");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn rejects_empty_translations_so_callers_can_fallback() {
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(matchers::method("POST"))
|
||||
.and(matchers::path("/translate/yoda.json"))
|
||||
.respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({
|
||||
"contents": { "translated": " " }
|
||||
})))
|
||||
.mount(&server)
|
||||
.await;
|
||||
let client = client_for(&server);
|
||||
|
||||
let error = client
|
||||
.translate(TranslationKind::Yoda, "hello")
|
||||
.await
|
||||
.expect_err("empty translation should fail");
|
||||
|
||||
assert!(error.to_string().contains("empty translation"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn maps_translation_http_failures_to_upstream_errors() {
|
||||
let server = MockServer::start().await;
|
||||
|
||||
@@ -1,2 +1,4 @@
|
||||
mod body;
|
||||
|
||||
pub mod funtranslations;
|
||||
pub mod pokeapi;
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
use crate::{domain::PokemonInfo, error::AppError};
|
||||
use crate::{clients::body, domain::PokemonInfo, error::AppError};
|
||||
use serde::Deserialize;
|
||||
use std::time::Duration;
|
||||
use tracing::instrument;
|
||||
@@ -44,12 +44,9 @@ impl PokeApiClient {
|
||||
)));
|
||||
}
|
||||
|
||||
let species = response
|
||||
.json::<PokemonSpeciesResponse>()
|
||||
body::read_json::<PokemonSpeciesResponse>(response, "PokéAPI")
|
||||
.await
|
||||
.map_err(|error| map_reqwest_error(&error))?;
|
||||
|
||||
species.try_into()
|
||||
.and_then(TryInto::try_into)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -273,6 +270,24 @@ mod tests {
|
||||
assert!(matches!(error, AppError::InvalidUpstreamData(_)));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn rejects_oversized_upstream_bodies() {
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(matchers::method("GET"))
|
||||
.and(matchers::path("/pokemon-species/snorlax"))
|
||||
.respond_with(ResponseTemplate::new(200).set_body_string("x".repeat(1025 * 1024)))
|
||||
.mount(&server)
|
||||
.await;
|
||||
let client = client_for(&server);
|
||||
|
||||
let error = client
|
||||
.get_pokemon_info("snorlax")
|
||||
.await
|
||||
.expect_err("oversized body should fail");
|
||||
|
||||
assert!(error.to_string().contains("exceeded"));
|
||||
}
|
||||
|
||||
fn client_for(server: &MockServer) -> PokeApiClient {
|
||||
PokeApiClient::new(server.uri(), Duration::from_secs(1)).expect("client should build")
|
||||
}
|
||||
|
||||
@@ -70,7 +70,7 @@ fn default_bind_addr() -> SocketAddr {
|
||||
}
|
||||
|
||||
const fn validate_rate_limit(per_second: f64, burst: u32) -> Result<(), ConfigError> {
|
||||
if per_second.is_sign_positive() && burst > 0 {
|
||||
if per_second.is_finite() && per_second > 0.0 && burst > 0 {
|
||||
Ok(())
|
||||
} else {
|
||||
Err(ConfigError::InvalidRateLimit)
|
||||
|
||||
@@ -13,7 +13,8 @@ pub(super) async fn enforce_rate_limit(
|
||||
req: Request,
|
||||
next: middleware::Next,
|
||||
) -> Response {
|
||||
if state.rate_limiter.try_acquire() {
|
||||
let path = req.uri().path();
|
||||
if matches!(path, "/health" | "/metrics") || state.rate_limiter.try_acquire() {
|
||||
next.run(req).await
|
||||
} else {
|
||||
(
|
||||
|
||||
@@ -66,7 +66,7 @@ pub fn create_app(state: AppState) -> Router {
|
||||
tracing::info_span!(
|
||||
"http.request",
|
||||
method = %request.method(),
|
||||
uri = %request.uri(),
|
||||
path = %request.uri().path(),
|
||||
request_id = %request_id,
|
||||
)
|
||||
})
|
||||
|
||||
@@ -1,5 +1,8 @@
|
||||
use std::{sync::Mutex, time::Instant};
|
||||
|
||||
// This challenge uses a simple in-memory global limiter. In production,
|
||||
// prefer API gateway rate limiting so limits are shared across instances and
|
||||
// identity-aware policies can be keyed by API key, user, or client IP.
|
||||
#[derive(Debug)]
|
||||
pub struct RateLimiter {
|
||||
bucket: Mutex<TokenBucket>,
|
||||
@@ -7,7 +10,7 @@ pub struct RateLimiter {
|
||||
|
||||
impl RateLimiter {
|
||||
pub fn new(refill_per_second: f64, burst: u32) -> Result<Self, RateLimitError> {
|
||||
if !refill_per_second.is_sign_positive() || burst == 0 {
|
||||
if !refill_per_second.is_finite() || refill_per_second <= 0.0 || burst == 0 {
|
||||
return Err(RateLimitError::InvalidConfiguration);
|
||||
}
|
||||
|
||||
@@ -69,6 +72,13 @@ mod tests {
|
||||
use super::RateLimiter;
|
||||
use std::time::Duration;
|
||||
|
||||
#[test]
|
||||
fn rejects_non_positive_or_non_finite_configuration() {
|
||||
assert!(RateLimiter::new(0.0, 1).is_err());
|
||||
assert!(RateLimiter::new(f64::INFINITY, 1).is_err());
|
||||
assert!(RateLimiter::new(1.0, 0).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn allows_requests_up_to_the_burst_capacity() {
|
||||
let limiter = RateLimiter::new(100.0, 2).expect("rate limiter should build");
|
||||
@@ -84,6 +94,8 @@ mod tests {
|
||||
|
||||
assert!(limiter.try_acquire());
|
||||
assert!(!limiter.try_acquire());
|
||||
// A production-grade limiter would inject a clock; a short sleep keeps
|
||||
// this challenge implementation simple without coupling tests to internals.
|
||||
std::thread::sleep(Duration::from_millis(20));
|
||||
assert!(limiter.try_acquire());
|
||||
}
|
||||
|
||||
@@ -11,6 +11,27 @@ pub struct AppMetrics {
|
||||
upstream_request_duration: opentelemetry::metrics::Histogram<f64>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
|
||||
pub enum UpstreamRequestOutcome {
|
||||
Success,
|
||||
NotFound,
|
||||
Error,
|
||||
}
|
||||
|
||||
impl UpstreamRequestOutcome {
|
||||
const fn label(self) -> &'static str {
|
||||
match self {
|
||||
Self::Success => "ok",
|
||||
Self::NotFound => "not_found",
|
||||
Self::Error => "error",
|
||||
}
|
||||
}
|
||||
|
||||
const fn is_error(self) -> bool {
|
||||
matches!(self, Self::Error)
|
||||
}
|
||||
}
|
||||
|
||||
impl AppMetrics {
|
||||
#[must_use]
|
||||
pub fn new(meter: &Meter) -> Self {
|
||||
@@ -68,10 +89,10 @@ impl AppMetrics {
|
||||
&self,
|
||||
service: &'static str,
|
||||
operation: &'static str,
|
||||
success: bool,
|
||||
outcome: UpstreamRequestOutcome,
|
||||
duration: Duration,
|
||||
) {
|
||||
let status = if success { "ok" } else { "error" };
|
||||
let status = outcome.label();
|
||||
let attributes = [
|
||||
KeyValue::new("service", service),
|
||||
KeyValue::new("operation", operation),
|
||||
@@ -80,7 +101,7 @@ impl AppMetrics {
|
||||
self.upstream_requests.add(1, &attributes);
|
||||
self.upstream_request_duration
|
||||
.record(duration.as_secs_f64(), &attributes);
|
||||
if !success {
|
||||
if outcome.is_error() {
|
||||
self.upstream_errors.add(1, &attributes);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -6,7 +6,7 @@ use opentelemetry::metrics::MeterProvider;
|
||||
use opentelemetry_prometheus_text_exporter::PrometheusExporter;
|
||||
use opentelemetry_sdk::{Resource, metrics::SdkMeterProvider};
|
||||
|
||||
pub use metrics::AppMetrics;
|
||||
pub use metrics::{AppMetrics, UpstreamRequestOutcome};
|
||||
pub use tracing::init_tracing;
|
||||
|
||||
pub struct Telemetry {
|
||||
|
||||
Reference in New Issue
Block a user