(catalogd) add more unit tests for localdir storage.Instance (#1713)

Also rename `query` to `metas` in places that were missed by #1703

Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>

Author: anik120

catalogd/cmd/catalogd/main.go

@@ -306,7 +306,7 @@ func main() {
 	localStorage = &storage.LocalDirV1{
 		RootDir:            storeDir,
 		RootURL:            baseStorageURL,
-		EnableQueryHandler: features.CatalogdFeatureGate.Enabled(features.APIV1QueryHandler),
+		EnableMetasHandler: features.CatalogdFeatureGate.Enabled(features.APIV1MetasHandler),
 	}
 
 	// Config for the catalogd web server

catalogd/internal/features/features.go

@@ -6,11 +6,11 @@ import (
 )
 
 const (
-	APIV1QueryHandler = featuregate.Feature("APIV1QueryHandler")
+	APIV1MetasHandler = featuregate.Feature("APIV1MetasHandler")
 )
 
 var catalogdFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
-	APIV1QueryHandler: {Default: false, PreRelease: featuregate.Alpha},
+	APIV1MetasHandler: {Default: false, PreRelease: featuregate.Alpha},
 }
 
 var CatalogdFeatureGate featuregate.MutableFeatureGate = featuregate.NewFeatureGate()

catalogd/internal/storage/localdir.go

@@ -28,12 +28,12 @@ import (
 type LocalDirV1 struct {
 	RootDir            string
 	RootURL            *url.URL
-	EnableQueryHandler bool
+	EnableMetasHandler bool
 
 	m sync.RWMutex
 	// this singleflight Group is used in `getIndex()`` to handle concurrent HTTP requests
 	// optimally. With the use of this slightflight group, the index is loaded from disk
-	// once per concurrent group of HTTP requests being handled by the query handler.
+	// once per concurrent group of HTTP requests being handled by the metas handler.
 	// The single flight instance gives us a way to load the index from disk exactly once
 	// per concurrent group of callers, and then let every concurrent caller have access to
 	// the loaded index. This avoids lots of unnecessary open/decode/close cycles when concurrent
@@ -60,7 +60,7 @@ func (s *LocalDirV1) Store(ctx context.Context, catalog string, fsys fs.FS) erro
 	defer os.RemoveAll(tmpCatalogDir)
 
 	storeMetaFuncs := []storeMetasFunc{storeCatalogData}
-	if s.EnableQueryHandler {
+	if s.EnableMetasHandler {
 		storeMetaFuncs = append(storeMetaFuncs, storeIndexData)
 	}
 
@@ -126,7 +126,7 @@ func (s *LocalDirV1) ContentExists(catalog string) bool {
 		return false
 	}
 
-	if s.EnableQueryHandler {
+	if s.EnableMetasHandler {
 		indexFileStat, err := os.Stat(catalogIndexFilePath(s.catalogDir(catalog)))
 		if err != nil {
 			return false
@@ -189,8 +189,8 @@ func (s *LocalDirV1) StorageServerHandler() http.Handler {
 	mux := http.NewServeMux()
 
 	mux.HandleFunc(s.RootURL.JoinPath("{catalog}", "api", "v1", "all").Path, s.handleV1All)
-	if s.EnableQueryHandler {
-		mux.HandleFunc(s.RootURL.JoinPath("{catalog}", "api", "v1", "metas").Path, s.handleV1Query)
+	if s.EnableMetasHandler {
+		mux.HandleFunc(s.RootURL.JoinPath("{catalog}", "api", "v1", "metas").Path, s.handleV1Metas)
 	}
 	allowedMethodsHandler := func(next http.Handler, allowedMethods ...string) http.Handler {
 		allowedMethodSet := sets.New[string](allowedMethods...)
@@ -219,10 +219,24 @@ func (s *LocalDirV1) handleV1All(w http.ResponseWriter, r *http.Request) {
 	http.ServeContent(w, r, "", catalogStat.ModTime(), catalogFile)
 }
 
-func (s *LocalDirV1) handleV1Query(w http.ResponseWriter, r *http.Request) {
+func (s *LocalDirV1) handleV1Metas(w http.ResponseWriter, r *http.Request) {
 	s.m.RLock()
 	defer s.m.RUnlock()
 
+	// Check for unexpected query parameters
+	expectedParams := map[string]bool{
+		"schema":  true,
+		"package": true,
+		"name":    true,
+	}
+
+	for param := range r.URL.Query() {
+		if !expectedParams[param] {
+			httpError(w, errInvalidParams)
+			return
+		}
+	}
+
 	catalog := r.PathValue("catalog")
 	catalogFile, catalogStat, err := s.catalogData(catalog)
 	if err != nil {

catalogd/internal/storage/localdir_test.go

@@ -66,11 +66,11 @@ func TestLocalDirStoraget(t *testing.T) {
 			},
 		},
 		{
-			name: "storing with query handler enabled should create indexes",
+			name: "storing with metas handler enabled should create indices",
 			setup: func(t *testing.T) (*LocalDirV1, fs.FS) {
 				s := &LocalDirV1{
 					RootDir:            t.TempDir(),
-					EnableQueryHandler: true,
+					EnableMetasHandler: true,
 				}
 				return s, createTestFS(t)
 			},
@@ -106,7 +106,7 @@ func TestLocalDirStoraget(t *testing.T) {
 				for i := 0; i < 10; i++ {
 					wg.Add(1)
 					go func() {
-						defer wg.Add(-1)
+						defer wg.Done()
 						for j := 0; j < 100; j++ {
 							s.ContentExists(catalog)
 						}
@@ -266,13 +266,13 @@ func TestLocalDirServerHandler(t *testing.T) {
 	}
 }
 
-// Tests to verify the behavior of the query endpoint, as described in
-// https://docs.google.com/document/d/1s6_9IFEKGQLNh3ueH7SF4Yrx4PW9NSiNFqFIJx0pU-8/edit?usp=sharing
-func TestQueryEndpoint(t *testing.T) {
+// Tests to verify the behavior of the metas endpoint, as described in
+// https://docs.google.com/document/d/1s6_9IFEKGQLNh3ueH7SF4Yrx4PW9NSiNFqFIJx0pU-8/
+func TestMetasEndpoint(t *testing.T) {
 	store := &LocalDirV1{
 		RootDir:            t.TempDir(),
 		RootURL:            &url.URL{Path: urlPrefix},
-		EnableQueryHandler: true,
+		EnableMetasHandler: true,
 	}
 	if store.Store(context.Background(), "test-catalog", createTestFS(t)) != nil {
 		t.Fatal("failed to store test catalog")
@@ -281,7 +281,7 @@ func TestQueryEndpoint(t *testing.T) {
 
 	testCases := []struct {
 		name               string
-		setupStore         func() (*httptest.Server, error)
+		initRequest        func(req *http.Request) error
 		queryParams        string
 		expectedStatusCode int
 		expectedContent    string
@@ -329,27 +329,50 @@ func TestQueryEndpoint(t *testing.T) {
 			expectedContent:    "",
 		},
 		{
-			name:               "cached response with If-Modified-Since",
-			queryParams:        "?schema=olm.package",
+			name:               "valid query with packageName that returns multiple blobs",
+			queryParams:        "?package=webhook_operator_test",
+			expectedStatusCode: http.StatusOK,
+			expectedContent: `{"image":"quaydock.io/namespace/bundle:0.0.3","name":"bundle.v0.0.1","package":"webhook_operator_test","properties":[{"type":"olm.bundle.object","value":{"data":"dW5pbXBvcnRhbnQK"}},{"type":"some.other","value":{"data":"arbitrary-info"}}],"relatedImages":[{"image":"testimage:latest","name":"test"}],"schema":"olm.bundle"}
+{"entries":[{"name":"bundle.v0.0.1"}],"name":"preview_test","package":"webhook_operator_test","schema":"olm.channel"}`,
+		},
+		{
+			name:        "cached response with If-Modified-Since",
+			queryParams: "?schema=olm.package",
+			initRequest: func(req *http.Request) error {
+				resp, err := http.DefaultClient.Do(req)
+				if err != nil {
+					return err
+				}
+				resp.Body.Close()
+				req.Header.Set("If-Modified-Since", resp.Header.Get("Last-Modified"))
+				return nil
+			},
 			expectedStatusCode: http.StatusNotModified,
 			expectedContent:    "",
 		},
+		{
+			name:               "request with unknown parameters",
+			queryParams:        "?non-existent=foo",
+			expectedStatusCode: http.StatusBadRequest,
+			expectedContent:    "400 Bad Request",
+		},
+		{
+			name:               "request with duplicate parameters",
+			queryParams:        "?schema=olm.bundle&&schema=olm.bundle",
+			expectedStatusCode: http.StatusOK,
+			expectedContent:    `{"image":"quaydock.io/namespace/bundle:0.0.3","name":"bundle.v0.0.1","package":"webhook_operator_test","properties":[{"type":"olm.bundle.object","value":{"data":"dW5pbXBvcnRhbnQK"}},{"type":"some.other","value":{"data":"arbitrary-info"}}],"relatedImages":[{"image":"testimage:latest","name":"test"}],"schema":"olm.bundle"}`,
+		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/catalogs/test-catalog/api/v1/metas%s", testServer.URL, tc.queryParams), nil)
+			reqGet, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/catalogs/test-catalog/api/v1/metas%s", testServer.URL, tc.queryParams), nil)
 			require.NoError(t, err)
 
-			if strings.Contains(tc.name, "If-Modified-Since") {
-				// Do an initial request to get a Last-Modified timestamp
-				// for the actual request
-				resp, err := http.DefaultClient.Do(req)
-				require.NoError(t, err)
-				resp.Body.Close()
-				req.Header.Set("If-Modified-Since", resp.Header.Get("Last-Modified"))
+			if tc.initRequest != nil {
+				require.NoError(t, tc.initRequest(reqGet))
 			}
-			resp, err := http.DefaultClient.Do(req)
+			resp, err := http.DefaultClient.Do(reqGet)
 			require.NoError(t, err)
 			defer resp.Body.Close()
 
@@ -358,6 +381,30 @@ func TestQueryEndpoint(t *testing.T) {
 			actualContent, err := io.ReadAll(resp.Body)
 			require.NoError(t, err)
 			require.Equal(t, tc.expectedContent, strings.TrimSpace(string(actualContent)))
+
+			// Also do a HEAD request
+			reqHead, err := http.NewRequest(http.MethodHead, fmt.Sprintf("%s/catalogs/test-catalog/api/v1/metas%s", testServer.URL, tc.queryParams), nil)
+			require.NoError(t, err)
+			if tc.initRequest != nil {
+				require.NoError(t, tc.initRequest(reqHead))
+			}
+			resp, err = http.DefaultClient.Do(reqHead)
+			require.NoError(t, err)
+			require.Equal(t, tc.expectedStatusCode, resp.StatusCode)
+			actualContent, err = io.ReadAll(resp.Body)
+			require.NoError(t, err)
+			require.Equal(t, "", string(actualContent)) // HEAD should not return a body
+			resp.Body.Close()
+
+			// And make sure any other method is not allowed
+			for _, method := range []string{http.MethodPost, http.MethodPut, http.MethodDelete} {
+				reqPost, err := http.NewRequest(method, fmt.Sprintf("%s/catalogs/test-catalog/api/v1/metas%s", testServer.URL, tc.queryParams), nil)
+				require.NoError(t, err)
+				resp, err = http.DefaultClient.Do(reqPost)
+				require.NoError(t, err)
+				require.Equal(t, http.StatusMethodNotAllowed, resp.StatusCode)
+				resp.Body.Close()
+			}
 		})
 	}
 }
@@ -366,7 +413,7 @@ func TestServerLoadHandling(t *testing.T) {
 	store := &LocalDirV1{
 		RootDir:            t.TempDir(),
 		RootURL:            &url.URL{Path: urlPrefix},
-		EnableQueryHandler: true,
+		EnableMetasHandler: true,
 	}
 
 	// Create large test data
@@ -443,20 +490,20 @@ func TestServerLoadHandling(t *testing.T) {
 			},
 		},
 		{
-			name:       "mixed all and query endpoints",
+			name:       "mixed all and metas endpoints",
 			concurrent: 40,
 			requests: func(baseURL string) []*http.Request {
 				var reqs []*http.Request
 				for i := 0; i < 20; i++ {
 					allReq, _ := http.NewRequest(http.MethodGet,
 						fmt.Sprintf("%s/catalogs/test-catalog/api/v1/all", baseURL),
 						nil)
-					queryReq, _ := http.NewRequest(http.MethodGet,
+					metasReq, _ := http.NewRequest(http.MethodGet,
 						fmt.Sprintf("%s/catalogs/test-catalog/api/v1/metas?schema=olm.bundle", baseURL),
 						nil)
 					allReq.Header.Set("Accept", "application/jsonl")
-					queryReq.Header.Set("Accept", "application/jsonl")
-					reqs = append(reqs, allReq, queryReq)
+					metasReq.Header.Set("Accept", "application/jsonl")
+					reqs = append(reqs, allReq, metasReq)
 				}
 				return reqs
 			},