Repository: osbuild/osbuild-composer
Pull request #2908 (2022-08-23 00:45:39)

Add search command to dnf-json and use it for package searches
This pull request includes:

- [X] adequate testing for the new functionality or fixed issue
- [X] adequate documentation informing people about the change such as
  - [ ] submit a PR for the [guides](https://github.com/osbuild/guides) repository if this PR changed any behavior described there: https://www.osbuild.org/guides/


- [X] PR contains automated tests for new functionality and

bcl: On my VM (4G ram, 2 cores) and a hot metadata cache the searches drop from around 10s to 2-3s which makes a noticeable difference.

This does not speed up initial loading with a cold cache of course, and does not speed up listing all packages (I am looking into that next, to see if something can be done there).


Patch:
diff --git a/cmd/mock-dnf-json/dnf-json.go b/cmd/mock-dnf-json/dnf-json.go
index 2d6248037..fb8fd8d99 100644
--- a/cmd/mock-dnf-json/dnf-json.go
+++ b/cmd/mock-dnf-json/dnf-json.go
@@ -14,6 +14,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"strings"
 
 	"github.com/osbuild/osbuild-composer/internal/dnfjson"
 )
@@ -49,11 +50,28 @@ func readTestCase() string {
 	return os.Args[1]
 }
 
-func parseResponse(resp []byte, command string) json.RawMessage {
+func parseResponse(resp []byte, req dnfjson.Request) json.RawMessage {
 	parsedResponse := make(map[string]json.RawMessage)
 	err := json.Unmarshal(resp, &parsedResponse)
 	maybeFail(err)
-	return parsedResponse[command]
+
+	if req.Command == "search" {
+		// Search requests need to return results based on the search
+		// The key to the search is a comma-separated list of the requested packages
+		key := strings.Join(req.Arguments.Search.Packages, ",")
+
+		// Extract the possible response map
+		var searches map[string]json.RawMessage
+		err = json.Unmarshal(parsedResponse["search"], &searches)
+		maybeFail(err)
+
+		if _, ok := searches[key]; !ok {
+			fail(fmt.Errorf("search response map is missing key = %s", key))
+		}
+		return searches[key]
+	} else {
+		return parsedResponse[req.Command]
+	}
 }
 
 func checkForError(msg json.RawMessage) bool {
@@ -79,7 +97,7 @@ func main() {
 		fail(fmt.Errorf("failed to read test file %q\n", testFilePath))
 	}
 
-	res := parseResponse(response, req.Command)
+	res := parseResponse(response, req)
 
 	if req.Command == "depsolve" {
 		// add repo ID to packages
diff --git a/dnf-json b/dnf-json
index f043516a7..5fef065cf 100755
--- a/dnf-json
+++ b/dnf-json
@@ -116,6 +116,59 @@ class Solver():
             })
         return packages
 
+    def search(self, args):
+        """ Perform a search on the available packages
+
+        args contains a "search" dict with parameters to use for searching.
+        "packages" list of package name globs to search for
+        "latest" is a boolean that will return only the latest NEVRA instead
+        of all matching builds in the metadata.
+
+        eg.
+
+            "search": {
+                "latest": false,
+                "packages": ["tmux", "vim*", "*ssh*"]
+            },
+        """
+        pkg_globs = args.get("packages", [])
+
+        packages = []
+
+        # NOTE: Build query one piece at a time, don't pass all to filterm at the same
+        # time.
+        available = self.base.sack.query().available()
+        for name in pkg_globs:
+            # If the package name glob has * in it, use glob.
+            # If it has *name* use substr
+            # If it has neither use exact match
+            if "*" in name:
+                if name[0] != "*" or name[-1] != "*":
+                    q = available.filter(name__glob=name)
+                else:
+                    q = available.filter(name__substr=name.replace("*", ""))
+            else:
+                q = available.filter(name__eq=name)
+
+            if args.get("latest", False):
+                q = q.latest()
+
+            for package in q:
+                packages.append({
+                    "name": package.name,
+                    "summary": package.summary,
+                    "description": package.description,
+                    "url": package.url,
+                    "repo_id": package.repoid,
+                    "epoch": package.epoch,
+                    "version": package.version,
+                    "release": package.release,
+                    "arch": package.arch,
+                    "buildtime": self._timestamp_to_rfc3339(package.buildtime),
+                    "license": package.license
+                })
+        return packages
+
     def depsolve(self, transactions):
         last_transaction = []
 
@@ -203,6 +256,8 @@ def solve(request, cache_dir):
                 result = solver.dump()
             elif command == "depsolve":
                 result = solver.depsolve(transactions)
+            elif command == "search":
+                result = solver.search(arguments.get("search", {}))
 
         except dnf.exceptions.MarkingErrors as e:
             printe("error install_specs")
@@ -250,7 +305,7 @@ def respond(result):
 
 def validate_request(request):
     command = request.get("command")
-    valid_cmds = ("depsolve", "dump")
+    valid_cmds = ("depsolve", "dump", "search")
     if command not in valid_cmds:
         return {
             "kind": "InvalidRequest",
diff --git a/internal/dnfjson/dnfjson.go b/internal/dnfjson/dnfjson.go
index cf5c305b9..c3088f32d 100644
--- a/internal/dnfjson/dnfjson.go
+++ b/internal/dnfjson/dnfjson.go
@@ -179,6 +179,44 @@ func (s *Solver) FetchMetadata(repos []rpmmd.RepoConfig) (rpmmd.PackageList, err
 	return pkgs, nil
 }
 
+// SearchMetadata searches for packages and returns a list of the info for matches.
+func (s *Solver) SearchMetadata(repos []rpmmd.RepoConfig, packages []string) (rpmmd.PackageList, error) {
+	req, err := s.makeSearchRequest(repos, packages)
+	if err != nil {
+		return nil, err
+	}
+
+	// get non-exclusive read lock
+	s.cache.locker.RLock()
+	defer s.cache.locker.RUnlock()
+
+	result, err := run(s.dnfJsonCmd, req)
+	if err != nil {
+		return nil, err
+	}
+
+	// touch repos to now
+	now := time.Now().Local()
+	for _, r := range repos {
+		// ignore errors
+		_ = s.cache.touchRepo(r.Hash(), now)
+	}
+	s.cache.updateInfo()
+
+	var pkgs rpmmd.PackageList
+	if err := json.Unmarshal(result, &pkgs); err != nil {
+		return nil, err
+	}
+
+	sortID := func(pkg rpmmd.Package) string {
+		return fmt.Sprintf("%s-%s-%s", pkg.Name, pkg.Version, pkg.Release)
+	}
+	sort.Slice(pkgs, func(i, j int) bool {
+		return sortID(pkgs[i]) < sortID(pkgs[j])
+	})
+	return pkgs, nil
+}
+
 func (s *Solver) reposFromRPMMD(rpmRepos []rpmmd.RepoConfig) ([]repoConfig, error) {
 	dnfRepos := make([]repoConfig, len(rpmRepos))
 	for idx, rr := range rpmRepos {
@@ -319,6 +357,27 @@ func (s *Solver) makeDumpRequest(repos []rpmmd.RepoConfig) (*Request, error) {
 	return &req, nil
 }
 
+// Helper function for creating a search request payload
+func (s *Solver) makeSearchRequest(repos []rpmmd.RepoConfig, packages []string) (*Request, error) {
+	dnfRepos, err := s.reposFromRPMMD(repos)
+	if err != nil {
+		return nil, err
+	}
+	req := Request{
+		Command:          "search",
+		ModulePlatformID: s.modulePlatformID,
+		Arch:             s.arch,
+		CacheDir:         s.cache.root,
+		Arguments: arguments{
+			Repos: dnfRepos,
+			Search: searchArgs{
+				Packages: packages,
+			},
+		},
+	}
+	return &req, nil
+}
+
 // convert internal a list of PackageSpecs to the rpmmd equivalent and attach
 // key and subscription information based on the repository configs.
 func (pkgs packageSpecs) toRPMMD(repos map[string]rpmmd.RepoConfig) []rpmmd.PackageSpec {
@@ -368,10 +427,23 @@ type arguments struct {
 	// Repositories to use for depsolving
 	Repos []repoConfig `json:"repos"`
 
+	// Search terms to use with search command
+	Search searchArgs `json:"search"`
+
 	// Depsolve package sets and repository mappings for this request
 	Transactions []transactionArgs `json:"transactions"`
 }
 
+type searchArgs struct {
+	// Only include latest NEVRA when true
+	Latest bool `json:"latest"`
+
+	// List of package name globs to search for
+	// If it has '*' it is passed to dnf glob search, if it has *name* it is passed
+	// to substr matching, and if it has neither an exact match is expected.
+	Packages []string `json:"packages"`
+}
+
 type transactionArgs struct {
 	// Packages to depsolve
 	PackageSpecs []string `json:"package-specs"`
diff --git a/internal/mocks/dnfjson/dnfjson.go b/internal/mocks/dnfjson/dnfjson.go
index 39652f8b9..1c5064e70 100644
--- a/internal/mocks/dnfjson/dnfjson.go
+++ b/internal/mocks/dnfjson/dnfjson.go
@@ -46,6 +46,45 @@ func generatePackageList() rpmmd.PackageList {
 	return packageList
 }
 
+// generateSearchResults creates results for use with the dnfjson search command
+// which is used for listing a subset of modules and projects.
+//
+// The map key is a comma-separated list of the packages requested
+// If no packages are included it returns all 22 packages, same as the mock dump
+//
+// nonexistingpkg returns an empty list
+// badpackage1 returns a fetch error, same as when the package name is unknown
+// baddepsolve returns package1, the test then tries to depsolve package1 using BadDepsolve()
+// wich will return a depsolve error.
+func generateSearchResults() map[string]interface{} {
+	allPackages := generatePackageList()
+
+	// This includes package16, package2, package20, and package21
+	var wildcardResults rpmmd.PackageList
+	wildcardResults = append(wildcardResults, allPackages[32], allPackages[33])
+	wildcardResults = append(wildcardResults, allPackages[4], allPackages[5])
+	for i := 40; i < 44; i++ {
+		wildcardResults = append(wildcardResults, allPackages[i])
+	}
+
+	fetchError := dnfjson.Error{
+		Kind:   "FetchError",
+		Reason: "There was a problem when fetching packages.",
+	}
+
+	return map[string]interface{}{
+		"":                    allPackages,
+		"*":                   allPackages,
+		"nonexistingpkg":      rpmmd.PackageList{},
+		"package1":            rpmmd.PackageList{allPackages[2], allPackages[3]},
+		"package1,package2":   rpmmd.PackageList{allPackages[2], allPackages[3], allPackages[4], allPackages[5]},
+		"package2*,package16": wildcardResults,
+		"package16":           rpmmd.PackageList{allPackages[32], allPackages[33]},
+		"badpackage1":         fetchError,
+		"baddepsolve":         rpmmd.PackageList{allPackages[2], allPackages[3]},
+	}
+}
+
 func createBaseDepsolveFixture() []dnfjson.PackageSpec {
 	return []dnfjson.PackageSpec{
 		{
@@ -112,6 +151,7 @@ func Base(tmpdir string) string {
 	data := map[string]interface{}{
 		"depsolve": createBaseDepsolveFixture(),
 		"dump":     generatePackageList(),
+		"search":   generateSearchResults(),
 	}
 	path := filepath.Join(tmpdir, "base.json")
 	write(data, path)
@@ -140,6 +180,7 @@ func BadDepsolve(tmpdir string) string {
 	data := map[string]interface{}{
 		"depsolve": deps,
 		"dump":     generatePackageList(),
+		"search":   generateSearchResults(),
 	}
 	path := filepath.Join(tmpdir, "baddepsolve.json")
 	write(data, path)
@@ -158,6 +199,7 @@ func BadFetch(tmpdir string) string {
 	data := map[string]interface{}{
 		"depsolve": deps,
 		"dump":     pkgs,
+		"search":   generateSearchResults(),
 	}
 	path := filepath.Join(tmpdir, "badfetch.json")
 	write(data, path)
diff --git a/internal/weldr/api.go b/internal/weldr/api.go
index 851a746bb..8992221eb 100644
--- a/internal/weldr/api.go
+++ b/internal/weldr/api.go
@@ -1027,48 +1027,34 @@ func (api *API) modulesListHandler(writer http.ResponseWriter, request *http.Req
 		statusResponseError(writer, http.StatusBadRequest, errors)
 		return
 	}
-	availablePackages, err := api.fetchPackageList(distroName)
 
-	if err != nil {
-		errors := responseError{
-			ID:  "ModulesError",
-			Msg: fmt.Sprintf("msg: %s", err.Error()),
-		}
-		statusResponseError(writer, http.StatusBadRequest, errors)
-		return
-	}
-
-	var packages rpmmd.PackageList
+	var names []string
 	if modulesParam != "" && modulesParam != "/" {
 		// we have modules for search
 
 		// remove leading /
 		modulesParam = modulesParam[1:]
 
-		names := strings.Split(modulesParam, ",")
-
-		packages, err = availablePackages.Search(names...)
+		names = strings.Split(modulesParam, ",")
+	}
+	packages, err := api.fetchPackageList(distroName, names)
 
-		if err != nil {
-			errors := responseError{
-				ID:  "ModulesError",
-				Msg: fmt.Sprintf("Wrong glob pattern: %s", err.Error()),
-			}
-			statusResponseError(writer, http.StatusBadRequest, errors)
-			return
+	if err != nil {
+		errors := responseError{
+			ID:  "ModulesError",
+			Msg: fmt.Sprintf("msg: %s", err.Error()),
 		}
+		statusResponseError(writer, http.StatusBadRequest, errors)
+		return
+	}
 
-		if len(packages) == 0 {
-			errors := responseError{
-				ID:  "UnknownModule",
-				Msg: "No packages have been found.",
-			}
-			statusResponseError(writer, http.StatusBadRequest, errors)
-			return
+	if len(packages) == 0 {
+		errors := responseError{
+			ID:  "UnknownModule",
+			Msg: "No packages have been found.",
 		}
-	} else {
-		// just return all available packages
-		packages = availablePackages
+		statusResponseError(writer, http.StatusBadRequest, errors)
+		return
 	}
 
 	packageInfos := packages.ToPackageInfos()
@@ -1124,7 +1110,7 @@ func (api *API) projectsListHandler(writer http.ResponseWriter, request *http.Re
 		statusResponseError(writer, http.StatusBadRequest, errors)
 		return
 	}
-	availablePackages, err := api.fetchPackageList(distroName)
+	availablePackages, err := api.fetchPackageList(distroName, []string{})
 
 	if err != nil {
 		errors := responseError{
@@ -1207,23 +1193,12 @@ func (api *API) modulesInfoHandler(writer http.ResponseWriter, request *http.Req
 		statusResponseError(writer, http.StatusBadRequest, errors)
 		return
 	}
-	availablePackages, err := api.fetchPackageList(distroName)
-
-	if err != nil {
-		errors := responseError{
-			ID:  "ModulesError",
-			Msg: fmt.Sprintf("msg: %s", err.Error()),
-		}
-		statusResponseError(writer, http.StatusBadRequest, errors)
-		return
-	}
-
-	foundPackages, err := availablePackages.Search(names...)
+	foundPackages, err := api.fetchPackageList(distroName, names)
 
 	if err != nil {
 		errors := responseError{
 			ID:  errorId,
-			Msg: fmt.Sprintf("Wrong glob pattern: %s", err.Error()),
+			Msg: fmt.Sprintf("msg: %s", err.Error()),
 		}
 		statusResponseError(writer, http.StatusBadRequest, errors)
 		return
@@ -3209,7 +3184,7 @@ func (api *API) composeFailedHandler(writer http.ResponseWriter, request *http.R
 }
 
 // fetchPackageList returns the package list or the selected distribution
-func (api *API) fetchPackageList(distroName string) (rpmmd.PackageList, error) {
+func (api *API) fetchPackageList(distroName string, names []string) (packages rpmmd.PackageList, err error) {
 	d := api.getDistro(distroName)
 	if d == nil {
 		return nil, fmt.Errorf("GetDistro - unknown distribution: %s", distroName)
@@ -3220,7 +3195,11 @@ func (api *API) fetchPackageList(distroName string) (rpmmd.PackageList, error) {
 	}
 
 	solver := api.solver.NewWithConfig(d.ModulePlatformID(), d.Releasever(), api.archName)
-	packages, err := solver.FetchMetadata(repos)
+	if len(names) == 0 {
+		packages, err = solver.FetchMetadata(repos)
+	} else {
+		packages, err = solver.SearchMetadata(repos, names)
+	}
 	if err != nil {
 		return nil, err
 	}


Test patch:
diff --git a/internal/weldr/api_test.go b/internal/weldr/api_test.go
index f3a925b4e..ba4995f5a 100644
--- a/internal/weldr/api_test.go
+++ b/internal/weldr/api_test.go
@@ -1543,7 +1543,7 @@ func TestProjectsInfo(t *testing.T) {
 		{rpmmd_mock.BaseFixture, "/api/v0/projects/info/nonexistingpkg", http.StatusBadRequest, `{"status":false,"errors":[{"id":"UnknownProject","msg":"No packages have been found."}]}`},
 		{rpmmd_mock.BaseFixture, "/api/v0/projects/info/*", http.StatusOK, projectsInfoResponse},
 		{rpmmd_mock.BaseFixture, "/api/v0/projects/info/package2*,package16", http.StatusOK, projectsInfoFilteredResponse},
-		{rpmmd_mock.BadFetch, "/api/v0/projects/info/package2*,package16", http.StatusBadRequest, `{"status":false,"errors":[{"id":"ModulesError","msg":"msg: DNF error occurred: FetchError: There was a problem when fetching packages."}]}`},
+		{rpmmd_mock.BadFetch, "/api/v0/projects/info/badpackage1", http.StatusBadRequest, `{"status":false,"errors":[{"id":"ProjectsError","msg":"msg: DNF error occurred: FetchError: There was a problem when fetching packages."}]}`},
 		{rpmmd_mock.BaseFixture, "/api/v0/projects/info/package16?distro=test-distro-2", http.StatusOK, projectsInfoPackage16Response},
 		{rpmmd_mock.BaseFixture, "/api/v0/projects/info/package16?distro=fedora-1", http.StatusBadRequest, `{"status":false,"errors":[{"id":"DistroError","msg":"Invalid distro: fedora-1"}]}`},
 	}
@@ -1566,10 +1566,10 @@ func TestModulesInfo(t *testing.T) {
 		{rpmmd_mock.BaseFixture, "/api/v0/modules/info", http.StatusBadRequest, `{"status":false,"errors":[{"id":"UnknownModule","msg":"No packages specified."}]}`},
 		{rpmmd_mock.BaseFixture, "/api/v0/modules/info/", http.StatusBadRequest, `{"status":false,"errors":[{"id":"UnknownModule","msg":"No packages specified."}]}`},
 		{rpmmd_mock.BaseFixture, "/api/v0/modules/info/nonexistingpkg", http.StatusBadRequest, `{"status":false,"errors":[{"id":"UnknownModule","msg":"No packages have been found."}]}`},
-		{rpmmd_mock.BadDepsolve, "/api/v0/modules/info/package1", http.StatusBadRequest, `{"status":false,"errors":[{"id":"ModulesError","msg":"Cannot depsolve package package1: DNF error occurred: DepsolveError: There was a problem depsolving ['go2rpm']: \n Problem: conflicting requests\n  - nothing provides askalono-cli needed by go2rpm-1-4.fc31.noarch"}]}`},
+		{rpmmd_mock.BadDepsolve, "/api/v0/modules/info/baddepsolve", http.StatusBadRequest, `{"status":false,"errors":[{"id":"ModulesError","msg":"Cannot depsolve package package1: DNF error occurred: DepsolveError: There was a problem depsolving ['go2rpm']: \n Problem: conflicting requests\n  - nothing provides askalono-cli needed by go2rpm-1-4.fc31.noarch"}]}`},
 		{rpmmd_mock.BaseFixture, "/api/v0/modules/info/package2*,package16", http.StatusOK, modulesInfoFilteredResponse},
 		{rpmmd_mock.BaseFixture, "/api/v0/modules/info/*", http.StatusOK, modulesInfoResponse},
-		{rpmmd_mock.BadFetch, "/api/v0/modules/info/package2*,package16", http.StatusBadRequest, `{"status":false,"errors":[{"id":"ModulesError","msg":"msg: DNF error occurred: FetchError: There was a problem when fetching packages."}]}`},
+		{rpmmd_mock.BadFetch, "/api/v0/modules/info/badpackage1", http.StatusBadRequest, `{"status":false,"errors":[{"id":"ModulesError","msg":"msg: DNF error occurred: FetchError: There was a problem when fetching packages."}]}`},
 		{rpmmd_mock.BaseFixture, "/api/v1/modules/info/package2*,package16", http.StatusOK, modulesInfoFilteredResponse},
 		{rpmmd_mock.BaseFixture, "/api/v1/modules/info/package16?distro=test-distro-2", http.StatusOK, modulesInfoPackage16Response},
 		{rpmmd_mock.BaseFixture, "/api/v1/modules/info/package16?distro=fedora-1", http.StatusBadRequest, `{"status":false,"errors":[{"id":"DistroError","msg":"Invalid distro: fedora-1"}]}`},
@@ -1617,7 +1617,7 @@ func TestModulesList(t *testing.T) {
 		{rpmmd_mock.BaseFixture, "/api/v0/modules/list/", http.StatusOK, modulesListResponse},
 		{rpmmd_mock.BaseFixture, "/api/v0/modules/list/nonexistingpkg", http.StatusBadRequest, `{"status":false,"errors":[{"id":"UnknownModule","msg":"No packages have been found."}]}`},
 		{rpmmd_mock.BaseFixture, "/api/v0/modules/list/package2*,package16", http.StatusOK, modulesListFilteredResponse},
-		{rpmmd_mock.BadFetch, "/api/v0/modules/list/package2*,package16", http.StatusBadRequest, `{"status":false,"errors":[{"id":"ModulesError","msg":"msg: DNF error occurred: FetchError: There was a problem when fetching packages."}]}`},
+		{rpmmd_mock.BadFetch, "/api/v0/modules/list/badpackage1", http.StatusBadRequest, `{"status":false,"errors":[{"id":"ModulesError","msg":"msg: DNF error occurred: FetchError: There was a problem when fetching packages."}]}`},
 		{rpmmd_mock.BaseFixture, "/api/v0/modules/list/package2*,package16?offset=1&limit=1", http.StatusOK, `{"total":4,"offset":1,"limit":1,"modules":[{"name":"package2","group_type":"rpm"}]}`},
 		{rpmmd_mock.BaseFixture, "/api/v0/modules/list/*", http.StatusOK, modulesListResponse},
 		{rpmmd_mock.BaseFixture, "/api/v0/modules/list/package2*,package16?distro=test-distro-2&offset=1&limit=1", http.StatusOK, `{"total":4,"offset":1,"limit":1,"modules":[{"name":"package2","group_type":"rpm"}]}`},