diff --git a/.github/workflows/format.yml b/.github/workflows/format.yml index 45ab1aea..8021f2c5 100644 --- a/.github/workflows/format.yml +++ b/.github/workflows/format.yml @@ -41,8 +41,8 @@ jobs: - name: Run cargo fmt uses: actions-rust-lang/rustfmt@v1 - - name: Run cargo clipply - run: cargo clippy --all-targets --all-features + - name: Run cargo clippy + run: cargo clippy --all-targets --all-features -- -D warnings format-typescript: runs-on: ubuntu-latest diff --git a/Cargo.toml b/Cargo.toml index 9545c847..5133b733 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,6 +12,28 @@ default-members = [ "crates/fastly", ] +[workspace.lints.clippy] +# Correctness - Force explicit error handling in production code +unwrap_used = "deny" +expect_used = "allow" # Allow expect with context message +panic = "deny" + +# Style +module_name_repetitions = "allow" +must_use_candidate = "warn" + +# Pedantic (selective) +doc_markdown = "warn" +missing_errors_doc = "warn" +missing_panics_doc = "warn" +needless_pass_by_value = "warn" # Encourage borrowing over ownership +redundant_closure_for_method_calls = "warn" + +# Restriction (selective) +print_stdout = "warn" +print_stderr = "warn" +dbg_macro = "warn" + [profile.release] debug = 1 diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 00000000..07564932 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,40 @@ +# Clippy configuration for trusted-server +# See: https://doc.rust-lang.org/clippy/configuration.html + +# ============================================================================= +# Complexity Thresholds +# ============================================================================= + +# Cognitive complexity threshold for functions (default: 25) +# Set to 30 to accommodate existing complex HTML/RSC processing functions +# like `create_html_processor` and `rewrite_rsc_scripts_combined`. +# Consider refactoring functions that exceed this threshold. +cognitive-complexity-threshold = 30 + +# Maximum number of lines in a function (default: 100) +# Set to 200 to allow larger handler functions that process HTTP requests +# with multiple validation steps. Prefer extracting helpers when possible. +too-many-lines-threshold = 200 + +# ============================================================================= +# Test Allowances +# ============================================================================= + +# Allow expect() in tests for clearer failure messages than unwrap() +allow-expect-in-tests = true + +# Allow unwrap() in tests where panicking on failure is acceptable +allow-unwrap-in-tests = true + +# ============================================================================= +# Future Considerations +# ============================================================================= +# +# As the codebase matures, consider tightening these thresholds: +# - cognitive-complexity-threshold = 25 (default) +# - too-many-lines-threshold = 100 (default) +# +# Additional lints to consider enabling in Cargo.toml: +# - needless_pass_by_value = "warn" +# - redundant_closure_for_method_calls = "warn" +# - missing_const_for_fn = "warn" diff --git a/crates/common/Cargo.toml b/crates/common/Cargo.toml index ab3058ce..17a0e04b 100644 --- a/crates/common/Cargo.toml +++ b/crates/common/Cargo.toml @@ -8,6 +8,9 @@ edition = "2021" publish = false license = "Apache-2.0" +[lints] +workspace = true + [dependencies] base64 = { workspace = true } brotli = { workspace = true } diff --git a/crates/common/build.rs b/crates/common/build.rs index a97734b7..4bac64f0 100644 --- a/crates/common/build.rs +++ b/crates/common/build.rs @@ -1,3 +1,5 @@ +#![allow(clippy::unwrap_used, clippy::panic)] + #[path = "src/error.rs"] mod error; @@ -29,7 +31,7 @@ fn rerun_if_changed() { let settings_json = serde_json::to_value(&default_settings).unwrap(); let mut env_vars = HashSet::new(); - collect_env_vars(&settings_json, &mut env_vars, vec![]); + collect_env_vars(&settings_json, &mut env_vars, &[]); // Print rerun-if-env-changed for each variable let mut sorted_vars: Vec<_> = env_vars.into_iter().collect(); @@ -60,10 +62,10 @@ fn merge_toml() { fs::write(dest_path, merged_toml).unwrap_or_else(|_| panic!("Failed to write {:?}", dest_path)); } -fn collect_env_vars(value: &Value, env_vars: &mut HashSet, path: Vec) { +fn collect_env_vars(value: &Value, env_vars: &mut HashSet, path: &[String]) { if let Value::Object(map) = value { for (key, val) in map { - let mut new_path = path.clone(); + let mut new_path = path.to_owned(); new_path.push(key.to_uppercase()); match val { @@ -79,7 +81,7 @@ fn collect_env_vars(value: &Value, env_vars: &mut HashSet, path: Vec { // Recurse into nested objects - collect_env_vars(val, env_vars, new_path); + collect_env_vars(val, env_vars, &new_path); } _ => {} } diff --git a/crates/common/src/auction/config.rs b/crates/common/src/auction/config.rs index 8ffe5a5f..0262f33a 100644 --- a/crates/common/src/auction/config.rs +++ b/crates/common/src/auction/config.rs @@ -1,6 +1,6 @@ //! Configuration structures for auction orchestration. //! -//! The base types are defined in auction_config_types.rs to avoid circular dependencies -//! with build.rs. This module re-exports them. +//! The base types are defined in `auction_config_types.rs` to avoid circular dependencies +//! with `build.rs`. This module re-exports them. pub use crate::auction_config_types::AuctionConfig; diff --git a/crates/common/src/auction/endpoints.rs b/crates/common/src/auction/endpoints.rs index 29aa18bc..c2175caf 100644 --- a/crates/common/src/auction/endpoints.rs +++ b/crates/common/src/auction/endpoints.rs @@ -15,7 +15,15 @@ use super::AuctionOrchestrator; /// /// This is the main entry point for running header bidding auctions. /// It orchestrates bids from multiple providers (Prebid, APS, GAM, etc.) and returns -/// the winning bids in OpenRTB format with creative HTML inline in the `adm` field. +/// the winning bids in `OpenRTB` format with creative HTML inline in the `adm` field. +/// +/// # Errors +/// +/// Returns an error if: +/// - The request body cannot be parsed +/// - The auction request conversion fails (e.g., invalid ad units) +/// - The auction execution fails +/// - The response cannot be serialized pub async fn handle_auction( settings: &Settings, orchestrator: &AuctionOrchestrator, diff --git a/crates/common/src/auction/formats.rs b/crates/common/src/auction/formats.rs index a9eece28..6b446f05 100644 --- a/crates/common/src/auction/formats.rs +++ b/crates/common/src/auction/formats.rs @@ -2,7 +2,7 @@ //! //! This module handles: //! - Parsing incoming tsjs/Prebid.js format requests -//! - Converting internal auction results to OpenRTB 2.x responses +//! - Converting internal auction results to `OpenRTB` 2.x responses use error_stack::{ensure, Report, ResultExt}; use fastly::http::{header, StatusCode}; @@ -63,7 +63,13 @@ pub struct BannerUnit { pub sizes: Vec>, } -/// Convert tsjs/Prebid.js request format to internal AuctionRequest. +/// Convert tsjs/Prebid.js request format to internal `AuctionRequest`. +/// +/// # Errors +/// +/// Returns an error if: +/// - Synthetic ID generation fails +/// - Request contains invalid banner sizes (must be [width, height]) pub fn convert_tsjs_to_auction_request( body: &AdRequest, settings: &Settings, @@ -122,7 +128,9 @@ pub fn convert_tsjs_to_auction_request( // Get geo info if available let device = GeoInfo::from_request(req).map(|geo| DeviceInfo { - user_agent: req.get_header_str("user-agent").map(|s| s.to_string()), + user_agent: req + .get_header_str("user-agent") + .map(std::string::ToString::to_string), ip: req.get_client_ip_addr().map(|ip| ip.to_string()), geo: Some(geo), }); @@ -148,9 +156,15 @@ pub fn convert_tsjs_to_auction_request( }) } -/// Convert OrchestrationResult to OpenRTB response format. +/// Convert `OrchestrationResult` to `OpenRTB` response format. /// /// Returns rewritten creative HTML directly in the `adm` field for inline delivery. +/// +/// # Errors +/// +/// Returns an error if: +/// - A winning bid is missing a price +/// - The response serialization fails pub fn convert_to_openrtb_response( result: &OrchestrationResult, settings: &Settings, diff --git a/crates/common/src/auction/mod.rs b/crates/common/src/auction/mod.rs index 36187862..e78f2ecf 100644 --- a/crates/common/src/auction/mod.rs +++ b/crates/common/src/auction/mod.rs @@ -47,6 +47,7 @@ fn provider_builders() -> &'static [ProviderBuilder] { /// /// # Arguments /// * `settings` - Application settings used to configure the orchestrator and providers +#[must_use] pub fn build_orchestrator(settings: &Settings) -> AuctionOrchestrator { log::info!("Building auction orchestrator"); diff --git a/crates/common/src/auction/orchestrator.rs b/crates/common/src/auction/orchestrator.rs index a6614709..c53cc9b9 100644 --- a/crates/common/src/auction/orchestrator.rs +++ b/crates/common/src/auction/orchestrator.rs @@ -20,6 +20,7 @@ pub struct AuctionOrchestrator { impl AuctionOrchestrator { /// Create a new orchestrator with the given configuration. + #[must_use] pub fn new(config: AuctionConfig) -> Self { Self { config, @@ -35,6 +36,7 @@ impl AuctionOrchestrator { } /// Get the number of registered providers. + #[must_use] pub fn provider_count(&self) -> usize { self.providers.len() } @@ -44,6 +46,11 @@ impl AuctionOrchestrator { /// Strategy is determined by mediator configuration: /// - If mediator is configured: runs parallel mediation (bidders → mediator decides) /// - If no mediator: runs parallel only (bidders → highest CPM wins) + /// + /// # Errors + /// + /// Returns an error if the auction execution fails due to provider errors or + /// mediation errors. pub async fn run_auction( &self, request: &AuctionRequest, @@ -89,8 +96,7 @@ impl AuctionOrchestrator { let provider_responses = self.run_providers_parallel(request, context).await?; let floor_prices = self.floor_prices_by_slot(request); - let (mediator_response, winning_bids) = if self.config.has_mediator() { - let mediator_name = self.config.mediator.as_ref().unwrap(); + let (mediator_response, winning_bids) = if let Some(mediator_name) = &self.config.mediator { let mediator = self.get_provider(mediator_name)?; log::info!( @@ -450,6 +456,7 @@ impl AuctionOrchestrator { } /// Check if orchestrator is enabled. + #[must_use] pub fn is_enabled(&self) -> bool { self.config.enabled } @@ -472,11 +479,13 @@ pub struct OrchestrationResult { impl OrchestrationResult { /// Get the winning bid for a specific slot. + #[must_use] pub fn get_winning_bid(&self, slot_id: &str) -> Option<&Bid> { self.winning_bids.get(slot_id) } /// Get all bids from all providers for a specific slot. + #[must_use] pub fn get_all_bids_for_slot(&self, slot_id: &str) -> Vec<&Bid> { self.provider_responses .iter() @@ -486,6 +495,7 @@ impl OrchestrationResult { } /// Get the total number of bids received. + #[must_use] pub fn total_bids(&self) -> usize { self.provider_responses.iter().map(|r| r.bids.len()).sum() } diff --git a/crates/common/src/auction/provider.rs b/crates/common/src/auction/provider.rs index 3c7b7900..827b33fb 100644 --- a/crates/common/src/auction/provider.rs +++ b/crates/common/src/auction/provider.rs @@ -15,20 +15,29 @@ pub trait AuctionProvider: Send + Sync { /// Submit a bid request to this provider and return a pending request. /// /// Implementations should: - /// - Transform AuctionRequest to provider-specific format - /// - Make HTTP call to provider endpoint using send_async() - /// - Return PendingRequest for orchestrator to await + /// - Transform `AuctionRequest` to provider-specific format + /// - Make HTTP call to provider endpoint using `send_async()` + /// - Return `PendingRequest` for orchestrator to await /// /// The orchestrator will handle waiting for responses and parsing them. + /// + /// # Errors + /// + /// Returns an error if the request cannot be created or if the provider endpoint + /// cannot be reached (though usually network errors happen during `PendingRequest` await). fn request_bids( &self, request: &AuctionRequest, context: &AuctionContext<'_>, ) -> Result>; - /// Parse the response from the provider into an AuctionResponse. + /// Parse the response from the provider into an `AuctionResponse`. + /// + /// Called by the orchestrator after the `PendingRequest` completes. + /// + /// # Errors /// - /// Called by the orchestrator after the PendingRequest completes. + /// Returns an error if the response cannot be parsed into a valid `AuctionResponse`. fn parse_response( &self, response: fastly::Response, diff --git a/crates/common/src/auction/types.rs b/crates/common/src/auction/types.rs index 8593e061..6c6c4d63 100644 --- a/crates/common/src/auction/types.rs +++ b/crates/common/src/auction/types.rs @@ -146,7 +146,7 @@ pub struct Bid { pub metadata: HashMap, } -/// OpenRTB response metadata for the orchestrator. +/// `OpenRTB` response metadata for the orchestrator. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct OrchestratorExt { pub strategy: String, diff --git a/crates/common/src/auction_config_types.rs b/crates/common/src/auction_config_types.rs index e1cabd7a..916e839f 100644 --- a/crates/common/src/auction_config_types.rs +++ b/crates/common/src/auction_config_types.rs @@ -39,11 +39,13 @@ fn default_creative_store() -> String { #[allow(dead_code)] // Methods used in runtime but not in build script impl AuctionConfig { /// Get all provider names. + #[must_use] pub fn provider_names(&self) -> &[String] { &self.providers } /// Check if this config has a mediator configured. + #[must_use] pub fn has_mediator(&self) -> bool { self.mediator.is_some() } diff --git a/crates/common/src/backend.rs b/crates/common/src/backend.rs index ba5a1c84..798131f4 100644 --- a/crates/common/src/backend.rs +++ b/crates/common/src/backend.rs @@ -11,6 +11,10 @@ use crate::error::TrustedServerError; /// The backend name is derived from the scheme and `host[:port]` to avoid collisions across /// http/https or different ports. If a backend with the derived name already exists, /// this function logs and reuses it. +/// +/// # Errors +/// +/// Returns an error if the host is empty or if backend creation fails (except for `NameInUse` which reuses the existing backend). pub fn ensure_origin_backend( scheme: &str, host: &str, @@ -75,6 +79,13 @@ pub fn ensure_origin_backend( } } +/// Ensures a dynamic backend exists for the given origin URL. +/// +/// Parses the URL and delegates to `ensure_origin_backend` to create or reuse a backend. +/// +/// # Errors +/// +/// Returns an error if the URL cannot be parsed or lacks a host, or if backend creation fails. pub fn ensure_backend_from_url(origin_url: &str) -> Result> { let parsed_url = Url::parse(origin_url).change_context(TrustedServerError::Proxy { message: format!("Invalid origin_url: {}", origin_url), diff --git a/crates/common/src/cookies.rs b/crates/common/src/cookies.rs index de78b109..48f397bb 100644 --- a/crates/common/src/cookies.rs +++ b/crates/common/src/cookies.rs @@ -62,6 +62,7 @@ pub fn handle_request_cookies( /// /// Generates a properly formatted cookie with security attributes /// for storing the synthetic ID. +#[must_use] pub fn create_synthetic_cookie(settings: &Settings, synthetic_id: &str) -> String { format!( "synthetic_id={}; Domain={}; Path=/; Secure; SameSite=Lax; Max-Age={}", diff --git a/crates/common/src/creative.rs b/crates/common/src/creative.rs index 79e1e9dc..0265ad79 100644 --- a/crates/common/src/creative.rs +++ b/crates/common/src/creative.rs @@ -297,6 +297,7 @@ pub(super) fn proxied_attr_value(settings: &Settings, attr_val: Option) /// Rewrite a full CSS stylesheet body by normalizing url(...) references to the /// unified first-party proxy. Relative URLs are left unchanged. +#[must_use] pub fn rewrite_css_body(settings: &Settings, css: &str) -> String { rewrite_style_urls(settings, css) } @@ -307,6 +308,7 @@ pub fn rewrite_css_body(settings: &Settings, css: &str) -> String { /// - `