Domain configuration selection: add specificity-based matching logic and comprehensive tests #64
Conversation
- Add calculate_domain_specificity() to score domain patterns - Sort configurations by specificity before matching - Exact matches get highest priority (10000) - Wildcard patterns scored by number of non-wildcard segments - More specific wildcards (e.g., *.truc.elyrion.fr) now match before less specific ones (e.g., *.elyrion.fr) Co-authored-by: Flash303 <94756886+Flash303@users.noreply.github.com>
Replace elyrion.fr with example.com in test cases as requested Co-authored-by: Flash303 <94756886+Flash303@users.noreply.github.com>
…essing-order Sort domain patterns by specificity for deterministic wildcard matching
| for pattern in &config.domains { | ||
| if WildMatch::new(pattern).matches(&domain) { | ||
| let specificity = Self::calculate_domain_specificity(pattern, &domain); | ||
| matches.push((Arc::clone(config), pattern.as_str(), specificity)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| if matches.is_empty() { | ||
| debug!( | ||
| log_type = LogType::ConfigProvider.as_str(), | ||
| found = false, | ||
| "Domain lookup result" | ||
| ); | ||
| return None; | ||
| } | ||
|
|
||
| // Sort by specificity (higher = more specific) | ||
| // In case of tie, maintain stable order | ||
| matches.sort_by(|a, b| b.2.cmp(&a.2)); | ||
|
|
||
| debug!( | ||
| log_type = LogType::ConfigProvider.as_str(), | ||
| found = false, | ||
| found = true, | ||
| "Domain lookup result" | ||
| ); | ||
| None | ||
|
|
||
| Some(Arc::clone(&matches[0].0)) |
There was a problem hiding this comment.
It might be wise to instead sort the patterns by specificity, then match the domain and return the first one. That way, we can reduce the effective number of matches.
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces specificity-based domain matching to the ConfigurationService, ensuring that when multiple domain patterns match a request, the most specific one is selected. The implementation collects all matching configurations, scores them by specificity (exact matches score highest, followed by wildcards with more non-wildcard segments), and returns the most specific match.
Key Changes
- Modified
find_server_by_domainto collect all matching configurations and select by specificity rather than returning the first match - Added
calculate_pattern_specificityhelper method to score domain patterns based on wildcard usage and segment count - Added three comprehensive async test cases validating exact matches, wildcard precedence, and deeply nested domain patterns
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // | ||
| // The `matches` vector can then be used to sort or pick the most specific configuration | ||
| // (for example, by sorting on `specificity` in descending order). | ||
| let mut matches: Vec<(Arc<ServerConfig>, &str, i32)> = Vec::new(); |
There was a problem hiding this comment.
The &str pattern reference in the tuple (Arc<ServerConfig>, &str, i32) is stored but never used. Consider simplifying the tuple to (Arc<ServerConfig>, i32) to avoid unnecessary memory usage and complexity. The pattern string reference serves no purpose after calculating specificity.
| for config in configs_snapshot.values() { | ||
| if config | ||
| .domains | ||
| .iter() | ||
| .any(|pattern| WildMatch::new(pattern).matches(&domain)) | ||
| { | ||
| debug!( | ||
| log_type = LogType::ConfigProvider.as_str(), | ||
| found = true, | ||
| "Domain lookup result" | ||
| ); | ||
| return Some(Arc::clone(config)); | ||
| for pattern in &config.domains { | ||
| if WildMatch::new(pattern).matches(&domain) { | ||
| let specificity = Self::calculate_pattern_specificity(pattern); | ||
| matches.push((Arc::clone(config), pattern.as_str(), specificity)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
When a single ServerConfig has multiple domain patterns that match the queried domain, the same config will be added to the matches vector multiple times with different specificity scores. This is inefficient. Consider tracking only the highest specificity match per config, for example by using a HashMap<String, (Arc<ServerConfig>, i32)> keyed by config_id, and only keeping the entry with the highest specificity score for each config.
There was a problem hiding this comment.
This is not the expected behavior. It will dismiss all the domains, excluding the most specific one, so it will never work for configurations with multiple domains.
Improves the server configuration selection process by ensuring that the most specific domain match is always preferred. This prevents less specific wildcard configurations from incorrectly overriding more precise matches. Updates tests to reflect the fix.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| for config in configs_snapshot.values() { | ||
| if config | ||
| .domains | ||
| .iter() | ||
| .any(|pattern| WildMatch::new(pattern).matches(&domain)) | ||
| { | ||
| debug!( | ||
| log_type = LogType::ConfigProvider.as_str(), | ||
| found = true, | ||
| "Domain lookup result" | ||
| ); | ||
| return Some(Arc::clone(config)); | ||
| for pattern in &config.domains { | ||
| if WildMatch::new(pattern).matches(&domain) { | ||
| let specificity = Self::calculate_pattern_specificity(pattern); | ||
| matches.push((Arc::clone(config), pattern.as_str(), specificity)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is not the expected behavior. It will dismiss all the domains, excluding the most specific one, so it will never work for configurations with multiple domains.
|
|
||
| // Track the best (highest specificity) match per config. | ||
| // Uses a HashMap keyed by config_id to ensure each config appears at most once, | ||
| // storing only the highest specificity score when multiple patterns match. | ||
| let mut best_matches: HashMap<String, (Arc<ServerConfig>, i32)> = HashMap::new(); | ||
|
|
||
| for config in configs_snapshot.values() { | ||
| if config | ||
| .domains | ||
| .iter() | ||
| .any(|pattern| WildMatch::new(pattern).matches(&domain)) | ||
| { | ||
| debug!( | ||
| log_type = LogType::ConfigProvider.as_str(), | ||
| found = true, | ||
| "Domain lookup result" | ||
| ); | ||
| return Some(Arc::clone(config)); | ||
| for pattern in &config.domains { | ||
| if WildMatch::new(pattern).matches(&domain) { | ||
| let specificity = Self::calculate_pattern_specificity(pattern); | ||
|
|
||
| // Only keep this match if it's the first or has higher specificity | ||
| best_matches | ||
| .entry(config.config_id.clone()) | ||
| .and_modify(|(_, existing_score)| { | ||
| if specificity > *existing_score { | ||
| *existing_score = specificity; | ||
| } | ||
| }) | ||
| .or_insert_with(|| (Arc::clone(config), specificity)); | ||
| } |
There was a problem hiding this comment.
I think this version is worse than the previous one; it's harder to understand and not that pretty. You should just go back to the previous one, and I think that way we can call it done.
2 participants
This pull request improves the domain matching logic in the
ConfigurationServiceby introducing specificity-based selection for server configurations and adds comprehensive tests to verify the new behavior. Now, when multiple configurations match a domain (including wildcards), the most specific one is reliably chosen.Domain Matching Logic Improvements
find_server_by_domainmethod to collect all matching configurations and select the most specific one using a calculated specificity score, ensuring exact matches and more specific wildcards take precedence.calculate_domain_specificityhelper method to score domain patterns, giving highest priority to exact matches and ranking wildcards by number of non-wildcard segments and overall pattern depth.Testing Enhancements