ni:// URI should not escape commas in query-portion

[sappenin]

Per RFC-3986 (which RFC-6920 delegates to for query-part construction), URI schemes should treat a comma (",") as a reserved character separator in the query-portion of the URI, and should not escape them.

Currently, however, NamedInformationUri.java URL-encodes commas in the query-portion of an ni://-schemed URI when there are multiple instances of a given "key" (e.g., for conditions with multiple subtypes keys).

For example, the following String is what I would expect NamedInformationUri to construct, per RFC-3986 (notice the plain-text commas):

ni:///sha-256?fpt=ed25519-sha-256,prefix-sha-256,preimage-sha-256

However, the following is what is produced currently: ni:///sha-256?fpt=ed25519-sha-256%2Cprefix-sha-256%2Cpreimage-sha-256

While not invalid (RFC-3986 seems to allow commas in the query to be encoded depending on context), the above style moves the meaning of the comma separator into the fpt definition, instead of allowing it to be handled via RFC-3986. Here's a SO answer that examines the usage of commas in URI query portions, especially in light of RFC-3986 (which obsoletes RFC-2396 as referenced by the SO question-submitter).

One final rationale to not escape commas is the result of various Java implementations of URL/URI, which do not escape commas in the query-portion when creating URI strings.

For example, here's what Java does with an http URI:

URI.create("https://example.com?foo=ba%20r,baz").getQuery();  
// Emits: "foo=ba r,baz"

URI.create("https://example.com?foo=ba%20r,baz").getRawQuery();  
// Emits: "foo=ba%20r,baz"

Likewise, Square's OkHttp library has a very nice HttpUrl class (see here for whey they created a new URL class), which emits the following:

HttpUrl.parse("https://example.com?foo=ba%20r,baz").toString();
// Emits: "https://example.com/?foo=ba%20r,baz"

HttpUrl.parse("https://example.com?foo=ba r,baz").toString();
// Emits: "https://example.com/?foo=ba%20r,baz"

Notice things like a "space" are percent-encoded in the query, but commas are not.

[sappenin]

@adrianhopebailie I think this test vector in the Javascript project confirms that this issue is a bug (see the URL query-params for the Threshold condition).

I've opened issue 12 in the Crypto-Condition RFC project to get the spec to clarify this behavior, but for now we should probably maintain compatibility with the JS implementation. If you agree, feel free to approve this PR and I'll merge it in.

[adrianhopebailie]

As discussed on the phone I actually think the JS impl and test vectors are wrong but this is one of those cases where the impl drive the spec so I agree, let's get in sync with the JS and test vectors.

[sappenin]

Cool - can you approve the PR (#51) if it looks ok to you?

[sappenin]

Closing this since #51 is now merged.