Skip to content

Commit d9b94ee

Browse files
piotr-roslanieclonerapier
piotr-roslaniec
and
lonerapier
authored
feat!: Return actionable errors from Manifest validation (#552)
* feat: return aggregated errors from manifest validation * chore: improve error tracing --------- Co-authored-by: Sambhav Dusad <lonerapier@proton.me>
1 parent 7dd1477 commit d9b94ee

File tree

11 files changed

+1111
-440
lines changed

11 files changed

+1111
-440
lines changed

‎Cargo.lock‎

Lines changed: 384 additions & 302 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎core/src/error.rs‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@ pub enum WebProverCoreError {
99
#[error("Invalid manifest: {0}")]
1010
InvalidManifest(String),
1111

12+
/// The error is a manifest HTTP error
13+
#[error("Manifest HTTP error: {0}")]
14+
ManifestHttpError(#[from] ManifestHttpError),
15+
1216
/// Serde operation failed
1317
#[error("Serde error occurred: {0}")]
1418
SerdeError(#[from] serde_json::Error),
@@ -20,6 +24,41 @@ pub enum WebProverCoreError {
2024
/// Template-specific errors
2125
#[error("Template error: {0}")]
2226
Template(#[from] TemplateError),
27+
28+
/// Indicates that extraction failed
29+
#[error("Extraction failed: {0}")]
30+
ExtractionFailed(String),
31+
}
32+
33+
#[derive(Debug, Error)]
34+
pub enum ManifestHttpError {
35+
/// HTTP status mismatch between expected and actual status
36+
#[error("HTTP status mismatch: expected {expected}, actual {actual}")]
37+
StatusMismatch { expected: String, actual: String },
38+
39+
/// HTTP version mismatch between expected and actual version
40+
#[error("HTTP version mismatch: expected {expected}, actual {actual}")]
41+
VersionMismatch { expected: String, actual: String },
42+
43+
/// HTTP message mismatch between expected and actual message
44+
#[error("HTTP message mismatch: expected {expected}, actual {actual}")]
45+
MessageMismatch { expected: String, actual: String },
46+
47+
/// HTTP header value mismatch between expected and actual value
48+
#[error("HTTP header mismatch: expected {expected}, actual {actual}")]
49+
HeaderMismatch { expected: String, actual: String },
50+
51+
/// Expected HTTP header is missing
52+
#[error("HTTP header missing: expected {expected}, actual {actual}")]
53+
HeaderMissing { expected: String, actual: String },
54+
55+
/// HTTP method mismatch between expected and actual method
56+
#[error("HTTP method mismatch: expected {expected}, actual {actual}")]
57+
MethodMismatch { expected: String, actual: String },
58+
59+
/// HTTP URL mismatch between expected and actual URL
60+
#[error("HTTP URL mismatch: expected {expected}, actual {actual}")]
61+
UrlMismatch { expected: String, actual: String },
2362
}
2463

2564
/// Represents specific error conditions related to template handling
@@ -44,4 +83,12 @@ pub enum TemplateError {
4483
/// Empty regex pattern
4584
#[error("Empty regex pattern for `{0}`")]
4685
EmptyRegexPattern(String),
86+
87+
/// Variable is missing
88+
#[error("Variable missing for key: {key}")]
89+
VariableMissing { key: String },
90+
91+
/// Variable doesn't match
92+
#[error("Variable mismatch for key: {key}")]
93+
VariableMismatch { key: String },
4794
}

‎core/src/http.rs‎

Lines changed: 152 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,9 @@ use serde::{Deserialize, Serialize};
1919
use tracing::debug;
2020

2121
use crate::{
22-
error::WebProverCoreError,
23-
parser::{DataFormat, ExtractionValues, ExtractorConfig},
22+
error::{ManifestHttpError, TemplateError, WebProverCoreError},
23+
manifest::ManifestValidationResult,
24+
parser::{DataFormat, ExtractorConfig},
2425
template,
2526
template::TemplateVar,
2627
};
@@ -165,27 +166,38 @@ impl NotaryResponse {
165166
pub fn match_and_extract(
166167
&self,
167168
other: &ManifestResponse,
168-
) -> Result<Option<ExtractionValues>, WebProverCoreError> {
169+
) -> Result<ManifestValidationResult, WebProverCoreError> {
170+
let mut result = ManifestValidationResult::default();
171+
169172
// Check basic response properties
170-
if self.response.status != other.status
171-
|| self.response.version != other.version
172-
|| self.response.message != other.message
173-
{
174-
debug!("Exact matches for status, version, or message do not match");
175-
return Ok(None);
173+
if self.response.status != other.status {
174+
result.report_error(
175+
ManifestHttpError::StatusMismatch {
176+
expected: other.status.clone(),
177+
actual: self.response.status.clone(),
178+
}
179+
.into(),
180+
);
176181
}
177-
178-
// Check headers
179-
if !self.headers_match(other) {
180-
return Ok(None);
182+
if self.response.version != other.version {
183+
result.report_error(
184+
ManifestHttpError::VersionMismatch {
185+
expected: other.version.clone(),
186+
actual: self.response.version.clone(),
187+
}
188+
.into(),
189+
);
190+
}
191+
if self.response.message != other.message {
192+
result.report_error(
193+
ManifestHttpError::MessageMismatch {
194+
expected: other.message.clone(),
195+
actual: self.response.message.clone(),
196+
}
197+
.into(),
198+
);
181199
}
182200

183-
// Check body
184-
self.body_matches(other)
185-
}
186-
187-
/// Helper method to check if headers match
188-
fn headers_match(&self, other: &ManifestResponse) -> bool {
189201
for (key, other_value) in &other.headers {
190202
match self
191203
.response
@@ -195,51 +207,75 @@ impl NotaryResponse {
195207
{
196208
Some(actual_value) if actual_value.to_lowercase() == other_value.to_lowercase() => continue,
197209
Some(actual_value) => {
198-
debug!(
199-
"Header mismatch for key: {}: expected={}, actual={}",
200-
key, other_value, actual_value
210+
result.report_error(
211+
ManifestHttpError::HeaderMismatch {
212+
expected: other_value.clone(),
213+
actual: actual_value.clone(),
214+
}
215+
.into(),
201216
);
202-
return false;
203217
},
204218
None => {
205-
debug!("Header key not present in self: {}", key);
206-
return false;
219+
result.report_error(
220+
ManifestHttpError::HeaderMissing {
221+
expected: other_value.clone(),
222+
actual: key.clone(),
223+
}
224+
.into(),
225+
);
207226
},
208227
}
209228
}
210-
true
229+
230+
// Check body
231+
let body_result = self.body_matches(other)?;
232+
result.merge(&body_result);
233+
234+
// Return combined result
235+
Ok(result)
211236
}
212237

213238
/// Helper method to check if body matches and extract values
214239
fn body_matches(
215240
&self,
216241
other: &ManifestResponse,
217-
) -> Result<Option<ExtractionValues>, WebProverCoreError> {
242+
) -> Result<ManifestValidationResult, WebProverCoreError> {
243+
let mut result = ManifestValidationResult::default();
244+
218245
match &self.notary_response_body.body {
219246
Some(body) => {
220-
let result = other.body.0.extract_and_validate(body)?;
247+
let extraction_result = other.body.0.extract_and_validate(body)?;
248+
result.merge_extraction_result(&extraction_result);
221249

222-
if !result.errors.is_empty() {
223-
debug!("Response body does not match: {:?}", result.errors);
224-
return Ok(None);
250+
if !extraction_result.is_success() {
251+
result.report_error(WebProverCoreError::ExtractionFailed(
252+
"Extraction returned errors".to_string(),
253+
));
225254
}
226255

227-
if result.values.len() != other.body.0.extractors.len() {
228-
debug!("Not all extractors were matched");
229-
return Ok(None);
256+
if extraction_result.values.len() != other.body.0.extractors.len() {
257+
result.report_error(WebProverCoreError::ExtractionFailed(
258+
"Not all values were extracted".to_string(),
259+
));
230260
}
231261

232262
debug!("Client response matches");
233-
Ok(Some(result.values))
263+
Ok(result)
234264
},
235265
None if other.body.0.extractors.is_empty() => {
236266
// If we get here, there was a match but no data
237267
debug!("Client response matches (no data in response body)");
238-
Ok(Some(HashMap::new()))
268+
result.report_error(WebProverCoreError::ExtractionFailed(
269+
"Client response returned no data".to_string(),
270+
));
271+
Ok(result)
239272
},
240273
None => {
241274
debug!("No data to match against");
242-
Ok(None)
275+
result.report_error(WebProverCoreError::ExtractionFailed(
276+
"No data to match against".to_string(),
277+
));
278+
Ok(result)
243279
},
244280
}
245281
}
@@ -517,34 +553,71 @@ impl ManifestRequest {
517553
/// - All headers in `self` must exist in `other` with matching values.
518554
/// - All vars in `self` must exist in `other` with matching constraints.
519555
/// - All remaining fields like `method`, `url`, and `body` must also match.
520-
pub fn is_subset_of(&self, other: &ManifestRequest) -> bool {
556+
pub fn is_subset_of(
557+
&self,
558+
other: &ManifestRequest,
559+
) -> Result<ManifestValidationResult, WebProverCoreError> {
560+
let mut result = ManifestValidationResult::default();
561+
521562
// Check if all headers in `self` exist in `other` with the same value
522563
for (key, value) in &self.headers {
523564
let expected_header =
524565
other.headers.get(key).or_else(|| other.headers.get(&key.to_lowercase()));
525566
if expected_header != Some(value) {
526-
return false;
567+
result.report_error(
568+
ManifestHttpError::HeaderMismatch {
569+
expected: value.clone(),
570+
actual: expected_header.cloned().unwrap_or_else(|| "missing".to_string()),
571+
}
572+
.into(),
573+
);
527574
}
528575
}
529576

530-
// TODO: Not sure how to handle `vars` now, so disabling this
531-
// Check if all vars in `self` exist in `other` with the same or compatible constraints
532-
// for (key, var) in &self.vars {
533-
// match other.vars.get(key) {
534-
// Some(other_var) =>
535-
// if var != other_var {
536-
// return false;
537-
// },
538-
// None => {
539-
// return false;
540-
// },
541-
// }
542-
// }
543-
544-
// TODO: Notice that we match body exactly below
545-
// TODO: What to do with the body? Ominous
546-
// self.method == other.method && self.url == other.url && self.body == other.body
547-
self.method == other.method && self.url == other.url
577+
for (key, var) in &self.vars {
578+
match other.vars.get(key) {
579+
Some(other_var) =>
580+
if var != other_var {
581+
result.report_error(WebProverCoreError::Template(TemplateError::VariableMismatch {
582+
key: key.clone(),
583+
}))
584+
},
585+
None => result.report_error(WebProverCoreError::Template(TemplateError::VariableMissing {
586+
key: key.clone(),
587+
})),
588+
}
589+
}
590+
591+
if self.method != other.method {
592+
result.report_error(
593+
ManifestHttpError::MethodMismatch {
594+
expected: other.method.clone(),
595+
actual: self.method.clone(),
596+
}
597+
.into(),
598+
);
599+
}
600+
601+
if self.url != other.url {
602+
result.report_error(
603+
ManifestHttpError::UrlMismatch { expected: other.url.clone(), actual: self.url.clone() }
604+
.into(),
605+
);
606+
}
607+
608+
if self.version != other.version {
609+
result.report_error(
610+
ManifestHttpError::VersionMismatch {
611+
expected: other.version.clone(),
612+
actual: self.version.clone(),
613+
}
614+
.into(),
615+
);
616+
}
617+
618+
// We don't compare the body here because this is what extractors are for
619+
620+
Ok(result)
548621
}
549622
}
550623

@@ -562,14 +635,13 @@ pub mod tests {
562635
macro_rules! request {
563636
// Match with optional parameters
564637
($($key:ident: $value:expr),* $(,)?) => {{
565-
// Make clippy happy
566-
#[allow(unused_mut) ]
638+
#[allow(unused_mut)]
567639
let mut request = ManifestRequest {
568-
method: "GET".to_string(),
640+
method: "GET".to_string(),
569641
url: "https://example.com".to_string(),
570642
version: "HTTP/1.1".to_string(),
571643
headers: std::collections::HashMap::from([
572-
("Authorization".to_string(), "Bearer <TOKEN>".to_string()),
644+
("Authorization".to_string(), "Bearer <%TOKEN%>".to_string()),
573645
("User-Agent".to_string(), "test-agent".to_string()),
574646
]),
575647
body: None,
@@ -606,7 +678,20 @@ pub mod tests {
606678
headers: std::collections::HashMap::from([
607679
("Content-Type".to_string(), "application/json".to_string())
608680
]),
609-
body: ManifestResponseBody::default(),
681+
body: ManifestResponseBody {
682+
0: ExtractorConfig {
683+
format: DataFormat::Json,
684+
extractors: vec![crate::parser::Extractor {
685+
id: "dummy".to_string(),
686+
description: "Dummy extractor".to_string(),
687+
selector: vec![],
688+
extractor_type: crate::parser::ExtractorType::String,
689+
required: true,
690+
predicates: vec![],
691+
attribute: None,
692+
}],
693+
},
694+
},
610695
};
611696

612697
// Override default fields with provided arguments
@@ -680,12 +765,12 @@ pub mod tests {
680765

681766
// Is a perfect match with itself
682767
let matching_result = notary_response.match_and_extract(&response).unwrap();
683-
assert!(matching_result.is_some());
684-
assert_eq!(matching_result.unwrap().get("testKey").unwrap(), &json!(3));
768+
assert!(matching_result.is_success());
769+
assert_eq!(matching_result.values().get("testKey").unwrap(), &json!(3));
685770

686771
// Fails if it doesn't match directly
687772
let non_matching_response = response!(status: "201".to_string());
688-
assert!(notary_response.match_and_extract(&non_matching_response).unwrap().is_none());
773+
assert!(!notary_response.match_and_extract(&non_matching_response).unwrap().is_success());
689774

690775
// Superset case
691776
let response_superset = {
@@ -699,7 +784,7 @@ pub mod tests {
699784
));
700785
res
701786
};
702-
assert!(response_superset.match_and_extract(&response).unwrap().is_some());
787+
assert!(response_superset.match_and_extract(&response).unwrap().is_success());
703788
}
704789

705790
#[test]
@@ -802,7 +887,7 @@ pub mod tests {
802887
)],
803888
})
804889
);
805-
assert!(!base_response.match_and_extract(&other_response).unwrap().is_some());
890+
assert!(!base_response.match_and_extract(&other_response).unwrap().is_success());
806891
}
807892

808893
#[test]

0 commit comments

Comments
 (0)