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

chirino · 2025-01-22T13:12:41Z

Verify the relationship type shows up in
/api/v2/analysis/root-component API calls.

Part of issue #1140

Verify the relationship type shows up in `/api/v2/analysis/root-component` API calls. Part of issue trustification#1140 Signed-off-by: Hiram Chirino <[email protected]>

jcrossley3 · 2025-01-23T17:47:21Z

modules/analysis/src/endpoints/test.rs

-    log::debug!("{response:#?}");
+    log::debug!("{}", serde_json::to_string_pretty(&response)?);


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

jcrossley3 · 2025-01-23T17:49:16Z

modules/analysis/src/endpoints/test.rs

+    );
+    let request: Request = TestRequest::get().uri(&uri).to_request();
+    let response: Value = app.call_and_read_body_json(request).await;
+    log::info!("{}", serde_json::to_string_pretty(&response)?);


Please use log::debug! inside tests. Our default test output is noisy enough.

your right.. will fix.

jcrossley3 · 2025-01-23T18:10:06Z

modules/analysis/src/endpoints/test.rs

+            m == &&json!({
+              "sbom_id":  sbom["sbom_id"],
+              "node_id": m["node_id"],
+              "relationship": "PackageOf",
+              "purl": m["purl"], // long list assume it's correct
+              "cpe": m["cpe"], // long list assume it's correct
+              "name": "rubygem-google-cloud-compute",
+              "version": "0.5.0-1.el8sat"
+            })


To me, this is a brittle test. I'd want my expectation to be the minimum required to affirm the test. If some future change adds or takes away one of the fields that has nothing to do with this test, it's still gonna break. Is that good or annoying? I'd rather something like this, maybe even omitting name and version, too:

Suggested change

m == &&json!({

"sbom_id": sbom["sbom_id"],

"node_id": m["node_id"],

"relationship": "PackageOf",

"purl": m["purl"], // long list assume it's correct

"cpe": m["cpe"], // long list assume it's correct

"name": "rubygem-google-cloud-compute",

"version": "0.5.0-1.el8sat"

})

m["sbom_id"] == sbom["sbom_id"]

&& m["relationship"] == "PackageOf"

&& m["name"] == "rubygem-google-cloud-compute"

&& m["version"] == "0.5.0-1.el8sat"

I kinda was thinking about similar lines. Is there an existing function that can test if an actual Value matches a partial set of fields of another expected Value? The assertion would become more concise and less brittle then.

Not that I know of. You can always downcast the Value to an AncestorSummary or DepSummary, but I'm not sure m["name"] is all that better or worse than m.name.

Another option is to use Value::pointer, but I haven't personally tried that.

jcrossley3

I like where you're going, but you haven't gone far enough!!!

jcrossley3 · 2025-01-23T20:29:49Z

modules/analysis/src/test.rs

+// 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,
+    }
+}


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?

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?

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.

Great ideas.. my next commit will have a version of it.

chirino · 2025-01-23T19:36:42Z

modules/analysis/src/test.rs

 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 {


@jcrossley3 is there a better file to place this in? Or a better way to implement it?

chirino · 2025-01-24T03:46:09Z

modules/analysis/src/test.rs

+// 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,
+    }
+}


Great ideas.. my next commit will have a version of it.

chirino · 2025-01-24T03:55:08Z

test-context/src/lib.rs

+    fn contains_subset(&self, value: Value) -> bool;
+    // Returns true if the value a deep subset of the receiver.
+    fn contains_deep_subset(&self, value: Value) -> bool;


@jcrossley3 did a shallow and a deep version so that the caller can choose how strict the field matching is.

Very nice! I might suggest putting the trait, impl and tests in another module, maybe test-context/src/subset.rs? lib.rs is getting kinda crowded, I think.

Added a ContainsSubset trait which allows you to Test if a value has a subset of elements/fields And also deep version which does it recursively. Signed-off-by: Hiram Chirino <[email protected]>

jcrossley3

We're getting there!

jcrossley3 · 2025-01-24T17:59:18Z

modules/analysis/src/endpoints/test.rs

+    let response: Value = app.call_and_read_body_json(request).await;
+    log::debug!("{}", serde_json::to_string_pretty(&response)?);
+
+    let sbom = &response["items"][0];


I think this line is why the test is failing. There are actually 2 items returned in the list. You could either filter them further to construct your matches or just replace all of it with:

assert!(response.contains_deep_subset(json!({ "items": [ { "ancestors": [ { "relationship": "PackageOf", "name": "rubygem-google-cloud-compute", "version": "0.5.0-1.el8sat" } ] } ] })));

your right.. lets do it the easy way.

jcrossley3 · 2025-01-24T18:03:02Z

test-context/src/subset.rs

+            (Value::Array(src), Value::Array(subset)) => {
+                subset.iter().all(|v| src.iter().any(|x| x == v))
+            }


I don't think this is necessary, because contains_deep_subset already does this. We want to be able to pass in an array and have it match explicitly, e.g. m.contains_subset(json!({"purl": [ "pkg:rpm/redhat/[email protected]?arch=src" ]}))

This is so that:

let actual = json!([{"a":1}]); actual.contains_subset(json!([{"a":1, "b":2}])); // should return false

if we used contains_deep_subset then it would return true.

I would expect the absence of b to cause that to return false. I'm not sure how that Value::Array branch would even come into play here. What am I missing?

confirmed:

let actual = json!([{"a":1}]); assert!(!actual.contains_subset(json!([{"a":1, "b":2}]))); assert!(!actual.contains_deep_subset(json!([{"a":1, "b":2}])));

Maybe add that to test_array_subset?

This works as I'd expect, too:

let actual = json!([{"a":1, "b": 2}]); assert!(!actual.contains_subset(json!([{"a":1}]))); assert!(actual.contains_deep_subset(json!([{"a":1}])));

Maybe contains_partial_subset seems best in light of those?

jcrossley3 · 2025-01-24T18:11:16Z

test-context/src/subset.rs

+        // actual can have additional fields
+        let actual = json!([1, 2, 3]);
+        assert!(actual.contains_subset(json!([2])));
+
+        // other values can be interleaved.
+        let actual = json!([1, 2, 3]);
+        assert!(actual.contains_subset(json!([1, 3])));
+
+        // case where a value is missing
+        let actual = json!([1, 2, 3]);
+        assert!(!actual.contains_subset(json!([0])));


Maybe "strict" or "fuzzy" or "partial" are better descriptors than "deep". The crucial idea is that we need both a way to test for an explicit match and a way to ask "are all of these in the other one?"

I think this test is more accurate thusly:

Suggested change

// actual can have additional fields

let actual = json!([1, 2, 3]);

assert!(actual.contains_subset(json!([2])));

// other values can be interleaved.

let actual = json!([1, 2, 3]);

assert!(actual.contains_subset(json!([1, 3])));

// case where a value is missing

let actual = json!([1, 2, 3]);

assert!(!actual.contains_subset(json!([0])));

let actual = json!([1, 2, 3]);

assert!(actual.contains_subset(json!([1, 2, 3])));

assert!(!actual.contains_subset(json!([2])));

assert!(actual.contains_deep_subset(json!([2])));

assert!(actual.contains_deep_subset(json!([1, 3])));

assert!(!actual.contains_deep_subset(json!([0])));

But again, I'm not sure "deep" is the right word. We should always recurse through a recursive structure.

Yeah. I'd be happy with any of those options.

Signed-off-by: Hiram Chirino <[email protected]>

jcrossley3 · 2025-01-24T21:03:31Z

Looking at it again after my walk, I noticed that contains_subset isn't recursive. This makes the "deep" in the other fn name make more sense, but IMO it makes contains_subset less useful. I don't see much added value in a non-recursive comparison over just doing the comparisons explicitly inside the .filter(...) fn.

I do find the contains_deep_subset useful as implemented, and I'd probably just rename that to contains_subset and have a single fn in the trait.

But I'll approve the PR and leave it to you. As we integrate the feature into more tests, hopefully The Right Way will reveal itself.

chirino · 2025-01-26T18:07:40Z

@jcrossley3 thanks for all the help and guidance on this PR. I'll queue to merge as is, but feel free to rename those functions if you have a better option. Naming is hard. Additional options:

contains_all : for the deep version: the _all makes it more obvious the passed parameter should be a collection.
contains_all_exact/strict: for the the more strict version.

jcrossley3 · 2025-01-26T18:24:22Z

Naming is hard.

Amen! I enjoyed the collaboration!

Add tests for spdx "relationshipType": "PACKAGE_OF"

63f860f

Verify the relationship type shows up in `/api/v2/analysis/root-component` API calls. Part of issue trustification#1140 Signed-off-by: Hiram Chirino <[email protected]>

chirino requested a review from jcrossley3 January 23, 2025 17:22

jcrossley3 reviewed Jan 23, 2025

View reviewed changes

jcrossley3 requested changes Jan 23, 2025

View reviewed changes

chirino force-pushed the package_of branch from 618f27b to 313232f Compare January 24, 2025 03:53

chirino commented Jan 24, 2025

View reviewed changes

Improve tests based on PR feedback.

c47dcaa

Added a ContainsSubset trait which allows you to Test if a value has a subset of elements/fields And also deep version which does it recursively. Signed-off-by: Hiram Chirino <[email protected]>

chirino force-pushed the package_of branch from 313232f to c47dcaa Compare January 24, 2025 14:33

jcrossley3 requested changes Jan 24, 2025

View reviewed changes

Fix failing test.

290d374

Signed-off-by: Hiram Chirino <[email protected]>

jcrossley3 approved these changes Jan 24, 2025

View reviewed changes

chirino added this pull request to the merge queue Jan 26, 2025

Merged via the queue into trustification:main with commit 14da853 Jan 26, 2025
1 of 2 checks passed

chirino deleted the package_of branch January 26, 2025 18:29

		log::debug!("{response:#?}");
		log::debug!("{}", serde_json::to_string_pretty(&response)?);

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

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

Uh oh!

Conversation

chirino commented Jan 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcrossley3 left a comment

Choose a reason for hiding this comment

Uh oh!

jcrossley3 Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcrossley3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcrossley3 commented Jan 24, 2025

Uh oh!

chirino commented Jan 26, 2025

Uh oh!

jcrossley3 commented Jan 26, 2025

Uh oh!

Uh oh!

Uh oh!

jcrossley3 Jan 23, 2025 •

edited

Loading