fix: don't encode ':' or '/' as part of the canonical representation #161

dwalluck · 2025-02-20T21:37:54Z

This makes the Java canonical representation match the majority of other implementations.

Fixes #122
Fixes #92

dwalluck · 2025-02-27T18:23:43Z

Note that this matches the current test suite data for '/':

https://github.com/package-url/purl-spec/blob/8040ff0be50f0c5b1986b1a0947bd539f5405fc4/test-suite-data.json#L112-L113

But ':' seems inconsistent. Here's a case of it used as a key value, first encoded, then unencoded:

https://github.com/package-url/purl-spec/blob/8040ff0be50f0c5b1986b1a0947bd539f5405fc4/test-suite-data.json#L88-L89
https://github.com/package-url/purl-spec/blob/8040ff0be50f0c5b1986b1a0947bd539f5405fc4/test-suite-data.json#L484-L485

jeremylong · 2025-02-27T22:40:20Z

The line 112-113 where you say the : is encode - I don't think it is. The URL just doesn't have a schema; just a host and path: repository_url=repo.spring.io/release

dwalluck · 2025-02-27T22:57:59Z

Let me run the full test suite again without the ':' and see if it fails.

dwalluck · 2025-02-27T23:00:42Z

OK, the first line has is for ':', the second line is for '/' ("%2F") (not ':').

dwalluck · 2025-02-27T23:04:06Z

Checking the latest test suite file, if you don't encode the colon, if fails. The file seems to be inconsistent. It's encoded in one spot and not in the other.

org.junit.ComparisonFailure: 
Expected :pkg:huggingface/microsoft/deberta-v3-base@559062ad13d311b87b2c455e67dcd5f1c8f65111?repository_url=https://hub-ci.huggingface.co
Actual   :pkg:huggingface/microsoft/deberta-v3-base@559062ad13d311b87b2c455e67dcd5f1c8f65111?repository_url=https%3A//hub-ci.huggingface.co

jeremylong · 2025-02-27T23:06:12Z

I was talking about the one you mentioned above - there doesn't seem to be any encoding.

https://github.com/package-url/purl-spec/blob/8040ff0be50f0c5b1986b1a0947bd539f5405fc4/test-suite-data.json#L112-L113

    "purl": "pkg:Maven/org.apache.xmlgraphics/[email protected]?classifier=sources&repositorY_url=repo.spring.io/release",
    "canonical_purl": "pkg:maven/org.apache.xmlgraphics/[email protected]?classifier=sources&repository_url=repo.spring.io/release",

dwalluck · 2025-02-27T23:07:06Z

There is no encoding in the input. However, the output without the patch is "%2F", not '/', I think.

dwalluck · 2025-02-27T23:09:37Z

And I found the inconsistency in the test file: the colon is encoded when it's in the version (sha256%3A). It's not encoded when it's in a key value (repository_url=https:).

This is not right, is it? I am trying to verify if the problem is the code or the test file.

dwalluck · 2025-02-27T23:16:21Z

The line 112-113 where you say the : is encode - I don't think it is. The URL just doesn't have a schema; just a host and path: repository_url=repo.spring.io/release

Sorry, I flipped the examples! L112-113 is for '/', and L88-89 is for ':'. I hope it makes sense now. I updated the original comment.

ppkarwasz · 2025-02-28T11:05:05Z

src/main/java/com/github/packageurl/PackageURL.java

@@ -460,7 +460,7 @@ private static String uriEncode(String source, Charset charset) {
    }

    private static boolean isUnreserved(int c) {
-        return (isAlpha(c) || isDigit(c) || '-' == c || '.' == c || '_' == c || '~' == c);
+        return (isAlpha(c) || isDigit(c) || '-' == c || '.' == c || '_' == c || '~' == c || ':' == c || '/' == c);


There should be rather a different set for each PURL component:

namespace does not need to encode neither : nor /.

name does not need to encode :, but it needs to encode /, which is used to separate it from namespace.

qualifiers, probably does not need to encode :, / or ?.

subpath can probably add & and = to the mix.

I will add a param to the percentEncode to take a String of characters to allow.

Really, there are a lot more characters potentially allowed by the RFC than we list here.

I am of the opinion right now to add just enough in order to pass the test suite.

The '?' is not allowed unencoded per the purl spec itself.

dwalluck · 2025-02-28T16:52:18Z

So, with 8ab171f, this now fixes #122, but not #92. The test suite has "%3A", but all examples given in https://github.com/package-url/purl-spec/blob/8040ff0be50f0c5b1986b1a0947bd539f5405fc4/README.rst?plain=1#L116-L117 say otherwise.

Should we just change the test suite back to how to was and assume that there is a mistake in the tests and that the current README.rst is the correct one?

ppkarwasz · 2025-03-02T21:24:38Z

The exact definition of namespace, name, version and subpath are still WIP: https://github.com/orgs/package-url/projects/1

Maybe this PR is precocious and we should wait for the spec to be finalized.

dwalluck · 2025-03-03T18:10:06Z

The author of the Rust version has proposed the set of characters to use https://github.com/phylum-dev/purl/blob/151168733f75a9802556e4b07eb577b9d99f7cea/purl/src/format.rs#L9-L27

I could take them from here.

ppkarwasz · 2025-03-04T21:17:48Z

The author of the Rust version has proposed the set of characters to use https://github.com/phylum-dev/purl/blob/151168733f75a9802556e4b07eb577b9d99f7cea/purl/src/format.rs#L9-L27

I could take them from here.

That code is based on the WHATWG URL standard, while, unless I am mistaken, PURL will be based on RFC 3986. The living standard seems more liberal, so I would base the code on the more conservative RFC 3986. In particular:

The RFC 3986 says that the space character, ", %, <, >, \, the backtick character, {, | and } can never occur in an URI and must be percent-encoded. Obviously percent % also needs to be encoded.
WHATWG URL allows some of these characters to be unencoded, but it is probably safer to encode them.

dwalluck · 2025-03-11T16:19:50Z

I will revisit this later. It may need to be combined with #174.

ppkarwasz · 2025-03-19T11:16:31Z

Now that Spotless is there, there will be some conflicts. This should help:

git checkout origin/master -- pom.xml
mvn spotless:apply

dwalluck · 2025-03-19T13:08:53Z

git checkout origin/master -- pom.xml
mvn spotless:apply

OK, it does apply on build, but if you have to fix the conflicts first, then it doesn't really help.

dwalluck · 2025-03-19T13:09:42Z

git checkout origin/master -- pom.xml

Now you just need to run sortpom:sort on that 😉

ppkarwasz · 2025-03-19T22:00:59Z

git checkout origin/master -- pom.xml
mvn spotless:apply

OK, it does apply on build, but if you have to fix the conflicts first, then it doesn't really help.

You can:

Checkout pom.xml from master to have the Spotless configuration.
Format your code.
Merge the master branch into your PR branch. 99% of the code should be identical and identically formatted, so few conflicts should occur.

This makes the Java canonical representation match the majority of other implementations. Fixes package-url#122 Fixes package-url#92

ppkarwasz reviewed Feb 28, 2025

View reviewed changes

dwalluck marked this pull request as draft March 11, 2025 16:19

dwalluck force-pushed the GH-122 branch from 52dd44f to 21df2bf Compare March 18, 2025 18:14

dwalluck changed the title ~~Don't encode ':' or '/' as part of the canonical representation~~ fix: don't encode ':' or '/' as part of the canonical representation Mar 18, 2025

dwalluck force-pushed the GH-122 branch 3 times, most recently from e42515b to 447ba2a Compare March 18, 2025 21:48

dwalluck force-pushed the GH-122 branch from 447ba2a to 81dfd23 Compare March 19, 2025 12:53

dwalluck force-pushed the GH-122 branch 2 times, most recently from 7803217 to b6482d2 Compare March 19, 2025 13:33

dwalluck force-pushed the GH-122 branch from b6482d2 to 6aeea9b Compare March 20, 2025 19:20

fix: don't encode ':' or '/' as part of the canonical representation

9ee9198

This makes the Java canonical representation match the majority of other implementations. Fixes package-url#122 Fixes package-url#92

dwalluck force-pushed the GH-122 branch from 6aeea9b to 9ee9198 Compare March 21, 2025 16:12

Merge remote-tracking branch 'upstream/master' into package-urlGH-122

311122d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: don't encode ':' or '/' as part of the canonical representation #161

fix: don't encode ':' or '/' as part of the canonical representation #161

dwalluck commented Feb 20, 2025

dwalluck commented Feb 27, 2025 •

edited

Loading

jeremylong commented Feb 27, 2025

dwalluck commented Feb 27, 2025

dwalluck commented Feb 27, 2025

dwalluck commented Feb 27, 2025

jeremylong commented Feb 27, 2025

dwalluck commented Feb 27, 2025

dwalluck commented Feb 27, 2025 •

edited

Loading

dwalluck commented Feb 27, 2025 •

edited

Loading

ppkarwasz Feb 28, 2025 •

edited

Loading

dwalluck Feb 28, 2025

dwalluck Feb 28, 2025

dwalluck commented Feb 28, 2025

ppkarwasz commented Mar 2, 2025

dwalluck commented Mar 3, 2025

ppkarwasz commented Mar 4, 2025

dwalluck commented Mar 11, 2025

ppkarwasz commented Mar 19, 2025

dwalluck commented Mar 19, 2025

dwalluck commented Mar 19, 2025

ppkarwasz commented Mar 19, 2025

fix: don't encode ':' or '/' as part of the canonical representation #161

Are you sure you want to change the base?

fix: don't encode ':' or '/' as part of the canonical representation #161

Conversation

dwalluck commented Feb 20, 2025

dwalluck commented Feb 27, 2025 • edited Loading

jeremylong commented Feb 27, 2025

dwalluck commented Feb 27, 2025

dwalluck commented Feb 27, 2025

dwalluck commented Feb 27, 2025

jeremylong commented Feb 27, 2025

dwalluck commented Feb 27, 2025

dwalluck commented Feb 27, 2025 • edited Loading

dwalluck commented Feb 27, 2025 • edited Loading

ppkarwasz Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

dwalluck Feb 28, 2025

Choose a reason for hiding this comment

dwalluck Feb 28, 2025

Choose a reason for hiding this comment

dwalluck commented Feb 28, 2025

ppkarwasz commented Mar 2, 2025

dwalluck commented Mar 3, 2025

ppkarwasz commented Mar 4, 2025

dwalluck commented Mar 11, 2025

ppkarwasz commented Mar 19, 2025

dwalluck commented Mar 19, 2025

dwalluck commented Mar 19, 2025

ppkarwasz commented Mar 19, 2025

dwalluck commented Feb 27, 2025 •

edited

Loading

dwalluck commented Feb 27, 2025 •

edited

Loading

dwalluck commented Feb 27, 2025 •

edited

Loading

ppkarwasz Feb 28, 2025 •

edited

Loading