🎯 Code Coverage (details)
• Patch Coverage: 91.40%
• Overall Coverage: 84.83% (+0.09%)

I think the fact we need to name the tags things like DATADOG_SECRET_VALIDATION_ERROR_TYPE.0 and DATADOG_SECRET_VALIDATION_ERROR_TYPE.1 to flatten/encode array-like data hints that we're modeling this incorrectly.

The SARIF spec has Property Bags (which tags are a special case within). Why don't we add a property for this validation info?

{
  "properties": {
    "tags": [
      "DATADOG_CATEGORY:SECURITY",
      "DATADOG_SECRET_VALIDATION_STATUS:VALIDATION_ERROR"
    ],
    "datadogSecretValidationErrors": [
      { "type": "HttpError", "statusCode": 400, "message": "Invalid token" },
      { "type": "UnknownResponseType", "statusCode": 0, "message": "Connection timeout" }
    ]
  }
}

I think the fact we need to name the tags things like DATADOG_SECRET_VALIDATION_ERROR_TYPE.0 and DATADOG_SECRET_VALIDATION_ERROR_TYPE.1 to flatten/encode array-like data hints that we're modeling this incorrectly.

The SARIF spec has Property Bags (which tags are a special case within). Why don't we add a property for this validation info?

{
  "properties": {
    "tags": [
      "DATADOG_CATEGORY:SECURITY",
      "DATADOG_SECRET_VALIDATION_STATUS:VALIDATION_ERROR"
    ],
    "datadogSecretValidationErrors": [
      { "type": "HttpError", "statusCode": 400, "message": "Invalid token" },
      { "type": "UnknownResponseType", "statusCode": 0, "message": "Connection timeout" }
    ]
  }
}

I like the idea of using custom property datadogSecretValidationErrors.
Updated the PR, @jasonforal can you please take another look?

I wouldn't have get_validation_errors_data return a serde_json::Value, because then you have to do all the unit tests to roundtrip JSON to check schema conformance. You can instead lean on serde and the structs you already have:

So you'd have function signature

fn get_validation_errors_data(&self) -> Option<&[ValidationErrorInfo]>

with a simple implementation like

fn get_validation_errors_data(&self) -> Option<&[ValidationErrorInfo]> {
    let Secret(_, SecretValidationStatus::ValidationError(error_infos)) = &self else {
        return None;
    };
    (!error_infos.is_empty()).then_some(error_infos.as_slice())
}

You'd then change the serde hooks on ValidationErrorInfo to represent your desired naming, e.g.

#[derive(Serialize, Deserialize)
#[serde(rename_all="camelCase")]
pub struct ValidationErrorInfo {
    #[serde(rename="type")]
    pub error_type: ValidationErrorType,
    pub status_code: u16,
    pub message: String,
}

And then in the call site, it becomes:

let json_array = serde_json::to_value(errors_data)?;
properties.additional_properties.insert(
    "datadogSecretValidationErrors".to_string(),
    json_array,
);

And so you don't need to test serialization/deserialization because you can assume serde handles that correctly.

However, when it comes to testing, we're currently mixing two things:
A. Correctness (the data values are what they should be)
B. Serialization (the data looks like what it should)

By unit testing using assert_json_include, you're trying to infer A by testing B.

Instead, you should only test A (and let serde's guarantees implicitly prove B without needing a specific test). So your unit test shouldn't care about JSON at all. In pseudo-code, you'll do something like

let jsonValue = sarif_report.runs[0].results.unwrap()[0].properties.unwrap().additional_properties.get("datadogSecretValidationErrors").unwrap();
let parsed: Vec<ValidationErrorInfo> = serde_json::from_value(jsonValue).unwrap();
// ... assert on Rust struct equivalence
assert_eq!(parsed, vec![ValidationErrorInfo { ... }, ...])

And then if you need to prove schema conformance, you'd add a very simple test for each struct:

// pseudo code
#[test]
fn serialize_validation_error_info() {
    let v = ValidationErrorInfo {
       error_type: ...
       status_code: ...
       message: ...
    }
    let expected = "<a hardcoded json string>"
    // and then compare this -- this is effectively testing your serde(rename) directives and serialization/deserialization
    assert_eq!(serde_json::to_value(v)..., expected) //
}

The important thing to note here is that all that unit test does is test serialization/deserialization -- not any logic like where the struct appears. So you would have separate tests for A and B (per the message above)

• Updated get_validation_errors_data to remove serialization logic in commit: cfb8f59
• Created another test case for ValidationErrorInfo serialization 5d39024