update code to honor the canonical purl encoding #83

petergardfjall · 2025-08-27T12:28:47Z

This PR is an attempt to make the code honor the spec rules on producing a canonical purl when a PackageURL is turned into its string representation (ToString() and String()).

The old test suite (test-suite-data.json) is no longer present upstream. For now I updated the Makefile to run against the last commit where test-suite-data.json was still present in the repo (it was removed in 8a699a).

This should be removed as soon as we can migrate to the new test suite (see separate PR).

It fails on the following tests:

Two tests with invalid subpaths: we fail to parse these, which used to be correct before all of a sudden is_invalid for these cases was swapped from true to false in this commit without much of an explanation. These test cases appear to have been removed from the new test set though.

For the new test suite (a pull request to make use of it is up here) it currently fails on 45 and passes on 468 test cases. For the failed ones:

For two test cases for pkg:conan I cannot figure out why we are failing. I've opened a ticket since I suspect that the test cases might be incorrect purl-spec#643.
Of the remaining errors almost all are ones where the expected output in the test case is not a canonical purl/does not follow purl encoding guidelines. I've opened purl-spec#644 to better understand what's expected from library implementations.

petergardfjall · 2025-08-27T12:29:36Z

Makefile


 test:
-	curl -Ls https://raw.githubusercontent.com/package-url/purl-spec/master/test-suite-data.json -o testdata/test-suite-data.json
+	curl -Ls https://raw.githubusercontent.com/package-url/purl-spec/de752b1160c7dc309482aaa80fd31884005aaec5/test-suite-data.json -o testdata/test-suite-data.json


This should be entirely replaced if/when #80 is merged.

coveralls · 2025-08-27T12:30:31Z

Pull Request Test Coverage Report for Build 17266694404

Details

35 of 36 (97.22%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+3.6%) to 85.761%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packageurl.go	35	36	97.22%

Totals
Change from base Build 13610888770:	3.6%
Covered Lines:	265
Relevant Lines:	309

💛 - Coveralls

petergardfjall · 2025-08-27T12:32:02Z

packageurl.go

-	for _, qq := range q {
-		v.Add(qq.Key, qq.Value)
-	}
-	return v.Encode()


We cannot use Values.Encode() since it uses https://pkg.go.dev/net/url#QueryEscape under the hood to encode the value. This works in many cases but has caveats (does not work according to spec for whitespace and colon :).

petergardfjall · 2025-08-27T12:36:37Z

packageurl.go

 	u := &url.URL{
 		Scheme:   "pkg",
-		RawQuery: p.Qualifiers.urlQuery(),
-		Fragment: p.Subpath,


Note: we cannot use url.Fragment since url.String will call u.EscapedFragment() and we end up with an incorrect escaping (similar to url.QueryEscape I believe).

Instead we "manually" add any fragment part at the end.

petergardfjall · 2025-08-27T12:37:47Z

packageurl.go

+		qualifierKeys := slices.Collect(maps.Keys(p.Qualifiers.Map()))
+		hasNamespace := p.Namespace != ""
+		hasChannel := slices.Contains(qualifierKeys, "channel")
+		if hasNamespace && !hasChannel {
+			return errors.New("channel qualifier is required if namespace is present")
+		}
+		if hasChannel && !hasNamespace {
+			return errors.New("namespace is required if channel qualifier is present")


An attempt to make the code more readable.

petergardfjall · 2025-08-27T12:38:36Z

qualifier_test.go

+import "testing"
+
+// Verifies that qualifier values are properly percent-encoded.
+func TestQualifierValueEncoding(t *testing.T) {


Added some explicit testing of qualifier encoding (outside of purl-spec test suite) since this was a problematic spot which had poor coverage in the upstream tests.

petergardfjall · 2025-10-22T07:42:20Z

@shibumi @mcombuechen ?
Would anyone care to have a look?

petergardfjall · 2025-11-04T12:54:39Z

Barring some outstanding issues with the purl spec test suite (see below) this code now passes the entire purl-spec suite. We should first merge #80 which changes the code to run against the new test suite in the purl-spec repo. When that is merged we can merge this PR.

Until these issues/PRs are fixed the code in this PR will fail to complete the entire test suite (it will fail on about 10 out of 500).

pkg:cran testcases require presence of version despite it being optional in the spec purl-spec#736
- remove 'invalid cran purl without version' testcases purl-spec#735
Fix Conan Tests purl-spec#731
- remove 'invalid conan purl only namespace' testcases purl-spec#733
- remove 'invalid conan purl only channel qualifier' testcases purl-spec#734
Fix Bazel tests purl-spec#729
- Expected output in testcases do not follow spec encoding purl-spec#644 (comment)

petergardfjall · 2025-11-04T12:56:45Z

Makefile


 test:
-	curl -Ls https://raw.githubusercontent.com/package-url/purl-spec/master/test-suite-data.json -o testdata/test-suite-data.json
 	go test -v -cover ./...


Note: relies on #80 being merged.

petergardfjall · 2025-11-04T12:57:37Z

packageurl.go

 func validCustomRules(p PackageURL) error {
-	q := p.Qualifiers.Map()
 	switch p.Type {
-	case TypeConan:


The conan spec has been relaxed.

petergardfjall · 2025-11-04T12:58:11Z

packageurl.go

-				return errors.New("channel qualifier does not exist")
-			}
-		} else {
-			if val, ok := q["channel"]; ok {


The conan spec has been relaxed.

petergardfjall · 2025-11-04T12:58:36Z

packageurl.go

 		if p.Version == "" {
 			return errors.New("version is required")
 		}
-	case TypeCran:


See package-url/purl-spec#735

petergardfjall · 2025-11-04T12:59:25Z

packageurl_test.go

@@ -1,572 +0,0 @@
-/*


This file should be replaced with packageurl_test.go from #80

petergardfjall · 2025-11-04T13:00:06Z

Anyone up for reviewing?

petergardfjall added 7 commits August 27, 2025 13:35

fix qualifier value encoding

f9a066c

A version MUST be a percent-encoded string

e17e9b4

fix percent-encoding of purl components

b81354e

pkg:conan validation (still breaking tests)

5e2fa7b

percent-encode subpath

da6a3c8

Makefile: run against last version of the old test-suite-data.json

622fe9c

godoc updates

2b74080

petergardfjall commented Aug 27, 2025

View reviewed changes

petergardfjall mentioned this pull request Aug 27, 2025

Run tests using reference data from purl-spec #80

Open

petergardfjall added 4 commits October 24, 2025 15:42

remove old tests: packageurl_test.go

03bcbd9

adapt to new cpan spec wording

c71b70c

simplify pkg:conan validation

1465897

julia-specific validation

be4d927

cleanup Makefile

36f0afa

petergardfjall commented Nov 4, 2025

View reviewed changes

packageurl_test.go

@@ -1,572 +0,0 @@

/*

Copy link

Contributor Author

petergardfjall Nov 4, 2025

Choose a reason for hiding this comment

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

This file should be replaced with packageurl_test.go from #80

update code to honor the canonical purl encoding #83

Are you sure you want to change the base?

update code to honor the canonical purl encoding #83

Conversation

petergardfjall commented Aug 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 27, 2025

Pull Request Test Coverage Report for Build 17266694404

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petergardfjall Aug 27, 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!

petergardfjall commented Oct 22, 2025

Uh oh!

petergardfjall commented Nov 4, 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!

petergardfjall commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

petergardfjall Aug 27, 2025 •

edited

Loading