From f37b87a791ce36b0bdfe299a929ed770edf710a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ramon=20R=C3=BCttimann?= Date: Tue, 18 Jul 2023 10:47:01 +0200 Subject: [PATCH] fix: use url.Values for Qualifiers BREAKING CHANGE: This commit removes all the custom qualifier-logic that existed in order to keep the ordering of the qualifiers. The spec says: > sort this list of qualifier strings lexicographically However, it doesn't say anything about the ordering within the typed representation. Using `url.Values` through a type alias gives users an easier way to access specific values and removes code that we now don't need to maintain anymore. See #53 for more information. --- packageurl.go | 191 ++++++++++++++------------------------------- packageurl_test.go | 136 +++----------------------------- 2 files changed, 73 insertions(+), 254 deletions(-) diff --git a/packageurl.go b/packageurl.go index ebef512..e208ea0 100644 --- a/packageurl.go +++ b/packageurl.go @@ -29,7 +29,6 @@ import ( "net/url" "path" "regexp" - "sort" "strings" ) @@ -110,66 +109,7 @@ var ( TypeSwift = "swift" ) -// Qualifier represents a single key=value qualifier in the package url -type Qualifier struct { - Key string - Value string -} - -func (q Qualifier) String() string { - // A value must be a percent-encoded string - return fmt.Sprintf("%s=%s", q.Key, url.PathEscape(q.Value)) -} - -// Qualifiers is a slice of key=value pairs, with order preserved as it appears -// in the package URL. -type Qualifiers []Qualifier - -// urlQuery returns a raw URL query with all the qualifiers as keys + values. -func (q Qualifiers) urlQuery() (rawQuery string) { - v := make(url.Values) - for _, qq := range q { - v.Add(qq.Key, qq.Value) - } - return v.Encode() -} - -// QualifiersFromMap constructs a Qualifiers slice from a string map. To get a -// deterministic qualifier order (despite maps not providing any iteration order -// guarantees) the returned Qualifiers are sorted in increasing order of key. -func QualifiersFromMap(mm map[string]string) Qualifiers { - q := Qualifiers{} - - for k, v := range mm { - q = append(q, Qualifier{Key: k, Value: v}) - } - - // sort for deterministic qualifier order - sort.Slice(q, func(i int, j int) bool { return q[i].Key < q[j].Key }) - - return q -} - -// Map converts a Qualifiers struct to a string map. -func (qq Qualifiers) Map() map[string]string { - m := make(map[string]string) - - for i := 0; i < len(qq); i++ { - k := qq[i].Key - v := qq[i].Value - m[k] = v - } - - return m -} - -func (qq Qualifiers) String() string { - var kvPairs []string - for _, q := range qq { - kvPairs = append(kvPairs, q.String()) - } - return strings.Join(kvPairs, "&") -} +type Qualifiers = url.Values // PackageURL is the struct representation of the parts that make a package url type PackageURL struct { @@ -200,7 +140,7 @@ func NewPackageURL(purlType, namespace, name, version string, func (p *PackageURL) ToString() string { u := &url.URL{ Scheme: "pkg", - RawQuery: p.Qualifiers.urlQuery(), + RawQuery: p.Qualifiers.Encode(), Fragment: p.Subpath, } @@ -251,10 +191,11 @@ func FromString(purl string) (PackageURL, error) { } typ = strings.ToLower(typ) - qualifiers, err := parseQualifiers(u.RawQuery) + qualifiers, err := getQualifiers(u.RawQuery) if err != nil { return PackageURL{}, fmt.Errorf("invalid qualifiers: %w", err) } + namespace, name, version, err := separateNamespaceNameVersion(p) if err != nil { return PackageURL{}, err @@ -283,6 +224,33 @@ func escape(s string) string { return strings.ReplaceAll(url.QueryEscape(s), "+", "%20") } +func getQualifiers(rawQuery string) (url.Values, error) { + qualifiers, err := url.ParseQuery(rawQuery) + if err != nil { + return nil, fmt.Errorf("could not parse qualifiers: %w", err) + } + + for k := range qualifiers { + if !validQualifierKey(k) { + return nil, fmt.Errorf("invalid qualifier key: %q", k) + } + + v := qualifiers.Get(k) + // only the first character needs to be lowercased. Note that pURL is alwyas UTF8, so we + // don't need to care about unicode here. + normalisedValue := strings.ToLower(v[:1]) + v[1:] + + if normalisedKey := strings.ToLower(k); normalisedKey != k { + qualifiers.Del(k) + qualifiers.Set(normalisedKey, normalisedValue) + } else if normalisedValue != v { + qualifiers.Set(k, normalisedValue) + } + } + + return qualifiers, nil +} + func separateNamespaceNameVersion(path string) (ns, name, version string, err error) { name = path @@ -316,46 +284,6 @@ func separateNamespaceNameVersion(path string) (ns, name, version string, err er return ns, name, version, nil } -func parseQualifiers(rawQuery string) (Qualifiers, error) { - // we need to parse the qualifiers ourselves and cannot rely on the `url.Query` type because - // that uses a map, meaning it's unordered. We want to keep the order of the qualifiers, so this - // function re-implements the `url.parseQuery` function based on our `Qualifier` type. Most of - // the code here is taken from `url.parseQuery`. - q := Qualifiers{} - for rawQuery != "" { - var key string - key, rawQuery, _ = strings.Cut(rawQuery, "&") - if strings.Contains(key, ";") { - return nil, fmt.Errorf("invalid semicolon separator in query") - } - if key == "" { - continue - } - key, value, _ := strings.Cut(key, "=") - key, err := url.QueryUnescape(key) - if err != nil { - return nil, fmt.Errorf("error unescaping qualifier key %q", key) - } - - if !validQualifierKey(key) { - return nil, fmt.Errorf("invalid qualifier key: '%s'", key) - } - - value, err = url.QueryUnescape(value) - if err != nil { - return nil, fmt.Errorf("error unescaping qualifier value %q", value) - } - - q = append(q, Qualifier{ - Key: strings.ToLower(key), - // only the first character needs to be lowercase. Note that pURL is always UTF8, so we - // don't need to care about unicode here. - Value: strings.ToLower(value[:1]) + value[1:], - }) - } - return q, nil -} - // Make any purl type-specific adjustments to the parsed namespace. // See https://github.com/package-url/purl-spec#known-purl-types func typeAdjustNamespace(purlType, ns string) string { @@ -378,7 +306,6 @@ func typeAdjustNamespace(purlType, ns string) string { // Make any purl type-specific adjustments to the parsed name. // See https://github.com/package-url/purl-spec#known-purl-types func typeAdjustName(purlType, name string, qualifiers Qualifiers) string { - quals := qualifiers.Map() switch purlType { case TypeAlpm, TypeApk, @@ -393,7 +320,7 @@ func typeAdjustName(purlType, name string, qualifiers Qualifiers) string { case TypePyPi: return strings.ToLower(strings.ReplaceAll(name, "_", "-")) case TypeMLFlow: - return adjustMlflowName(name, quals) + return adjustMlflowName(name, qualifiers) } return name } @@ -409,21 +336,23 @@ func typeAdjustVersion(purlType, version string) string { } // https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst#mlflow -func adjustMlflowName(name string, qualifiers map[string]string) string { - if repo, ok := qualifiers["repository_url"]; ok { - if strings.Contains(repo, "azureml") { - // Azure ML is case-sensitive and must be kept as-is - return name - } else if strings.Contains(repo, "databricks") { - // Databricks is case-insensitive and must be lowercased - return strings.ToLower(name) - } else { - // Unknown repository type, keep as-is - return name - } - } else { +func adjustMlflowName(name string, qualifiers Qualifiers) string { + switch v := qualifiers.Get("repository_url"); { + case v == "": // No repository qualifier given, keep as-is return name + + case strings.Contains(v, "azureml"): + // Azure ML is case-sensitive and must be kept as-is + return name + + case strings.Contains(v, "databricks"): + // Databricks is case-insensitive and must be lowercased + return strings.ToLower(name) + + default: + // Unknown repository type, keep as-is + return name } } @@ -435,24 +364,24 @@ func validQualifierKey(key string) bool { // validCustomRules evaluates additional rules for each package url type, as specified in the package-url specification. // On success, it returns nil. On failure, a descriptive error will be returned. func validCustomRules(p PackageURL) error { - q := p.Qualifiers.Map() + q := p.Qualifiers switch p.Type { case TypeConan: - if p.Namespace != "" { - if val, ok := q["channel"]; ok { - if val == "" { - return errors.New("the qualifier channel must be not empty if namespace is present") - } - } else { - return errors.New("channel qualifier does not exist") + switch channelSet, nsSet := q.Has("channel"), p.Namespace != ""; { + case nsSet && channelSet: + if q.Get("channel") == "" { + return errors.New("the qualifier channel must be not empty if namespace is present") } - } else { - if val, ok := q["channel"]; ok { - if val != "" { - return errors.New("namespace is required if channel is non empty") - } + + case nsSet && !channelSet: + return errors.New("channel qualifier does not exist") + + case !nsSet && channelSet: + if q.Get("channel") != "" { + return errors.New("namespace is required if channel is non empty") } } + case TypeSwift: if p.Namespace == "" { return errors.New("namespace is required") diff --git a/packageurl_test.go b/packageurl_test.go index 4423c72..00a3577 100644 --- a/packageurl_test.go +++ b/packageurl_test.go @@ -23,98 +23,31 @@ package packageurl_test import ( "encoding/json" - "fmt" "os" "reflect" - "regexp" - "strings" "testing" "github.com/package-url/packageurl-go" ) type TestFixture struct { - Description string `json:"description"` - Purl string `json:"purl"` - CanonicalPurl string `json:"canonical_purl"` - PackageType string `json:"type"` - Namespace string `json:"namespace"` - Name string `json:"name"` - Version string `json:"version"` - QualifierMap OrderedMap `json:"qualifiers"` - Subpath string `json:"subpath"` - IsInvalid bool `json:"is_invalid"` + Description string `json:"description"` + Purl string `json:"purl"` + CanonicalPurl string `json:"canonical_purl"` + PackageType string `json:"type"` + Namespace string `json:"namespace"` + Name string `json:"name"` + Version string `json:"version"` + QualifierMap map[string]string `json:"qualifiers"` + Subpath string `json:"subpath"` + IsInvalid bool `json:"is_invalid"` } -// OrderedMap is used to store the TestFixture.QualifierMap, to ensure that the -// declaration order of qualifiers is preserved. -type OrderedMap struct { - OrderedKeys []string - Map map[string]string -} - -// qualifiersMapPattern is used to parse the TestFixture "qualifiers" field to -// ensure that it's a json object. -var qualifiersMapPattern = regexp.MustCompile(`(?ms)^\{.*\}$`) - -// UnmarshalJSON unmarshals the qualifiers field for a TestFixture. The -// qualifiers field is given as a json object such as: -// -// "qualifiers": {"arch": "i386", "distro": "fedora-25"} -// -// This function performs in-order parsing of these values into an OrderedMap to -// preserve items in order of declaration. Note that parsing as a -// map[string]string won't preserve element order. -func (m *OrderedMap) UnmarshalJSON(bytes []byte) error { - data := string(bytes) - switch data { - case "null": - m.OrderedKeys = []string{} - m.Map = make(map[string]string) - return nil - default: - // ensure that the data is a json object "{...}" - if !qualifiersMapPattern.MatchString(data) { - return fmt.Errorf("qualifiers parse error: not a json object: %s", data) - } - - // find out the order in which map keys occur - dec := json.NewDecoder(strings.NewReader(data)) - // consume opening '{' - _, _ = dec.Token() - for dec.More() { - t, _ := dec.Token() - switch token := t.(type) { - case json.Delim: - if token != '}' { - return fmt.Errorf("qualifiers parse error: expected delimiter '}', got: %v", token) - } - // closed json object -> we're done - case string: - // this token is a dictionary key - m.OrderedKeys = append(m.OrderedKeys, token) - // consume the value (the token following the colon after the key) - _, _ = dec.Token() - } - } - - // now that we know the key order, just fill the OrderedMap.Map field - if err := json.Unmarshal(bytes, &m.Map); err != nil { - return err - } - return nil - } -} - -// Qualifiers converts the TestFixture.QualifierMap field to an object of type -// packageurl.Qualifiers. func (t TestFixture) Qualifiers() packageurl.Qualifiers { q := packageurl.Qualifiers{} - - for _, key := range t.QualifierMap.OrderedKeys { - q = append(q, packageurl.Qualifier{Key: key, Value: t.QualifierMap.Map[key]}) + for k, v := range t.QualifierMap { + q.Add(k, v) } - return q } @@ -200,8 +133,7 @@ func TestToStringExamples(t *testing.T) { } instance := packageurl.NewPackageURL( tc.PackageType, tc.Namespace, tc.Name, tc.Version, - // Use QualifiersFromMap so that the qualifiers have a defined order, which is needed for string comparisons - packageurl.QualifiersFromMap(tc.Qualifiers().Map()), tc.Subpath) + tc.Qualifiers(), tc.Subpath) result := instance.ToString() // NOTE: We create a purl with ToString and then load into a PackageURL @@ -258,48 +190,6 @@ func TestStringer(t *testing.T) { } } -// Verify correct conversion of Qualifiers to a string map and vice versa. -func TestQualifiersMapConversion(t *testing.T) { - tests := []struct { - kvMap map[string]string - qualifiers packageurl.Qualifiers - }{ - { - kvMap: map[string]string{}, - qualifiers: packageurl.Qualifiers{}, - }, - { - kvMap: map[string]string{"arch": "amd64"}, - qualifiers: packageurl.Qualifiers{ - packageurl.Qualifier{Key: "arch", Value: "amd64"}, - }, - }, - { - kvMap: map[string]string{"arch": "amd64", "os": "linux"}, - qualifiers: packageurl.Qualifiers{ - packageurl.Qualifier{Key: "arch", Value: "amd64"}, - packageurl.Qualifier{Key: "os", Value: "linux"}, - }, - }, - } - - for _, test := range tests { - // map -> Qualifiers - got := packageurl.QualifiersFromMap(test.kvMap) - if !reflect.DeepEqual(got, test.qualifiers) { - t.Logf("map -> qualifiers conversion failed: got: %#v, wanted: %#v", got, test.qualifiers) - t.Fail() - } - - // Qualifiers -> map - mp := test.qualifiers.Map() - if !reflect.DeepEqual(mp, test.kvMap) { - t.Logf("qualifiers -> map conversion failed: got: %#v, wanted: %#v", mp, test.kvMap) - t.Fail() - } - } -} - func TestNameEscaping(t *testing.T) { testCases := map[string]string{ "abc": "pkg:deb/abc",