Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,9 @@ public FetchPackageMetadataResolutionCandidatesRes execute(
try {
purl = new PackageURL(purlStr);
} catch (MalformedPackageURLException e) {
LOGGER.warn("Failed to parse PURL '{}'; Assigning to empty resolver", purlStr, e);
purlsByResolver
.computeIfAbsent("", k -> new ArrayList<>())
.add(purlStr);
// Malformed PURL cannot be resolved and is data quality issue.
// Skip to avoid resolution attempts and spamming WARN logs on every cycle.
LOGGER.debug("Skipping malformed PURL '{}'", purlStr, e);
continue;
Comment on lines +76 to 79

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Simply skipping these would cause them to always remain eligible for resolution because no result is persisted for them.

The previous behavior assigned the "empty" resolver, which then later trivially persists empty results:

if (resolverName.isEmpty()) {
// The resolver name is empty when no resolver exists that
// supports the given batch of PURLs. In this case, simply
// store empty results from preventing these PURLs to become
// resolution candidates again in the next batch.
final var buffer = new ResultBuffer();
for (final String purlStr : purlStrings) {
buffer.addEmptyResult(purlStr);
}
buffer.flush();
return null;
}

This prevents the next FetchPackageMetadataResolutionCandidatesRes iteration from discovering entries with invalid PURLs again.

By skipping this mechanism, ResolvePackageMetadataWorkflow would effectively loop forever and never terminate. It basically breaks the exit condition, i.e. this query returning no results:

SELECT DISTINCT c."PURL"
FROM "COMPONENT" c
WHERE c."PURL" IS NOT NULL
AND NOT EXISTS (
SELECT 1
FROM "PACKAGE_ARTIFACT_METADATA" pam
JOIN "PACKAGE_METADATA" pm
ON pm."PURL" = pam."PACKAGE_PURL"
WHERE pam."PURL" = c."PURL"
AND pm."RESOLVED_AT" > NOW() - INTERVAL '24 hours'
)
ORDER BY c."PURL"
LIMIT :limit

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm seeing the same malformed PURL being logged on every resolution cycle with the current implementation. So the empty resolver path is not actually preventing it from being rediscovered. Same malformed PURL should not be processed again after the first successful workflow execution.
I'll check whether the empty result is being persisted and why FetchPackageMetadataResolutionCandidatesRes is still selecting it.

@sahibamittal sahibamittal Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay found it, I noticed that for the malformed PURL, ResultBuffer.addEmptyResult() returns immediately without persisting empty results because PurlUtil.silentPurl(purlStr) returns null:

final PackageURL purl = PurlUtil.silentPurl(purlStr);
if (purl == null) {
    return;
}

As a result, no PackageMetadata or PackageArtifactMetadata records are written, and the same component is selected again by FetchPackageMetadataResolutionCandidatesActivity on every cycle. This needs to be fixed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The model currently requires a valid PURL since the identifiers are PackageURL objects. We'd need a way to pass empty results for broken PURLs, ideally without loosening the types (i.e. changing from PackageURL to String) too much.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OR instead of persisting malformed PURLs (empty results), should it not just reject or sanitize them during BOM ingestion so they never reach the metadata resolver?

Also, currently we decide resolution state based on metadata tables if data exists. How about persisting the outcome of metadata resolution separately from the metadata itself. Say, PACKAGE_METADATA_RESOLUTION to record resolution state (e.g. SUCCESS, EMPTY, INVALID_PURL etc) keyed by the raw PURL. This way metadata becomes just the payload.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OR instead of persisting malformed PURLs (empty results), should it not just reject or sanitize them during BOM ingestion so they never reach the metadata resolver?

Hmmm that is already happening:

if (cdxComponent.getPurl() != null) {
try {
final var purl = new PackageURL(cdxComponent.getPurl());
component.setPurl(purl);
component.setPurlCoordinates(silentPurlCoordinatesOnly(purl));
} catch (MalformedPackageURLException e) {
LOGGER.debug("Encountered invalid PURL", e);
}
}

Which begs the question how you could still see invalid PURLs reaching the metadata resolver. I wonder if it's related to an encoding bug in the packageurl-java library (see #6383 where it came up recently). Does that error pattern match what you're observing in your instance?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I checked none of repository records have authenticationRequired set to null. Error is due to malformed escape pair in invalid PURL during metadata resolution:

image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, no I meant the PURL encoding issue that's also mentioned there.

pkg:nuget/serilog.sinks.file%A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%205.0.0@5.0.0 is likely not how the PURL looked when it entered the system, the packageurl-java lib may have borked it when converting it from PackageURL to String. That would explain why it survived the import:

  • First new PackageURL(cdxComponent.getPurl()) works fine
  • Value is persisted to DB using PackageURL.canonicalize(), which borks the encoding
  • new PackageURL(<value-from-db>) now fails

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,65 @@ void shouldPersistEmptyResultsForUnknownResolver() {
});
}

@Test
void shouldSkipMalformedPurlAndNotReselectIt() {
final String malformedPurl = "pkg:nuget/serilog.sinks.file%A%20...5.0.0@5.0.0";
final Instant resolvedAt = Instant.now();
mockResolveFnRef.set(purl -> new PackageMetadata("9.9.9", Instant.now(), resolvedAt, null));

var project = new Project();
project.setName("test-project");
project = qm.persist(project);

final var malformedComponent = new Component();
malformedComponent.setProject(project);
malformedComponent.setName("serilog.sinks.file");
malformedComponent.setVersion("5.0.0");
malformedComponent.setPurl(malformedPurl);
qm.persist(malformedComponent);

final var componentFoo = new Component();
componentFoo.setProject(project);
componentFoo.setGroup("org.acme");
componentFoo.setName("foo");
componentFoo.setVersion("1.0");
componentFoo.setPurl("pkg:maven/org.acme/foo@1.0");
qm.persist(componentFoo);

final var fetchActivity = new FetchPackageMetadataResolutionCandidatesActivity(pluginManager);
final List<String> candidatePurls = fetchActivity.execute(null, null)
.getCandidateGroupsList().stream()
.flatMap(group -> group.getPurlsList().stream())
.toList();
assertThat(candidatePurls).contains("pkg:maven/org.acme/foo@1.0")
.doesNotContain(malformedPurl);

// Running workflow again should not reselect the malformed PURL
for (int i = 0; i < 2; i++) {
final UUID runId = workflowTest.getEngine().createRun(
new CreateWorkflowRunRequest<>(ResolvePackageMetadataWorkflow.class));
workflowTest.awaitRunStatus(runId, WorkflowRunStatus.COMPLETED);
}

List<String> metadataPurls = withJdbiHandle(handle -> handle
.createQuery("""
SELECT "PURL"
FROM "PACKAGE_METADATA"
""")
.mapTo(String.class)
.list());
assertThat(metadataPurls).containsExactly("pkg:maven/org.acme/foo");

metadataPurls = withJdbiHandle(handle -> handle
.createQuery("""
SELECT "PURL"
FROM "PACKAGE_ARTIFACT_METADATA"
""")
.mapTo(String.class)
.list());
assertThat(metadataPurls).containsExactly("pkg:maven/org.acme/foo@1.0");
}

@Test
void shouldPassPriorArtifactMetadataToResolverWhenAvailable() {
useJdbiTransaction(handle -> {
Expand Down
Loading