Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make PackageURL.ToString() deterministic #7

Closed
wants to merge 0 commits into from

Conversation

Projects
None yet
3 participants
@petergardfjall
Copy link
Contributor

commented Apr 1, 2019

This commit offers a fix for #5 and #6.

A parsed PackageURL should preserve the order of qualifiers (as in the parsed
URL) when output at a later time via ToString(). To do this, Qualifiers cannot
make use of a regular map (keys are unordered).

type Qualifiers map[string]string

This commit changes the type of Qualifiers into

type Qualifiers []Qualifier

type Qualifier struct {
    Key   string
    Value string
}

to preserve qualifier ordering.

The tests are also updated to ensure that qualifiers are read in
declaration-order from test-suite-data.json.

PackageURL is also made to implement the Stringer interface, which is idiomatic
Go.

@furuholm

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@ashcrow

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

This is a great change! It does look like one of the tests is failing though. If possible PTAL at the failing unit test in Travis and update either the code or test. If you don't have the time I'm happy to update the tests/code in a follow up PR.

@ashcrow ashcrow self-assigned this May 14, 2019

@petergardfjall

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@ashcrow Thanks for taking the time to look at the PR. I seem to remember that a few of the tests were failing even before I added this fix. So I decided not to broaden the scope of the PR to address that. But I'm not sure if my memory serves me well on this one. I can have another look tomorrow.

@petergardfjall

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@ashcrow I just ran the tests on master and noticed the same tests fail there.

The same problems can be observed in the travis builds: https://travis-ci.org/package-url/packageurl-go/jobs/514173724

go test -v -cover ./...
=== RUN   TestFromStringExamples
--- FAIL: TestFromStringExamples (0.00s)
	packageurl_test.go:72: checks for invalid qualifier keys did not fail and returned packageurl.PackageURL{Type:"npm", Namespace:"", Name:"myartifact", Version:"1.0.0", Qualifiers:packageurl.Qualifiers{"in%20production":"true"}, Subpath:""}
=== RUN   TestToStringExamples
--- FAIL: TestToStringExamples (0.00s)
	packageurl_test.go:111: valid maven purl containing a space in the version and qualifier failed: pkg:maven/mygroup/myartifact@1.0.0 Final?mykey=my value != pkg:maven/mygroup/myartifact@1.0.0%20Final?mykey=my%20value
FAIL
coverage: 94.9% of statements
exit status 1
FAIL	github.com/package-url/packageurl-go	0.003s
make: *** [test] Error 1
The command "make test" exited with 2.```
@petergardfjall

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@ashcrow Have you had the chance to look into this?

@petergardfjall

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@ashcrow I've made an attempt to fix the broken tests on master. Please review #12

@petergardfjall petergardfjall force-pushed the petergardfjall:master branch from a3fa35d to 79c5c52 May 20, 2019

@petergardfjall

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

I messed up my branches in a big way. So I reopened a PR to replace this one. #13. Sorry about the confusion..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.