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

[dwalluck]

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

Fixes #122
Fixes #92

[dwalluck]

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]

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]

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

[dwalluck]

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

[dwalluck]

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]

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/batik-anim@1.9.1?classifier=sources&repositorY_url=repo.spring.io/release",
    "canonical_purl": "pkg:maven/org.apache.xmlgraphics/batik-anim@1.9.1?classifier=sources&repository_url=repo.spring.io/release",

[dwalluck]

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

[dwalluck]

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]

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]

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.

[dwalluck]

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

[dwalluck]

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]

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]

The exact definition of namespace, name, version and subpath are still WIP: Core PURL spec

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

[dwalluck]

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]

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]

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

[ppkarwasz]

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

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

[dwalluck]

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]

git checkout origin/master -- pom.xml

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

[ppkarwasz]

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:

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

[ppkarwasz]

This is blocked by package-url/purl-spec#438

[ppkarwasz]

While PR package-url/purl-spec#461 haven't been merged yet, it is only receiving changes in the wording, not the rules.

I think it is pretty safe to assume that the following bytes must be left as-is:

• alphanumeric a through z, A through Z, and 0 through 9.
• the five bytes: ., -, _, ~ and :.

So I think the only change we need to apply is to add : to StringUtil.UNRESERVED_CHARS (1 LoC). What do you think?

[jeremylong]

@ppkarwasz I agree - we need to add : to the StringUtil.UNRESERVED_CHARS.