Skip to content

fix: discard dot segments from subpath - #176

Open
dwalluck wants to merge 7 commits into
package-url:masterfrom
dwalluck:fix-parse-path
Open

fix: discard dot segments from subpath#176
dwalluck wants to merge 7 commits into
package-url:masterfrom
dwalluck:fix-parse-path

Conversation

@dwalluck

@dwalluckdwalluck commented Mar 4, 2025

Copy link
Copy Markdown
Contributor

This change makes the two test pass that were introduced in package-url/purl-spec#368.

See also package-url/purl-spec#404.

ppkarwasz
ppkarwasz requested changes Mar 5, 2025
View reviewed changes

@ppkarwaszppkarwasz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My 2 cents:

Comment threadsrc/main/java/com/github/packageurl/PackageURL.java Outdated
Comment threadsrc/main/java/com/github/packageurl/PackageURL.java Outdated
Comment threadsrc/main/java/com/github/packageurl/PackageURL.java Outdated
Comment threadsrc/main/java/com/github/packageurl/PackageURL.java Outdated
Comment threadsrc/main/java/com/github/packageurl/PackageURL.java Outdated
Comment threadsrc/test/java/com/github/packageurl/PackageURLTest.java Outdated
@dwalluck
dwalluckforce-pushed the fix-parse-path branch 3 times, most recently from 6cb7290 to 8a03dd6CompareMarch 17, 2025 15:27
@dwalluckdwalluck mentioned this pull request Mar 17, 2025
Open
dwalluck
dwalluck commented Mar 17, 2025
View reviewed changes
Comment threadsrc/main/java/com/github/packageurl/PackageURL.java Outdated
dwalluck
dwalluck commented Mar 17, 2025
View reviewed changes
Comment threadsrc/main/java/com/github/packageurl/PackageURL.java
Comment on lines +434 to +464
if (".".equals(segment) || "..".equals(segment)) {
if (!isSubpath) {
throw new MalformedPackageURLException("Segments in the namespace must not be a period ('.') or repeated period ('..'): '" + segment + "'");
}

@dwalluckdwalluckMar 17, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I can't actually find any mention of namespace not containing dots, but I left the code as it was.

@ppkarwaszppkarwaszMar 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't the condition inverted? Segments in the subpath can not be neither . nor .., but no such condition exists for namespace.

@dwalluckdwalluckMar 18, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I agree that it looks inverted compared to the original code, but I think it is the behavior intended by the test cases:

  • If !subpath, i.e., namespace, then if . or .. appears it's an error.
  • If subpath, then discard it without error and don't add it to the results.

@dwalluckdwalluckMar 18, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think it's confusing because there are two methods, parsePath and validatePath, and they don't share the same code.

One is pre-decoding and silently filters paths there, and one is post-decoding. Probably, there is some way to refactor it to make this clearer, but I haven't figured it out yet.

@dwalluckdwalluckMar 18, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I added a test case. Does it make sense?

@dwalluck
dwalluckforce-pushed the fix-parse-path branch 4 times, most recently from 6fc05ab to db191adCompareMarch 17, 2025 15:54
@dwalluckdwalluck changed the title Silently drop empty and dot segments for subpathDiscard dot segments from subpathMar 17, 2025
@dwalluck
dwalluckforce-pushed the fix-parse-path branch 3 times, most recently from 2fa4d68 to c4171eaCompareMarch 17, 2025 16:39
ppkarwasz
ppkarwasz requested changes Mar 17, 2025
View reviewed changes
Comment threadsrc/main/java/com/github/packageurl/PackageURL.java
Comment on lines +434 to +464
if (".".equals(segment) || "..".equals(segment)) {
if (!isSubpath) {
throw new MalformedPackageURLException("Segments in the namespace must not be a period ('.') or repeated period ('..'): '" + segment + "'");
}

@ppkarwaszppkarwaszMar 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't the condition inverted? Segments in the subpath can not be neither . nor .., but no such condition exists for namespace.

Comment threadsrc/test/java/com/github/packageurl/PackageURLTest.java Outdated
Comment threadsrc/test/java/com/github/packageurl/PackageURLTest.java Outdated
Comment on lines +176 to +177
assertEquals(version, purl.getVersion());
assertEquals(qualifiers, purl.getQualifiers());
assertEquals(subpath, purl.getSubpath());
//assertEquals(subpath, purl.getSubpath());

@ppkarwaszppkarwaszMar 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above.

@dwalluckdwalluckMar 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Actually, I was told that we were using the test suite incorrectly, and that we shouldn't compare the fields at all.

After berating me for even pointing out that there was an issue with the tests because these two tests introduced a different representation of the fields (original instead of canonical), they finally settled on changing the test suite so that all fields were original. At least there's an open pull request for that now.

So, either we'll end up having to remove everything except string comparison in our tests, or they'll finally settle on something. I am sort of against fixing the test suite locally in our copy, though, because we don't even know their intention.

@dwalluckdwalluckMar 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Actually, it was @jeremylong that submitted these tests upstream, so we can find out exactly what was intended with the fields. Was it a mistake or not? The tests don't pass with the current Java code and what do we do now about the fact that even after the "fix" in this pull request the Java code is failing the subpath comparison.

@jeremylongjeremylongMar 22, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Before continuing this one we may want to wait for the spec to be updated a bit - per comments like: package-url/purl-spec#409 (comment)

I know at least one of my changes to the test case is being reverted due to a misunderstanding/wording of the spec.

@dwalluckdwalluck changed the title Discard dot segments from subpathfix: discard dot segments from subpathMar 18, 2025
@ppkarwasz

ppkarwasz commented Mar 19, 2025

Copy link
Copy Markdown
Contributor

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

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

@dwalluck
fix: discard dot segments from subpath
d57cecb
This change makes the two test pass that were introduced in
package-url/purl-spec#368.
See also package-url/purl-spec#404.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeremylongjeremylongjeremylong left review comments
@ppkarwaszppkarwaszppkarwasz requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dwalluck@ppkarwasz@jeremylong