Open
Conversation
Assisted-by: Claude Code
Assisted-by: Claude Code
Assisted-by: Claude Code
Also, restructure some things.
Contributor
Reviewer's GuideIntroduce SBOM group management: a new hierarchical grouping model for SBOMs with full CRUD REST API, validations, permissions, database schema/migrations, and extensive tests, including support for querying groups by name (including the literal string "null") via the existing q-filter mechanism. Sequence diagram for SBOM group update with revision and cycle validationsequenceDiagram
actor Client
participant Api as SbomGroupEndpoints
participant Service as SbomGroupService
participant DB as Database
Client->>Api: PUT /v2/group/sbom/{id}\nIf-Match: ETag\nGroupRequest
Api->>Api: extract_revision(IfMatch)
Api->>DB: begin()
DB-->>Api: Transaction
Api->>Service: update(id, revision, group, tx)
Service->>Service: validate_group_name_or_fail(group.name)
Service->>Service: parse_parent_group(group.parent)
alt parent is Some
Service->>Service: validate_no_cycle(id, parent_id, tx)
Service-->>Service: ok or Conflict
end
Service->>DB: query_by_revision(id, revision).update_many(...)
DB-->>Service: rows_affected or unique_violation
alt unique_violation
Service-->>Api: Error::Conflict
Api->>DB: rollback()
DB-->>Api: ok
Api-->>Client: 409 Conflict
else rows_affected == 0
Service->>DB: query_by_revision(id, None).count()
DB-->>Service: count
alt count == 0
Service-->>Api: Error::NotFound
Api->>DB: rollback()
DB-->>Api: ok
Api-->>Client: 404 Not Found
else count > 0
Service-->>Api: Error::RevisionNotFound
Api->>DB: rollback()
DB-->>Api: ok
Api-->>Client: 412 Precondition Failed
end
else success
Service-->>Api: Ok
Api->>DB: commit()
DB-->>Api: ok
Api-->>Client: 204 No Content
end
ER diagram for SBOM groups and assignmentserDiagram
Sbom {
uuid sbom_id PK
}
SbomGroup {
uuid id PK
option_uuid parent FK
string name
uuid revision
jsonb labels
}
SbomGroupAssignment {
uuid sbom_id FK
uuid group_id FK
}
SbomGroup ||--o| SbomGroup : parent
Sbom ||--o{ SbomGroupAssignment : has_assignments
SbomGroup ||--o{ SbomGroupAssignment : has_assignments
Sbom ||--o{ SbomGroup : belongs_to_via_assignment
Class diagram for SBOM group service and modelsclassDiagram
class SbomGroupService {
+usize max_group_name_length
+new(max_group_name_length usize) SbomGroupService
+list(options ListOptions, paginated Paginated, query Query, db ConnectionTrait) Result~PaginatedResults_GroupDetails_, Error~
+create(group GroupRequest, db ConnectionTrait) Result~Revisioned_string_, Error~
+delete(id str, expected_revision Option_str_, db ConnectionTrait) Result~bool, Error~
+update(id str, revision Option_str_, group GroupRequest, db ConnectionTrait) Result~void, Error~
+read(id str, db ConnectionTrait) Result~Option_Revisioned_Group__, Error~
-resolve_totals(ids Uuid[], db ConnectionTrait, query Selector_SelectGetableTuple_Uuid_i64__ ) Result~u64[], Error~
-resolve_total_groups(ids Uuid[], db ConnectionTrait) Result~u64[], Error~
-resolve_total_sboms(ids Uuid[], db ConnectionTrait) Result~u64[], Error~
-resolve_parents(ids Uuid[], db ConnectionTrait) Result~Vec_Vec_string__, Error~
-update_columns(id str, revision Option_str_, updates Vec_Tuple2_sbom_group_Column_SimpleExpr__, db ConnectionTrait) Result~void, Error~
-validate_group_name(name str) Vec_Cow_static_str__
-validate_group_name_or_fail(name str) Result~void, Error~
-validate_no_cycle(group_id str, parent_id str, db ConnectionTrait) Result~void, Error~
}
class ListOptions {
+bool totals
+bool parents
}
class Group {
+string id
+Option_string_ parent
+string name
+Labels labels
}
class GroupDetails {
+Group group
+Option_u64_ number_of_groups
+Option_u64_ number_of_sboms
+Option_Vec_string__ parents
}
class GroupRequest {
+string name
+Option_string_ parent
+Labels labels
}
class Error {
<<enum>>
+BadRequest(Cow_static_str_, Option_Cow_static_str_)
+Conflict(Cow_static_str_)
+RevisionNotFound
+bad_request(message Into_Cow_static_str_, details Option_Into_Cow_static_str_) Error
}
class sbom_group_Model {
+Uuid id
+Option_Uuid_ parent
+string name
+Uuid revision
+Labels labels
}
class sbom_group_assignment_Model {
+Uuid sbom_id
+Uuid group_id
}
class Paginated {
+i64 offset
+i64 limit
}
class PaginatedResults_GroupDetails_ {
+Vec_GroupDetails_ items
+u64 total
}
class Revisioned_T_ {
+T value
+string revision
}
class Query {
}
class ConnectionTrait {
<<trait>>
}
GroupDetails --> Group : deref
GroupRequest ..> Group : mutable_properties_of
SbomGroupService --> ListOptions
SbomGroupService --> Group
SbomGroupService --> GroupDetails
SbomGroupService --> GroupRequest
SbomGroupService --> Error
SbomGroupService --> Paginated
SbomGroupService --> PaginatedResults_GroupDetails_
SbomGroupService --> Revisioned_T_
SbomGroupService --> Query
SbomGroupService --> ConnectionTrait
Group ..> sbom_group_Model : From
sbom_group_Model --> Labels
Group --> Labels
GroupRequest --> Labels
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 the OpenAPI spec for
/api/v2/group/sbom/{id}, theidparameter is declared both as a query and a path parameter; this duplication is confusing and the query parameter should be removed so the API definition matches the actual path-based routing. - In
SbomGroupService::validate_no_cycle, the recursive CTE uses a hard-codedDatabaseBackend::Postgresand raw SQL; consider usingdb.get_database_backend()and aFromQueryResult-based query to avoid backend assumptions and keep the DB access consistent with the rest of the codebase. - The group name validation error for invalid characters currently ends with a trailing comma (
"name contains invalid characters, "); tightening this message (e.g., removing the comma or clarifying the allowed character set) would make client-side error handling and display clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the OpenAPI spec for `/api/v2/group/sbom/{id}`, the `id` parameter is declared both as a query and a path parameter; this duplication is confusing and the query parameter should be removed so the API definition matches the actual path-based routing.
- In `SbomGroupService::validate_no_cycle`, the recursive CTE uses a hard-coded `DatabaseBackend::Postgres` and raw SQL; consider using `db.get_database_backend()` and a `FromQueryResult`-based query to avoid backend assumptions and keep the DB access consistent with the rest of the codebase.
- The group name validation error for invalid characters currently ends with a trailing comma (`"name contains invalid characters, "`); tightening this message (e.g., removing the comma or clarifying the allowed character set) would make client-side error handling and display clearer.
## Individual Comments
### Comment 1
<location> `modules/fundamental/src/sbom_group/service.rs:362-366` </location>
<code_context>
+ has_cycle: bool,
+ }
+
+ let result = CycleCheck::find_by_statement(sea_orm::Statement::from_sql_and_values(
+ sea_orm::DatabaseBackend::Postgres,
+ sql,
+ vec![parent_id.into(), group_id.into()],
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Cycle check uses a hard-coded Postgres backend instead of the actual connection backend.
`validate_no_cycle` builds the statement with `DatabaseBackend::Postgres` instead of using the backend from `db` (e.g. `db.get_database_backend()`). This will fail on non-Postgres connections and is inconsistent with other queries here. Please derive the backend from `db` so this remains backend-agnostic and consistent.
```suggestion
let result = CycleCheck::find_by_statement(sea_orm::Statement::from_sql_and_values(
db.get_database_backend(),
sql,
vec![parent_id.into(), group_id.into()],
))
```
</issue_to_address>
### Comment 2
<location> `modules/fundamental/src/sbom_group/endpoints/test.rs:862-865` </location>
<code_context>
+}
+
+/// Test using an invalid parent ID
+#[ignore = "Caused by the q implementation"]
+#[test_context(TrustifyContext)]
+#[test_log::test(actix_web::test)]
+pub async fn list_groups_with_invalid_parent(ctx: &TrustifyContext) -> Result<(), anyhow::Error> {
+ let app = caller(ctx).await?;
+
</code_context>
<issue_to_address>
**suggestion (testing):** Clarify or adjust the ignored `list_groups_with_invalid_parent` test
This test is ignored due to a `q` limitation, which makes it easy to forget to re-enable. Either update the assertions to match the current, documented behavior so it can run now (and adjust later when `q` is fixed), or add a short comment linking to an issue/ADR that describes the intended future behavior and when this test should be restored. This will help avoid accumulating permanently ignored tests.
Suggested implementation:
```rust
//// Test using an invalid parent ID
///
/// This test exercises the behavior when an invalid parent ID is provided. It is
/// currently ignored due to a limitation in the `q` implementation when handling
/// invalid parent IDs. Once that limitation is fixed, this test should be re-enabled.
/// See tracking issue / ADR: <INSERT_ISSUE_URL_OR_ADR_REFERENCE_HERE>.
#[ignore = "Blocked by `q`'s handling of invalid parent IDs; see tracking issue in comment above"]
```
1. Replace `<INSERT_ISSUE_URL_OR_ADR_REFERENCE_HERE>` with the actual issue or ADR link that tracks the `q` limitation and future behavior.
2. When the `q` limitation is resolved, remove the `#[ignore(...)]` attribute and update the comment if necessary to reflect the final, intended behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+362
to
+366
| let result = CycleCheck::find_by_statement(sea_orm::Statement::from_sql_and_values( | ||
| sea_orm::DatabaseBackend::Postgres, | ||
| sql, | ||
| vec![parent_id.into(), group_id.into()], | ||
| )) |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Cycle check uses a hard-coded Postgres backend instead of the actual connection backend.
validate_no_cycle builds the statement with DatabaseBackend::Postgres instead of using the backend from db (e.g. db.get_database_backend()). This will fail on non-Postgres connections and is inconsistent with other queries here. Please derive the backend from db so this remains backend-agnostic and consistent.
Suggested change
| let result = CycleCheck::find_by_statement(sea_orm::Statement::from_sql_and_values( | |
| sea_orm::DatabaseBackend::Postgres, | |
| sql, | |
| vec![parent_id.into(), group_id.into()], | |
| )) | |
| let result = CycleCheck::find_by_statement(sea_orm::Statement::from_sql_and_values( | |
| db.get_database_backend(), | |
| sql, | |
| vec![parent_id.into(), group_id.into()], | |
| )) |
Comment on lines
+862
to
+865
| #[ignore = "Caused by the q implementation"] | ||
| #[test_context(TrustifyContext)] | ||
| #[test_log::test(actix_web::test)] | ||
| pub async fn list_groups_with_invalid_parent(ctx: &TrustifyContext) -> Result<(), anyhow::Error> { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Clarify or adjust the ignored list_groups_with_invalid_parent test
This test is ignored due to a q limitation, which makes it easy to forget to re-enable. Either update the assertions to match the current, documented behavior so it can run now (and adjust later when q is fixed), or add a short comment linking to an issue/ADR that describes the intended future behavior and when this test should be restored. This will help avoid accumulating permanently ignored tests.
Suggested implementation:
//// Test using an invalid parent ID
///
/// This test exercises the behavior when an invalid parent ID is provided. It is
/// currently ignored due to a limitation in the `q` implementation when handling
/// invalid parent IDs. Once that limitation is fixed, this test should be re-enabled.
/// See tracking issue / ADR: <INSERT_ISSUE_URL_OR_ADR_REFERENCE_HERE>.
#[ignore = "Blocked by `q`'s handling of invalid parent IDs; see tracking issue in comment above"]- Replace
<INSERT_ISSUE_URL_OR_ADR_REFERENCE_HERE>with the actual issue or ADR link that tracks theqlimitation and future behavior. - When the
qlimitation is resolved, remove the#[ignore(...)]attribute and update the comment if necessary to reflect the final, intended behavior.
Contributor
At present, the literal string "null" is used for querying NULL's in the db, e.g. If it's a high priority requirement, we'd need to redesign the |
jcrossley3
added a commit
to jcrossley3/trustify
that referenced
this pull request
Feb 4, 2026
Relates guacsec#2230 BREAKING-CHANGE: Querying for NULL fields is now achieved using an ASCII NUL value, percent-encoded as %00, instead of the literal string "null".
Contributor
Author
I think having to explain the in the docs that you're not supposed to name a group Creating a new query syntax for the group endpoint is equally strange. So I think this issue should be addressed. |
Contributor
Is the title of #2233 not clear enough? I requested your review 2 days ago. |
Contributor
Author
Just commented on it. |
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
2 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.
@jcrossley3 I need your help with this, how can a user search for a group named
null?Summary by Sourcery
Introduce SBOM grouping support with a new hierarchical group model, API endpoints, and storage, including validation, permissions, and documentation.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: