feat: add PATCH endpoint for partial SBOM group assignment updates#2450
Open
ctron wants to merge 2 commits into
Open
feat: add PATCH endpoint for partial SBOM group assignment updates#2450ctron wants to merge 2 commits into
ctron wants to merge 2 commits into
Conversation
Contributor
Reviewer's GuideAdds a new PATCH /v3/group/sbom-assignment endpoint for partial SBOM group assignment updates, including service-layer logic, request model, routing, documentation, and comprehensive integration tests. Sequence diagram for PATCH sbom group assignment endpointsequenceDiagram
actor User
participant Endpoint_patch_assignments
participant SbomGroupService
participant Db
User->>Endpoint_patch_assignments: PATCH /v3/group/sbom-assignment
Endpoint_patch_assignments->>Db: begin
Endpoint_patch_assignments->>SbomGroupService: patch_assignments(sbom_ids, add, remove, tx)
SbomGroupService->>Db: sbom::Entity::update_many.exec
SbomGroupService->>Db: sbom_group_assignment::Entity::delete_many.exec
SbomGroupService->>Db: sbom_group_assignment::Entity::insert_many.on_conflict.do_nothing.exec
SbomGroupService-->>Endpoint_patch_assignments: Result<(), Error>
Endpoint_patch_assignments->>Db: commit
Endpoint_patch_assignments-->>User: 204 NoContent
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
patch_assignments, the insert_many call uses both.on_conflict(...).do_nothing()and a trailing.do_nothing(), which is redundant and can be simplified to rely on the configuredOnConflictclause only. - The OpenAPI/utoipa annotation for
patch_assignmentsonly declares 204/400/401/403 but the service can returnError::NotFound, so consider adding a 404 response to the path metadata to align the API description with actual behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `patch_assignments`, the insert_many call uses both `.on_conflict(...).do_nothing()` and a trailing `.do_nothing()`, which is redundant and can be simplified to rely on the configured `OnConflict` clause only.
- The OpenAPI/utoipa annotation for `patch_assignments` only declares 204/400/401/403 but the service can return `Error::NotFound`, so consider adding a 404 response to the path metadata to align the API description with actual behavior.
## Individual Comments
### Comment 1
<location path="modules/fundamental/src/sbom_group/endpoints/mod.rs" line_range="52" />
<code_context>
+ .service(patch_assignments);
}
#[utoipa::path(
</code_context>
<issue_to_address>
**issue (bug_risk):** The OpenAPI responses omit 404, even though the service can return `NotFound`.
`patch_assignments` can return `Error::NotFound` (e.g., when SBOM IDs fail to parse or SBOMs are missing), but the `utoipa::path` annotation only declares 204/400/401/403. This divergence between behavior and OpenAPI spec can mislead generated clients and integrators. Please either document a 404 response in the path or adjust the error handling to use one of the declared status codes.
</issue_to_address>
### Comment 2
<location path="modules/fundamental/src/sbom_group/endpoints/test/assignment.rs" line_range="446-455" />
<code_context>
+
+#[test_context(TrustifyContext)]
+#[test_log::test(actix_web::test)]
+async fn patch_sbom_group_assignments_add(ctx: &TrustifyContext) -> anyhow::Result<()> {
+ let app = caller(ctx).await?;
+
+ let group1: GroupResponse = Create::new("Group 1").execute(&app).await?;
+ let group2: GroupResponse = Create::new("Group 2").execute(&app).await?;
+
+ let sbom = ctx
+ .ingest_document("zookeeper-3.9.2-cyclonedx.json")
+ .await?;
+ let sbom_id = sbom.id.to_string();
+
+ PatchAssignments::new(vec![sbom_id.clone()])
+ .add_groups(vec![group1.id.clone(), group2.id.clone()])
+ .execute(&app)
+ .await?;
+
+ assert_assigned_groups(&app, &sbom_id, &[&group1.id, &group2.id]).await?;
+
+ Ok(())
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for PATCH semantics when multiple SBOM IDs are passed (cartesian product behavior)
Current tests only cover a single SBOM, but the PATCH endpoint is documented to apply cartesian product semantics across all `sbom_ids` and `add`/`remove` groups. Please add at least one integration test using `PatchAssignments::new(vec![sbom1_id.clone(), sbom2_id.clone()])` with multiple groups in `add` and/or `remove`, and assert that each SBOM receives the expected assignments. This will verify the bulk semantics and help prevent regressions in multi-SBOM behavior.
Suggested implementation:
```rust
// --- PATCH assignment tests ---
//
// Bulk PATCH assignment semantics: multiple SBOMs with multiple groups (cartesian product)
#[test_context(TrustifyContext)]
#[test_log::test(actix_web::test)]
async fn patch_sbom_group_assignments_bulk_add(ctx: &TrustifyContext) -> anyhow::Result<()> {
let app = caller(ctx).await?;
// create groups to be added
let group1: GroupResponse = Create::new("Bulk Group 1").execute(&app).await?;
let group2: GroupResponse = Create::new("Bulk Group 2").execute(&app).await?;
// ingest two SBOMs
let sbom1 = ctx.ingest_document("zookeeper-3.9.2-cyclonedx.json").await?;
let sbom2 = ctx.ingest_document("zookeeper-3.9.2-cyclonedx.json").await?;
let sbom1_id = sbom1.id.to_string();
let sbom2_id = sbom2.id.to_string();
// apply PATCH with multiple sbom_ids and multiple groups; should be cartesian product
PatchAssignments::new(vec![sbom1_id.clone(), sbom2_id.clone()])
.add_groups(vec![group1.id.clone(), group2.id.clone()])
.execute(&app)
.await?;
// verify each SBOM has both groups assigned
assert_assigned_groups(&app, &sbom1_id, &[&group1.id, &group2.id]).await?;
assert_assigned_groups(&app, &sbom2_id, &[&group1.id, &group2.id]).await?;
Ok(())
}
// --- PATCH assignment tests ---
```
- This edit assumes `GroupResponse`, `Create`, `PatchAssignments`, and `assert_assigned_groups` are already imported in `assignment.rs`. If any are missing, add the appropriate `use` statements at the top of the file, consistent with existing conventions.
- If the test module uses specific naming or grouping/ordering conventions for tests (e.g., all `patch_*` tests grouped together), you may want to move `patch_sbom_group_assignments_bulk_add` next to the other PATCH assignment tests to keep the file organized.
- If there is an existing helper for creating multiple SBOMs or groups in tests, you can refactor the test to reuse that helper instead of calling `ingest_document` and `Create::new` directly, while keeping the core cartesian-product assertions unchanged.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| .service(patch_assignments); | ||
| } | ||
|
|
||
| #[utoipa::path( |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The OpenAPI responses omit 404, even though the service can return NotFound.
patch_assignments can return Error::NotFound (e.g., when SBOM IDs fail to parse or SBOMs are missing), but the utoipa::path annotation only declares 204/400/401/403. This divergence between behavior and OpenAPI spec can mislead generated clients and integrators. Please either document a 404 response in the path or adjust the error handling to use one of the declared status codes.
Comment on lines
+446
to
+455
| async fn patch_sbom_group_assignments_add(ctx: &TrustifyContext) -> anyhow::Result<()> { | ||
| let app = caller(ctx).await?; | ||
|
|
||
| let group1: GroupResponse = Create::new("Group 1").execute(&app).await?; | ||
| let group2: GroupResponse = Create::new("Group 2").execute(&app).await?; | ||
|
|
||
| let sbom = ctx | ||
| .ingest_document("zookeeper-3.9.2-cyclonedx.json") | ||
| .await?; | ||
| let sbom_id = sbom.id.to_string(); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add coverage for PATCH semantics when multiple SBOM IDs are passed (cartesian product behavior)
Current tests only cover a single SBOM, but the PATCH endpoint is documented to apply cartesian product semantics across all sbom_ids and add/remove groups. Please add at least one integration test using PatchAssignments::new(vec![sbom1_id.clone(), sbom2_id.clone()]) with multiple groups in add and/or remove, and assert that each SBOM receives the expected assignments. This will verify the bulk semantics and help prevent regressions in multi-SBOM behavior.
Suggested implementation:
// --- PATCH assignment tests ---
//
// Bulk PATCH assignment semantics: multiple SBOMs with multiple groups (cartesian product)
#[test_context(TrustifyContext)]
#[test_log::test(actix_web::test)]
async fn patch_sbom_group_assignments_bulk_add(ctx: &TrustifyContext) -> anyhow::Result<()> {
let app = caller(ctx).await?;
// create groups to be added
let group1: GroupResponse = Create::new("Bulk Group 1").execute(&app).await?;
let group2: GroupResponse = Create::new("Bulk Group 2").execute(&app).await?;
// ingest two SBOMs
let sbom1 = ctx.ingest_document("zookeeper-3.9.2-cyclonedx.json").await?;
let sbom2 = ctx.ingest_document("zookeeper-3.9.2-cyclonedx.json").await?;
let sbom1_id = sbom1.id.to_string();
let sbom2_id = sbom2.id.to_string();
// apply PATCH with multiple sbom_ids and multiple groups; should be cartesian product
PatchAssignments::new(vec![sbom1_id.clone(), sbom2_id.clone()])
.add_groups(vec![group1.id.clone(), group2.id.clone()])
.execute(&app)
.await?;
// verify each SBOM has both groups assigned
assert_assigned_groups(&app, &sbom1_id, &[&group1.id, &group2.id]).await?;
assert_assigned_groups(&app, &sbom2_id, &[&group1.id, &group2.id]).await?;
Ok(())
}
// --- PATCH assignment tests ---- This edit assumes
GroupResponse,Create,PatchAssignments, andassert_assigned_groupsare already imported inassignment.rs. If any are missing, add the appropriateusestatements at the top of the file, consistent with existing conventions. - If the test module uses specific naming or grouping/ordering conventions for tests (e.g., all
patch_*tests grouped together), you may want to movepatch_sbom_group_assignments_bulk_addnext to the other PATCH assignment tests to keep the file organized. - If there is an existing helper for creating multiple SBOMs or groups in tests, you can refactor the test to reuse that helper instead of calling
ingest_documentandCreate::newdirectly, while keeping the core cartesian-product assertions unchanged.
3 tasks
The existing PUT endpoint uses replace semantics — assigning an SBOM to a new group removes it from all previous groups. This adds a PATCH endpoint (`PATCH /v3/group/sbom-assignment`) that supports additive and subtractive assignment changes, preserving existing assignments. Uses cartesian product semantics: each SBOM in sbom_ids gets all add groups added and all remove groups removed. Idempotent via ON CONFLICT DO NOTHING for adds and targeted DELETE for removes. Fixes: TC-5036 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ctron
force-pushed
the
fix/sbom-group-patch-assignment
branch
from
9c2c6c0 to
e0f82b4
Compare
ctron
added a commit
to ctron/trustify-ui
that referenced
this pull request
Jul 6, 2026
Switch useAddSBOMsToGroupsMutation from the PUT bulk assignment endpoint (which replaces all assignments) to the new PATCH endpoint that preserves existing group assignments when adding an SBOM to a new group. Depends on: guacsec/trustify#2450 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2450 +/- ##
==========================================
+ Coverage 71.51% 71.62% +0.10%
==========================================
Files 453 453
Lines 27360 27470 +110
Branches 27360 27470 +110
==========================================
+ Hits 19567 19675 +108
+ Misses 6672 6662 -10
- Partials 1121 1133 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PATCH /v3/group/sbom-assignmentendpoint for partial SBOM group assignment updatessbom_idsgets alladdgroups added and allremovegroups removedFixes: TC-5036
Test plan
cargo fmt— cleancargo clippy— clean🤖 Generated with Claude Code
Summary by Sourcery
Add support for partially updating SBOM group assignments via a new PATCH API endpoint and document its behavior.
New Features:
Enhancements:
Tests: