Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for spdx "relationshipType": "PACKAGE_OF" #1186

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+90 −12
Open

Add tests for spdx "relationshipType": "PACKAGE_OF" #1186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63f860f
Add tests for spdx "relationshipType": "PACKAGE_OF"
chirino Jan 22, 2025
618f27b
Improve tests based on PR feedback.
chirino Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 37 additions & 12 deletions modules/analysis/src/endpoints/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::test::caller;
use crate::test::{caller, has_json_fields};
use actix_http::Request;
use actix_web::test::TestRequest;
use itertools::Itertools;
Expand Down Expand Up @@ -654,24 +654,49 @@ async fn spdx_package_of(ctx: &TrustifyContext) -> Result<(), anyhow::Error> {
let uri = format!("/api/v2/analysis/dep/{}", urlencoding::encode(purl));
let request: Request = TestRequest::get().uri(&uri).to_request();
let response: Value = app.call_and_read_body_json(request).await;
log::debug!("{response:#?}");
log::debug!("{}", serde_json::to_string_pretty(&response)?);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wish serde's impl of :#? used to_string_pretty. I can remember the former, but never the latter. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100%

let sbom = &response["items"][0];
let matches: Vec<_> = sbom["deps"]
.as_array()
.into_iter()
.flatten()
.filter(|m| {
m == &&json!({
"sbom_id": sbom["sbom_id"],
"node_id": "SPDXRef-83c9faa0-ca85-4e48-9165-707b2f9a324b",
"relationship": "PackageOf",
"purl": [],
"cpe": m["cpe"], // long list assume it's correct
"name": "SATELLITE-6.15-RHEL-8",
"version": "6.15",
"deps": [],
})
has_json_fields(
m,
&json!({
"relationship": "PackageOf",
"name": "SATELLITE-6.15-RHEL-8",
"version": "6.15",
}),
)
})
.collect();

assert_eq!(1, matches.len());

let uri = format!(
"/api/v2/analysis/root-component?q={}",
urlencoding::encode("SATELLITE-6.15-RHEL-8")
);
let request: Request = TestRequest::get().uri(&uri).to_request();
let response: Value = app.call_and_read_body_json(request).await;
log::debug!("{}", serde_json::to_string_pretty(&response)?);

let sbom = &response["items"][0];
let matches: Vec<_> = sbom["ancestors"]
.as_array()
.into_iter()
.flatten()
.filter(|m| {
has_json_fields(
m,
&json!({
"relationship": "PackageOf",
"name": "rubygem-google-cloud-compute",
"version": "0.5.0-1.el8sat"
}),
)
})
.collect();

Expand Down
53 changes: 53 additions & 0 deletions modules/analysis/src/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,64 @@ use crate::{
model::{AncNode, AncestorSummary},
};
use itertools::Itertools;
use serde_json::{json, Value};
use trustify_test_context::{
call::{self, CallService},
TrustifyContext,
};

// This function checks if the actual JSON object has all the fields of the expected JSON object.
pub fn has_json_fields(actual: &Value, expected: &Value) -> bool {
match (actual.as_object(), expected.as_object()) {
(Some(actual), Some(expected)) => {
for (key, value_a) in expected {
if Some(value_a) != actual.get(key.as_str()) {
return false;
}
}
true
}
_ => false,
}
}
Comment on lines +12 to +25
Copy link
Contributor

@jcrossley3 jcrossley3 Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very cool idea! I want it to do more, though, so forgive me for challenging you. 😄

The function expects Value's, but fails if they're not a specific type of Value. Make it work for any kind of Value. If it's an object, then use recursion to compare the key/value pairs. Probably want to rename the function to is_same or subset or contains or some such.

You'll essentially have two branches:

pub fn is_subset(actual: &Value, expected: &Value) -> bool {
    if expected.is_object() {
        expected.iter().all(|(k, v)| is_subset(&actual[k], v))
    } else {
        expected == actual
    }
}

And no, that won't actually compile due to Value's overly-complicated API, but you can make it work!

And this will be useful all over, so let's stick it somewhere in test-context maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better...

pub trait Contains {
    fn contains(&self, subset: Value) -> bool;
}

impl Contains for Value {
    fn contains(&self, subset: Value) -> bool {
        match (self.as_object(), subset.as_object()) {
            (Some(src), Some(tgt)) => tgt
                .iter()
                .all(|(k, v)| src.get(k).is_some_and(|x| x.contains(v.clone()))),
            _ => subset == *self,
        }
    }
}

Accepting a reference to self and taking ownership of subset allows you to clean up your calling code a bit, e.g.

        .filter(|m| {
            m.contains(json!({
              "relationship": "PackageOf",
              "name": "rubygem-google-cloud-compute",
              "version": "0.5.0-1.el8sat"
            }))
        })

Deciding to take a reference instead of ownership would be determined by whether we think most of our test subsets will be "one-off's". If we often re-use the same one, we might take a reference to avoid having to .clone() the subset each time we pass it. But I tend to think it's more likely we'll call json! every time we call .contains so taking ownership seems reasonable.

Make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't help myself...

If you add a branch for Value::Array types...

impl Contains for Value {
    fn contains(&self, subset: Value) -> bool {
        match (self, &subset) {
            (Value::Object(src), Value::Object(tgt)) => tgt
                .iter()
                .all(|(k, v)| src.get(k).is_some_and(|x| x.contains(v.clone()))),
            (Value::Array(src), Value::Array(tgt)) => tgt
                .iter()
                .all(|v| src.iter().any(|x| x.contains(v.clone()))),
            _ => subset == *self,
        }
    }
}

You can assert things like this:

    assert!(response.contains(json!({
        "items": [
            {
                "deps": [
                    {
                        "relationship": "PackageOf",
                        "name": "SATELLITE-6.15-RHEL-8",
                        "version": "6.15",
                    }
                ]
            }
        ]
    })));

There's a tradeoff, though. Sometimes you might want arrays to match on the exact contents, e.g. you might want to assert that "purls": ["pkg:blah"] has exactly one element in it, and removing that Value::Array arm would do that.


#[cfg(test)]
#[test]
fn test_has_json_fields() {
// actual can have additional fields
assert!(has_json_fields(
&json!({
"relationship": "PackageOf",
"other": "test",
}),
&json!({
"relationship": "PackageOf",
}),
));

// case where an expected field does not match
assert!(!has_json_fields(
&json!({
"relationship": "PackageOf",
"other": "test",
}),
&json!({
"relationship": "bad",
}),
));

// case where an expected field is missing
assert!(!has_json_fields(
&json!({
"relationship": "PackageOf",
"other": "test",
}),
&json!({
"name": "SATELLITE-6.15-RHEL-8",
}),
))
}

pub async fn caller(ctx: &TrustifyContext) -> anyhow::Result<impl CallService + '_> {
call::caller(|svc| configure(svc, ctx.db.clone())).await
}
Expand Down
Loading