Issue 2049 : Improve component search performance with JDBI#1867
Issue 2049 : Improve component search performance with JDBI#1867sahibamittal wants to merge 21 commits intomainfrom
Conversation
api/src/main/openapi/components/schemas/dependency-metrics.yaml
Outdated
Show resolved
Hide resolved
apiserver/src/main/java/org/dependencytrack/resources/v2/ComponentsResource.java
Show resolved
Hide resolved
apiserver/src/main/java/org/dependencytrack/resources/v2/ComponentsResource.java
Outdated
Show resolved
Hide resolved
apiserver/src/test/java/org/dependencytrack/resources/v2/ComponentsResourceTest.java
Show resolved
Hide resolved
api/src/main/openapi/components/schemas/list-components-response-item.yaml
Show resolved
Hide resolved
apiserver/src/main/java/org/dependencytrack/persistence/jdbi/ComponentDao.java
Outdated
Show resolved
Hide resolved
apiserver/src/main/java/org/dependencytrack/persistence/jdbi/ComponentDao.java
Show resolved
Hide resolved
apiserver/src/main/java/org/dependencytrack/persistence/jdbi/ComponentDao.java
Outdated
Show resolved
Hide resolved
apiserver/src/main/java/org/dependencytrack/persistence/jdbi/ComponentDao.java
Outdated
Show resolved
Hide resolved
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 2 medium |
🟢 Metrics 98 complexity
Metric Results Complexity 98
🟢 Coverage 73.04% diff coverage · -0.50% coverage variation
Metric Results Coverage variation ✅ -0.50% coverage variation (-1.00%) Diff coverage ✅ 73.04% diff coverage (70.00%) Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (1fc7e2a) Report Missing Report Missing Report Missing Head commit (2d5c9d9) 42695 (+13894) 35894 (+11538) 84.07% (-0.50%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#1867) 230 168 73.04% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
TIP This summary will be updated as you push new changes. Give us feedback
| AND ("C"."NAME" > :lastPrimaryValue | ||
| OR ("C"."NAME" = :lastPrimaryValue AND "C"."VERSION" < :lastSecondaryValue) | ||
| OR ("C"."NAME" = :lastPrimaryValue AND "C"."VERSION" = :lastSecondaryValue AND "C"."ID" > :lastId)) |
There was a problem hiding this comment.
VERSION column can be removed here since the default sorting no longer includes it.
apiserver/src/main/java/org/dependencytrack/persistence/jdbi/ComponentDao.java
Outdated
Show resolved
Hide resolved
apiserver/src/main/java/org/dependencytrack/persistence/jdbi/ComponentDao.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR introduces a new v2 components listing/search endpoint backed by an optimized JDBI query, aiming to improve component identity search performance and expand response data (project + metrics).
Changes:
- Added
GET /api/v2/componentswith filtering, sorting, and keyset pagination implemented viaComponentDao.listComponents(...). - Extended v2 API models/mapping to include project info and dependency metrics in component responses.
- Updated OpenAPI definitions and tests, and introduced dedicated schemas for project-components listing responses.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| apiserver/src/test/java/org/dependencytrack/resources/v2/ComponentsResourceTest.java | Adds test coverage for /api/v2/components pagination, sorting, filtering, ACL behavior, and error cases. |
| apiserver/src/test/java/org/dependencytrack/resources/v1/ComponentResourceTest.java | Fixes a test bug setting PURL on the wrong component instance. |
| apiserver/src/main/java/org/dependencytrack/resources/v2/mapping/ModelMapper.java | Adds mapping helpers for ComponentProject and DependencyMetrics. |
| apiserver/src/main/java/org/dependencytrack/resources/v2/WorkflowsResource.java | Exposes a sort-direction converter (now reused elsewhere). |
| apiserver/src/main/java/org/dependencytrack/resources/v2/ProjectsResource.java | Switches project-components listing response types to new ListProjectComponentsResponse* models. |
| apiserver/src/main/java/org/dependencytrack/resources/v2/ComponentsResource.java | Implements listComponents endpoint using JDBI, with input parsing/validation and mapping to v2 models. |
| apiserver/src/main/java/org/dependencytrack/persistence/jdbi/PaginationSupport.java | Adds bounded-total-count helper that can apply the API project ACL condition. |
| apiserver/src/main/java/org/dependencytrack/persistence/jdbi/ComponentDao.java | Adds the optimized JDBI query + pagination logic for listing/searching components (plus optional metrics loading). |
| api/src/main/openapi/paths/projects_uuid_components.yaml | Updates response schema reference for project components listing. |
| api/src/main/openapi/paths/components.yaml | Adds OpenAPI definition for GET /components (v2) with filtering/sorting/pagination. |
| api/src/main/openapi/components/schemas/list-project-components-response.yaml | New paginated schema for project components listing. |
| api/src/main/openapi/components/schemas/list-project-components-response-item.yaml | New response-item schema for project components listing. |
| api/src/main/openapi/components/schemas/list-components-response-item.yaml | Extends components list item schema with project and metrics; removes occurrence_count. |
| api/src/main/openapi/components/schemas/dependency-metrics.yaml | Adds schema for dependency metrics. |
| api/src/main/openapi/components/schemas/component-project.yaml | Adds schema for embedding project info in component responses. |
Comments suppressed due to low confidence (1)
apiserver/src/main/java/org/dependencytrack/resources/v2/WorkflowsResource.java:289
- Making
convert(SortDirection)publicon this resource appears to be only to support reuse from other resources (e.g., components). To avoid exposing resource-internal helpers as API surface, consider moving this mapping into a small shared utility class (or into the pagination/common layer) and keep this methodprivatehere.
public static org.dependencytrack.common.pagination.@Nullable SortDirection convert(
@Nullable SortDirection sortDirection) {
return switch (sortDirection) {
case ASC -> org.dependencytrack.common.pagination.SortDirection.ASC;
case DESC -> org.dependencytrack.common.pagination.SortDirection.DESC;
case null -> null;
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| summary: List all components | ||
| description: |- | ||
| Retrieves a list of all components that have the specified component identity. This resource accepts coordinates (group, name, version) or purl, cpe, or swidTagId. | ||
|
|
||
| ### Sortable fields |
There was a problem hiding this comment.
The operation description says it retrieves components with the “specified component identity”, but the endpoint (and tests) allow calling /api/v2/components with no identity-related query params at all (listing all accessible components). Please either (a) update the description to reflect that all filters are optional and omitting them lists all components, or (b) enforce that at least one identity filter must be provided.
| operationId: listComponents | ||
| summary: List all components | ||
| description: |- | ||
| Retrieves a list of all components that have the specified component identity. This resource accepts coordinates (group, name, version) or purl, cpe, or swidTagId. |
There was a problem hiding this comment.
The description mentions coordinates/purl/cpe/swidTagId, but the endpoint also supports filtering by project_uuid and by hash_type + hash (both are listed as parameters). Consider documenting these filter options in the description for completeness so API consumers don’t miss them.
| Retrieves a list of all components that have the specified component identity. This resource accepts coordinates (group, name, version) or purl, cpe, or swidTagId. | |
| Retrieves a list of all components that have the specified component identity. This resource accepts coordinates (group, name, version), `purl`, `cpe`, `swidTagId`, `project_uuid`, or a combination of `hash_type` and `hash`. |
| import static org.dependencytrack.persistence.jdbi.JdbiFactory.inJdbiTransaction; | ||
| import static org.dependencytrack.resources.v2.WorkflowsResource.convert; | ||
| import static org.dependencytrack.resources.v2.mapping.ModelMapper.mapDependencyMetrics; | ||
| import static org.dependencytrack.resources.v2.mapping.ModelMapper.mapHashes; | ||
| import static org.dependencytrack.resources.v2.mapping.ModelMapper.mapLicense; | ||
| import static org.dependencytrack.resources.v2.mapping.ModelMapper.mapOrganizationalContacts; | ||
| import static org.dependencytrack.resources.v2.mapping.ModelMapper.mapProject; |
There was a problem hiding this comment.
The v2 components endpoint is reusing WorkflowsResource.convert(SortDirection) via a static import, which forced making that method public on an otherwise unrelated resource. Please move this mapping to a shared utility (e.g., a pagination/sort mapping helper in resources.v2.mapping or common.pagination) and keep resource classes decoupled from each other.
| PackageURL packageURL = null; | ||
| if (purl != null) { | ||
| try { | ||
| packageURL = new PackageURL(purl); | ||
| } catch (MalformedPackageURLException e) { | ||
| throw new BadRequestException("Invalid package URL: %s".formatted(purl)); | ||
| } | ||
| } | ||
| if (cpe != null) { | ||
| try { | ||
| CpeParser.parse(cpe); | ||
| } catch (CpeParsingException e) { | ||
| throw new BadRequestException("Invalid CPE: %s".formatted(cpe)); | ||
| } | ||
| } | ||
| final Page<Component> componentsPage = handle.attach(ComponentDao.class) | ||
| .listComponents(projectId, true, packageURL != null ? packageURL.canonicalize().toLowerCase() : null, StringUtils.trimToNull(cpe), |
There was a problem hiding this comment.
purl/cpe are parsed/validated using the raw query parameter values, but later the DAO is called with trimToNull(...). This means requests with harmless leading/trailing whitespace will fail validation even though they’d otherwise work. Trim first (e.g., final var trimmedPurl = StringUtils.trimToNull(purl) / same for cpe) and use the trimmed value consistently for validation + querying.
| PackageURL packageURL = null; | |
| if (purl != null) { | |
| try { | |
| packageURL = new PackageURL(purl); | |
| } catch (MalformedPackageURLException e) { | |
| throw new BadRequestException("Invalid package URL: %s".formatted(purl)); | |
| } | |
| } | |
| if (cpe != null) { | |
| try { | |
| CpeParser.parse(cpe); | |
| } catch (CpeParsingException e) { | |
| throw new BadRequestException("Invalid CPE: %s".formatted(cpe)); | |
| } | |
| } | |
| final Page<Component> componentsPage = handle.attach(ComponentDao.class) | |
| .listComponents(projectId, true, packageURL != null ? packageURL.canonicalize().toLowerCase() : null, StringUtils.trimToNull(cpe), | |
| final var trimmedPurl = StringUtils.trimToNull(purl); | |
| final var trimmedCpe = StringUtils.trimToNull(cpe); | |
| PackageURL packageURL = null; | |
| if (trimmedPurl != null) { | |
| try { | |
| packageURL = new PackageURL(trimmedPurl); | |
| } catch (MalformedPackageURLException e) { | |
| throw new BadRequestException("Invalid package URL: %s".formatted(trimmedPurl)); | |
| } | |
| } | |
| if (trimmedCpe != null) { | |
| try { | |
| CpeParser.parse(trimmedCpe); | |
| } catch (CpeParsingException e) { | |
| throw new BadRequestException("Invalid CPE: %s".formatted(trimmedCpe)); | |
| } | |
| } | |
| final Page<Component> componentsPage = handle.attach(ComponentDao.class) | |
| .listComponents(projectId, true, packageURL != null ? packageURL.canonicalize().toLowerCase() : null, trimmedCpe, |
| if (componentHashType != null && componentHash != null) { | ||
| final HashType hashType = HashType.valueOf(componentHashType.toUpperCase()); | ||
| final String hashColumn = switch (hashType) { | ||
| case MD5 -> "\"C\".\"MD5\""; | ||
| case SHA1 -> "\"C\".\"SHA1\""; | ||
| case SHA_256 -> "\"C\".\"SHA_256\""; | ||
| case SHA_384 -> "\"C\".\"SHA_384\""; | ||
| case SHA_512 -> "\"C\".\"SHA_512\""; | ||
| case SHA3_256 -> "\"C\".\"SHA3_256\""; | ||
| case SHA3_384 -> "\"C\".\"SHA3_384\""; | ||
| case SHA3_512 -> "\"C\".\"SHA3_512\""; | ||
| case BLAKE2B_256 -> "\"C\".\"BLAKE2B_256\""; | ||
| case BLAKE2B_384 -> "\"C\".\"BLAKE2B_384\""; | ||
| case BLAKE2B_512 -> "\"C\".\"BLAKE2B_512\""; | ||
| case BLAKE3 -> "\"C\".\"BLAKE3\""; | ||
| }; | ||
| whereConditions.add("%s = :componentHash".formatted(hashColumn)); | ||
| queryParams.put("componentHash", componentHash); |
There was a problem hiding this comment.
HashType.valueOf(componentHashType.toUpperCase()) will throw IllegalArgumentException for any unsupported hash_type, which will bubble up as a 500 response. Please catch this and translate it to a 400 Bad Request (ideally with a clear message listing supported values), or validate hash_type at the resource layer before calling into the DAO.
| <#if hasCursor && sortByColumn?has_content> | ||
| AND ( | ||
| <#if sortDirection == "DESC"> | ||
| ("C"."${sortByColumn}" < | ||
| <#if sortByColumn == "LAST_RISKSCORE" > CAST(:lastPrimaryValue AS DOUBLE PRECISION) | ||
| <#else> :lastPrimaryValue | ||
| </#if> | ||
| OR ("C"."${sortByColumn}" = | ||
| <#if sortByColumn == "LAST_RISKSCORE" > CAST(:lastPrimaryValue AS DOUBLE PRECISION) | ||
| <#else> :lastPrimaryValue | ||
| </#if> | ||
| AND "C"."ID" > :lastId)) | ||
| <#else> | ||
| ("C"."${sortByColumn}" > | ||
| <#if sortByColumn == "LAST_RISKSCORE" > CAST(:lastPrimaryValue AS DOUBLE PRECISION) | ||
| <#else> :lastPrimaryValue | ||
| </#if> | ||
| OR ("C"."${sortByColumn}" = | ||
| <#if sortByColumn == "LAST_RISKSCORE" > CAST(:lastPrimaryValue AS DOUBLE PRECISION) | ||
| <#else>:lastPrimaryValue | ||
| </#if> | ||
| AND "C"."ID" > :lastId)) | ||
| </#if> | ||
| ) | ||
| <#elseif hasCursor && lastPrimaryValue?has_content && lastId?has_content> | ||
| AND ("C"."NAME" > :lastPrimaryValue | ||
| OR ("C"."NAME" = :lastPrimaryValue AND "C"."ID" > :lastId)) | ||
| </#if> |
There was a problem hiding this comment.
Keyset pagination for sort_by columns does not handle NULL values. For nullable columns like GROUP, PURL, CPE, SWIDTAGID, and LAST_RISKSCORE, the cursor predicate (<, >, =) evaluates to NULL and will (a) skip rows where the sort column is NULL, and (b) potentially make pagination get stuck when the last row’s sort value is NULL. The query needs explicit NULL ordering/handling (e.g., consistent NULLS LAST plus cursor predicates that include NULL rows) or the API must reject sorting by nullable fields.
Description
Old and existing:
/api/v1/component/identityNew:
/api/v2/componentsAddressed Issue
Closes DependencyTrack/hyades#2049
Additional Details
Frontend changes: DependencyTrack/hyades-frontend#438
Checklist