devpkg: use struct field values in FlakeRef.String

Improve FlakeRef.String so that it reassembles the flakeref using the
current field values instead of returning the original parsed string
that it came from. This makes it easier to modify parsed flakerefs or to
create new ones from scratch.

The strings are normalized so that when two flakerefs are equal, their
strings will also be equal. The docs for FlakeRef.String go into more
detail on what the normalized form looks like.

Being able to parse/modify/encode flake references is part of the work
to support multiple package outputs. Having a single (well-tested type)
for handling flakes should help manage the complexity as another syntax
for outputs is added.

Author: gcurtis

.golangci.yml

@@ -41,7 +41,7 @@ linters-settings:
       - name: bool-literal-in-expr
       - name: cognitive-complexity
         arguments:
-          - 30 # TODO: gradually reduce it
+          - 32 # TODO: gradually reduce it
       - name: datarace
       - name: duplicated-imports
       - name: early-return

internal/devpkg/flakeref.go

@@ -2,6 +2,7 @@ package devpkg
 
 import (
 	"net/url"
+	"path"
 	"strings"
 
 	"go.jetpack.io/devbox/internal/redact"
@@ -52,9 +53,6 @@ type FlakeRef struct {
 	// or "git". Note that the URL is not the same as the raw unparsed
 	// flakeref.
 	URL string `json:"url,omitempty"`
-
-	// raw stores the original unparsed flakeref string.
-	raw string
 }
 
 // ParseFlakeRef parses a raw flake reference. Nix supports a variety of
@@ -78,7 +76,7 @@ func ParseFlakeRef(ref string) (FlakeRef, error) {
 	}
 
 	// Handle path-style references first.
-	parsed := FlakeRef{raw: ref}
+	parsed := FlakeRef{}
 	if ref[0] == '.' || ref[0] == '/' {
 		if strings.ContainsAny(ref, "?#") {
 			// The Nix CLI does seem to allow paths with a '?'
@@ -99,7 +97,6 @@ func parseFlakeURLRef(ref string) (parsed FlakeRef, fragment string, err error)
 	// A good way to test how Nix parses a flake reference is to run:
 	//
 	// 	nix eval --json --expr 'builtins.parseFlakeRef "ref"' | jq
-	parsed.raw = ref
 	refURL, err := url.Parse(ref)
 	if err != nil {
 		return FlakeRef{}, "", redact.Errorf("parse flake reference as URL: %v", err)
@@ -231,9 +228,111 @@ func parseGitHubFlakeRef(refURL *url.URL, parsed *FlakeRef) error {
 	return nil
 }
 
-// String returns the raw flakeref string as given to ParseFlakeRef.
+// String encodes the flake reference as a URL-like string. It normalizes
+// the result such that if two FlakeRef values are equal, then their strings
+// will also be equal.
+//
+// There are multiple ways to encode a flake reference. String uses the
+// following rules to normalize the result:
+//
+//   - the URL-like form is always used, even for paths and indirects.
+//   - the scheme is always present, even if it's optional.
+//   - paths are cleaned per path.Clean.
+//   - fields that can be put in either the path or the query string are always
+//     put in the path.
+//   - query parameters are sorted by key.
+//
+// If f is missing a type or has any invalid fields, String returns an empty
+// string.
 func (f FlakeRef) String() string {
-	return f.raw
+	switch f.Type {
+	case "file":
+		if f.URL == "" {
+			return ""
+		}
+		return "file+" + f.URL
+	case "git":
+		if f.URL == "" {
+			return ""
+		}
+		if !strings.HasPrefix(f.URL, "git") {
+			f.URL = "git+" + f.URL
+		}
+
+		// Nix removes "ref" and "rev" from the query string
+		// (but not other parameters) after parsing. If they're empty,
+		// we can skip parsing the URL. Otherwise, we need to add them
+		// back.
+		if f.Ref == "" && f.Rev == "" {
+			return f.URL
+		}
+		url, err := url.Parse(f.URL)
+		if err != nil {
+			// This should be rare and only happen if the caller
+			// messed with the parsed URL.
+			return ""
+		}
+		url.RawQuery = buildQueryString("ref", f.Ref, "rev", f.Rev, "dir", f.Dir)
+		return url.String()
+	case "github":
+		if f.Owner == "" || f.Repo == "" {
+			return ""
+		}
+		url := &url.URL{
+			Scheme:   "github",
+			Opaque:   buildOpaquePath(f.Owner, f.Repo, f.Rev, f.Ref),
+			RawQuery: buildQueryString("host", f.Host, "dir", f.Dir),
+		}
+		return url.String()
+	case "indirect":
+		if f.ID == "" {
+			return ""
+		}
+		url := &url.URL{
+			Scheme:   "flake",
+			Opaque:   buildOpaquePath(f.ID, f.Ref, f.Rev),
+			RawQuery: buildQueryString("dir", f.Dir),
+		}
+		return url.String()
+	case "path":
+		if f.Path == "" {
+			return ""
+		}
+		f.Path = path.Clean(f.Path)
+		url := &url.URL{
+			Scheme: "path",
+			Opaque: buildOpaquePath(strings.Split(f.Path, "/")...),
+		}
+
+		// Add the / prefix back if strings.Split removed it.
+		if f.Path[0] == '/' {
+			url.Opaque = "/" + url.Opaque
+		} else if f.Path == "." {
+			url.Opaque = "."
+		}
+		return url.String()
+	case "tarball":
+		if f.URL == "" {
+			return ""
+		}
+		if !strings.HasPrefix(f.URL, "tarball") {
+			f.URL = "tarball+" + f.URL
+		}
+		if f.Dir == "" {
+			return f.URL
+		}
+
+		url, err := url.Parse(f.URL)
+		if err != nil {
+			// This should be rare and only happen if the caller
+			// messed with the parsed URL.
+			return ""
+		}
+		url.RawQuery = buildQueryString("dir", f.Dir)
+		return url.String()
+	default:
+		return ""
+	}
 }
 
 func isGitHash(s string) bool {
@@ -260,6 +359,33 @@ func isArchive(path string) bool {
 		strings.HasSuffix(path, ".zip")
 }
 
+// buildOpaquePath escapes and joins path elements for a URL flakeref. The
+// resulting path is cleaned according to url.JoinPath.
+func buildOpaquePath(elem ...string) string {
+	for i := range elem {
+		elem[i] = url.PathEscape(elem[i])
+	}
+	u := &url.URL{}
+	return u.JoinPath(elem...).String()
+}
+
+// buildQueryString builds a URL query string from a list of key-value string
+// pairs, omitting any keys with empty values.
+func buildQueryString(keyval ...string) string {
+	if len(keyval)%2 != 0 {
+		panic("buildQueryString: odd number of key-value pairs")
+	}
+
+	query := make(url.Values, len(keyval)/2)
+	for i := 0; i < len(keyval); i += 2 {
+		k, v := keyval[i], keyval[i+1]
+		if v != "" {
+			query.Set(k, v)
+		}
+	}
+	return query.Encode()
+}
+
 // FlakeInstallable is a Nix command line argument that specifies how to install
 // a flake. It can be a plain flake reference, or a flake reference with an
 // attribute path and/or output specification.
@@ -309,20 +435,15 @@ func ParseFlakeInstallable(raw string) (FlakeInstallable, error) {
 	// cannot point to files with a '#' or '?' in their name, since those
 	// would be parsed as the URL fragment or query string. This mimic's
 	// Nix's CLI behavior.
-	prefix := ""
 	if raw[0] == '.' || raw[0] == '/' {
-		prefix = "path:"
-		raw = prefix + raw
+		raw = "path:" + raw
 	}
 
 	var err error
 	install.Ref, install.AttrPath, err = parseFlakeURLRef(raw)
 	if err != nil {
 		return FlakeInstallable{}, err
 	}
-	// Make sure to reset the raw flakeref to the original string
-	// after parsing.
-	install.Ref.raw = raw[len(prefix):]
 	return install, nil
 }
 

internal/devpkg/flakeref_test.go

@@ -149,17 +149,81 @@ func TestParseFlakeRef(t *testing.T) {
 	for ref, want := range cases {
 		t.Run(ref, func(t *testing.T) {
 			got, err := ParseFlakeRef(ref)
-			if diff := cmp.Diff(want, got, cmpopts.IgnoreUnexported(FlakeRef{})); diff != "" {
+			if diff := cmp.Diff(want, got); diff != "" {
 				if err != nil {
 					t.Errorf("got error: %s", err)
 				}
 				t.Errorf("wrong flakeref (-want +got):\n%s", diff)
 			}
-			if err != nil {
-				return
-			}
-			if ref != got.String() {
-				t.Errorf("got.String() = %q != %q", got, ref)
+		})
+	}
+}
+
+func TestFlakeRefString(t *testing.T) {
+	cases := map[FlakeRef]string{
+		{}: "",
+
+		// Path references.
+		{Type: "path", Path: "."}:                "path:.",
+		{Type: "path", Path: "./"}:               "path:.",
+		{Type: "path", Path: "./flake"}:          "path:flake",
+		{Type: "path", Path: "./relative/flake"}: "path:relative/flake",
+		{Type: "path", Path: "/"}:                "path:/",
+		{Type: "path", Path: "/flake"}:           "path:/flake",
+		{Type: "path", Path: "/absolute/flake"}:  "path:/absolute/flake",
+
+		// Path references with escapes.
+		{Type: "path", Path: "%"}:                 "path:%25",
+		{Type: "path", Path: "/%2F"}:              "path:/%252F",
+		{Type: "path", Path: "./Ûñî©ôδ€/flake\n"}: "path:%C3%9B%C3%B1%C3%AE%C2%A9%C3%B4%CE%B4%E2%82%AC/flake%0A",
+		{Type: "path", Path: "/Ûñî©ôδ€/flake\n"}:  "path:/%C3%9B%C3%B1%C3%AE%C2%A9%C3%B4%CE%B4%E2%82%AC/flake%0A",
+
+		// Indirect references.
+		{Type: "indirect", ID: "indirect"}:                                                              "flake:indirect",
+		{Type: "indirect", ID: "indirect", Dir: "sub/dir"}:                                              "flake:indirect?dir=sub%2Fdir",
+		{Type: "indirect", ID: "indirect", Ref: "ref"}:                                                  "flake:indirect/ref",
+		{Type: "indirect", ID: "indirect", Ref: "my/ref"}:                                               "flake:indirect/my%2Fref",
+		{Type: "indirect", ID: "indirect", Rev: "5233fd2ba76a3accb5aaa999c00509a11fd0793c"}:             "flake:indirect/5233fd2ba76a3accb5aaa999c00509a11fd0793c",
+		{Type: "indirect", ID: "indirect", Ref: "ref", Rev: "5233fd2ba76a3accb5aaa999c00509a11fd0793c"}: "flake:indirect/ref/5233fd2ba76a3accb5aaa999c00509a11fd0793c",
+
+		// GitHub references.
+		{Type: "github", Owner: "NixOS", Repo: "nix"}:                                                  "github:NixOS/nix",
+		{Type: "github", Owner: "NixOS", Repo: "nix", Ref: "v1.2.3"}:                                   "github:NixOS/nix/v1.2.3",
+		{Type: "github", Owner: "NixOS", Repo: "nix", Ref: "my/ref"}:                                   "github:NixOS/nix/my%2Fref",
+		{Type: "github", Owner: "NixOS", Repo: "nix", Ref: "5233fd2ba76a3accb5aaa999c00509a11fd0793c"}: "github:NixOS/nix/5233fd2ba76a3accb5aaa999c00509a11fd0793c",
+		{Type: "github", Owner: "NixOS", Repo: "nix", Ref: "5233fd2bb76a3accb5aaa999c00509a11fd0793z"}: "github:NixOS/nix/5233fd2bb76a3accb5aaa999c00509a11fd0793z",
+		{Type: "github", Owner: "NixOS", Repo: "nix", Dir: "sub/dir"}:                                  "github:NixOS/nix?dir=sub%2Fdir",
+		{Type: "github", Owner: "NixOS", Repo: "nix", Dir: "sub/dir", Host: "example.com"}:             "github:NixOS/nix?dir=sub%2Fdir&host=example.com",
+
+		// Git references.
+		{Type: "git", URL: "git://example.com/repo/flake"}:     "git://example.com/repo/flake",
+		{Type: "git", URL: "https://example.com/repo/flake"}:   "git+https://example.com/repo/flake",
+		{Type: "git", URL: "ssh://[email protected]/repo/flake"}: "git+ssh://[email protected]/repo/flake",
+		{Type: "git", URL: "git:/repo/flake"}:                  "git:/repo/flake",
+		{Type: "git", URL: "file:///repo/flake"}:               "git+file:///repo/flake",
+		{Type: "git", URL: "ssh://[email protected]/repo/flake", Ref: "my/ref", Rev: "e486d8d40e626a20e06d792db8cc5ac5aba9a5b4"}:                               "git+ssh://[email protected]/repo/flake?ref=my%2Fref&rev=e486d8d40e626a20e06d792db8cc5ac5aba9a5b4",
+		{Type: "git", URL: "ssh://[email protected]/repo/flake?dir=sub%2Fdir", Ref: "my/ref", Rev: "e486d8d40e626a20e06d792db8cc5ac5aba9a5b4", Dir: "sub/dir"}: "git+ssh://[email protected]/repo/flake?dir=sub%2Fdir&ref=my%2Fref&rev=e486d8d40e626a20e06d792db8cc5ac5aba9a5b4",
+		{Type: "git", URL: "git:repo/flake?dir=sub%2Fdir", Ref: "my/ref", Rev: "e486d8d40e626a20e06d792db8cc5ac5aba9a5b4", Dir: "sub/dir"}:                   "git:repo/flake?dir=sub%2Fdir&ref=my%2Fref&rev=e486d8d40e626a20e06d792db8cc5ac5aba9a5b4",
+
+		// Tarball references.
+		{Type: "tarball", URL: "http://example.com/flake"}:                  "tarball+http://example.com/flake",
+		{Type: "tarball", URL: "https://example.com/flake"}:                 "tarball+https://example.com/flake",
+		{Type: "tarball", URL: "https://example.com/flake", Dir: "sub/dir"}: "tarball+https://example.com/flake?dir=sub%2Fdir",
+		{Type: "tarball", URL: "file:///home/flake"}:                        "tarball+file:///home/flake",
+
+		// File URL references.
+		{Type: "file", URL: "file:///flake"}:                                              "file+file:///flake",
+		{Type: "file", URL: "http://example.com/flake"}:                                   "file+http://example.com/flake",
+		{Type: "file", URL: "http://example.com/flake.git"}:                               "file+http://example.com/flake.git",
+		{Type: "file", URL: "http://example.com/flake.tar?dir=sub%2Fdir", Dir: "sub/dir"}: "file+http://example.com/flake.tar?dir=sub%2Fdir",
+	}
+
+	for ref, want := range cases {
+		t.Run(want, func(t *testing.T) {
+			t.Logf("input = %#v", ref)
+			got := ref.String()
+			if got != want {
+				t.Errorf("got %#q, want %#q", got, want)
 			}
 		})
 	}
@@ -253,3 +317,19 @@ func TestFlakeInstallableAllOutputs(t *testing.T) {
 		t.Errorf("AllOutputs() = true for empty outputs slice, want false")
 	}
 }
+
+func TestBuildQueryString(t *testing.T) {
+	defer func() {
+		if r := recover(); r == nil {
+			t.Error("wanted panic for odd-number of key-value parameters")
+		}
+	}()
+
+	// staticcheck impressively catches buildQueryString calls that have an
+	// odd number of parameters. Build the slice in a convoluted way to
+	// throw it off and suppress the warning (gopls doesn't have nolint
+	// directives).
+	var elems []string
+	elems = append(elems, "1")
+	buildQueryString(elems...)
+}