From 1191c46fa6fab0c0282cf9d6a968446a7f06d058 Mon Sep 17 00:00:00 2001 From: LTLA Date: Sun, 2 Feb 2025 00:19:42 -0800 Subject: [PATCH] Added option to forcibly delete versions/assets or reject probational status. If the manifest/summary files aren't available, this probably indicates that there's something wrong with the version directory, in which case it should be deleted. However, the deletion/rejection endpoints rely on these files to do various things (e.g., updating the project usage). Setting force=true will skip the additional checks if the manifest/summary files aren't present to ensure that the version can be forcibly deleted. The default is still force=false so that the caller can be notified of a problem before possibly repeating the request with force=true. --- README.md | 9 ++ delete.go | 84 +++++++++-------- delete_test.go | 228 ++++++++++++++++++++++++++++++++++------------ probation.go | 37 ++++---- probation_test.go | 95 +++++++++++++------ 5 files changed, 315 insertions(+), 138 deletions(-) diff --git a/README.md b/README.md index af2ab01..30ea249 100644 --- a/README.md +++ b/README.md @@ -267,6 +267,9 @@ This file should be JSON-formatted with the following properties: - `project`: string containing the name of the project. - `asset`: string containing the name of the asset. - `version`: string containing the name of the version. +- `force` (optional): boolean indicating whether a probational version should be forcibly deleted. + Occasionally necessary if the version contains corrupted summary or manifest files, + in which case they will be deleted but the project usage will need to be refreshed manually. On success, the relevant version is removed from the registry. The HTTP response will contain a JSON object with the `status` property set to `SUCCESS`. @@ -345,6 +348,9 @@ This file should be JSON-formatted with the following properties: - `project`: string containing the name of the project. - `asset`: string containing the name of the asset. +- `force` (optional): boolean indicating whether the asset should be forcibly deleted. + Occasionally necessary if the asset contains corrupted manifest files, + in which case they will be deleted but the project usage will need to be refreshed manually. On success, the asset is deleted. The HTTP response will contain a JSON object with the `status` property set to `SUCCESS`. @@ -356,6 +362,9 @@ This file should be JSON-formatted with the following properties: - `project`: string containing the name of the project. - `asset`: string containing the name of the asset. - `version`: string containing the name of the version. +- `force` (optional): boolean indicating whether the version should be forcibly deleted. + Occasionally necessary if the version contains corrupted summary or manifest files, + in which case they will be deleted but the project usage will need to be refreshed manually. On success, the version is deleted. The HTTP response will contain a JSON object with the `type` property set to `SUCCESS`. diff --git a/delete.go b/delete.go index a6d5d6a..474e224 100644 --- a/delete.go +++ b/delete.go @@ -72,6 +72,7 @@ func deleteAssetHandler(reqpath string, globals *globalConfiguration) error { incoming := struct { Project *string `json:"project"` Asset *string `json:"asset"` + Force *bool `json:"force"` }{} { handle, err := os.ReadFile(reqpath) @@ -95,6 +96,8 @@ func deleteAssetHandler(reqpath string, globals *globalConfiguration) error { } } + force_deletion := incoming.Force != nil && *(incoming.Force) + project_dir := filepath.Join(globals.Registry, *(incoming.Project)) if _, err := os.Stat(project_dir); errors.Is(err, os.ErrNotExist) { return nil @@ -109,13 +112,9 @@ func deleteAssetHandler(reqpath string, globals *globalConfiguration) error { if _, err := os.Stat(asset_dir); errors.Is(err, os.ErrNotExist) { return nil } - to_free, err := computeAssetUsage(asset_dir) - if err != nil { - return fmt.Errorf("failed to compute usage for %s; %v", asset_dir, err) - } - usage, err := readUsage(project_dir) - if err != nil { - return fmt.Errorf("failed to read usage for %s; %v", project_dir, err) + asset_usage, asset_usage_err := computeAssetUsage(asset_dir) + if asset_usage_err != nil && !force_deletion { + return fmt.Errorf("failed to compute usage for %s; %v", asset_dir, asset_usage_err) } err = os.RemoveAll(asset_dir) @@ -123,13 +122,19 @@ func deleteAssetHandler(reqpath string, globals *globalConfiguration) error { return fmt.Errorf("failed to delete %s; %v", asset_dir, err) } - usage.Total -= to_free - if usage.Total < 0 { - usage.Total = 0 - } - err = dumpJson(filepath.Join(project_dir, usageFileName), &usage) - if err != nil { - return fmt.Errorf("failed to update usage for %s; %v", project_dir, err) + if asset_usage_err == nil { + project_usage, err := readUsage(project_dir) + if err != nil { + return fmt.Errorf("failed to read usage for %s; %v", project_dir, err) + } + project_usage.Total -= asset_usage + if project_usage.Total < 0 { + project_usage.Total = 0 + } + err = dumpJson(filepath.Join(project_dir, usageFileName), &project_usage) + if err != nil { + return fmt.Errorf("failed to update usage for %s; %v", project_dir, err) + } } payload := map[string]string { @@ -158,6 +163,7 @@ func deleteVersionHandler(reqpath string, globals *globalConfiguration) error { Project *string `json:"project"` Asset *string `json:"asset"` Version *string `json:"version"` + Force *bool `json:"force"` }{} { handle, err := os.ReadFile(reqpath) @@ -184,6 +190,8 @@ func deleteVersionHandler(reqpath string, globals *globalConfiguration) error { } } + force_deletion := incoming.Force != nil && *(incoming.Force) + // We lock the project directory as (i) it's convention to lock the entire // project even if we're mutating a single asset and (ii) we need to update // the usage anyway so we'd have to obtain this lock eventually. @@ -206,18 +214,15 @@ func deleteVersionHandler(reqpath string, globals *globalConfiguration) error { if _, err := os.Stat(version_dir); errors.Is(err, os.ErrNotExist) { return nil } - to_free, err := computeVersionUsage(version_dir) - if err != nil { - return fmt.Errorf("failed to compute usage for %s; %v", version_dir, err) - } - usage, err := readUsage(project_dir) - if err != nil { - return fmt.Errorf("failed to read usage for %s; %v", project_dir, err) + + version_usage, version_usage_err := computeVersionUsage(version_dir) + if version_usage_err != nil && !force_deletion { + return fmt.Errorf("failed to compute usage for %s; %v", version_dir, version_usage_err) } - summ, err := readSummary(version_dir) - if err != nil { - return fmt.Errorf("failed to read summary for %s; %v", version_dir, err) + summ, summ_err := readSummary(version_dir) + if summ_err != nil && !force_deletion { + return fmt.Errorf("failed to read summary for %s; %v", version_dir, summ_err) } err = os.RemoveAll(version_dir) @@ -225,16 +230,24 @@ func deleteVersionHandler(reqpath string, globals *globalConfiguration) error { return fmt.Errorf("failed to delete %s; %v", asset_dir, err) } - usage.Total -= to_free - if usage.Total < 0 { - usage.Total = 0 - } - err = dumpJson(filepath.Join(project_dir, usageFileName), &usage) - if err != nil { - return fmt.Errorf("failed to update usage for %s; %v", project_dir, err) + if version_usage_err == nil { + project_usage, err := readUsage(project_dir) + if err != nil { + return fmt.Errorf("failed to read usage for %s; %v", project_dir, err) + } + + project_usage.Total -= version_usage + if project_usage.Total < 0 { + project_usage.Total = 0 + } + err = dumpJson(filepath.Join(project_dir, usageFileName), &project_usage) + if err != nil { + return fmt.Errorf("failed to update usage for %s; %v", project_dir, err) + } } - if summ.OnProbation == nil || !(*summ.OnProbation) { + if summ_err == nil && (summ.OnProbation == nil || !(*summ.OnProbation)) { + // Only need to make a log if the version is non-probational. prev, err := readLatest(asset_dir) was_latest := false if err == nil { @@ -257,11 +270,10 @@ func deleteVersionHandler(reqpath string, globals *globalConfiguration) error { } // Also refreshing the latest version. - _, err = refreshLatest(asset_dir) - if err != nil { - return fmt.Errorf("failed to update the latest version for %s; %v", asset_dir, err) + _, latest_err := refreshLatest(asset_dir) + if latest_err != nil && !force_deletion { + return fmt.Errorf("failed to update the latest version for %s; %v", asset_dir, latest_err) } - } return nil diff --git a/delete_test.go b/delete_test.go index 84daa7a..1088180 100644 --- a/delete_test.go +++ b/delete_test.go @@ -141,74 +141,117 @@ func TestDeleteProject(t *testing.T) { func TestDeleteAsset(t *testing.T) { project := "foobar" asset := "stuff" - reg, err := mockRegistryForDeletion(project, asset, []string{ "1", "2" }) - if err != nil { - t.Fatalf("failed to mock up registry; %v", err) - } - reqpath, err := dumpRequest("delete_asset", fmt.Sprintf(`{ "project": "%s", "asset": "%s" }`, project, asset)) - if err != nil { - t.Fatalf("failed to dump a request type; %v", err) - } + t.Run("simple", func(t *testing.T) { + reg, err := mockRegistryForDeletion(project, asset, []string{ "1", "2" }) + if err != nil { + t.Fatalf("failed to mock up registry; %v", err) + } + globals := newGlobalConfiguration(reg) - globals := newGlobalConfiguration(reg) - err = deleteAssetHandler(reqpath, &globals) - if err == nil || !strings.Contains(err.Error(), "not authorized") { - t.Fatal("unexpected authorization for non-admin") - } + reqpath, err := dumpRequest("delete_asset", fmt.Sprintf(`{ "project": "%s", "asset": "%s" }`, project, asset)) + if err != nil { + t.Fatalf("failed to dump a request type; %v", err) + } + err = deleteAssetHandler(reqpath, &globals) + if err == nil || !strings.Contains(err.Error(), "not authorized") { + t.Fatal("unexpected authorization for non-admin") + } - self, err := identifyUser(reg) - if err != nil { - t.Fatalf("failed to identify self; %v", err) - } - globals.Administrators = append(globals.Administrators, self) - err = deleteAssetHandler(reqpath, &globals) - if err != nil { - t.Fatalf("failed to delete an asset; %v", err) - } + self, err := identifyUser(reg) + if err != nil { + t.Fatalf("failed to identify self; %v", err) + } + globals.Administrators = append(globals.Administrators, self) + err = deleteAssetHandler(reqpath, &globals) + if err != nil { + t.Fatalf("failed to delete an asset; %v", err) + } - project_dir := filepath.Join(reg, project) - asset_dir := filepath.Join(project_dir, asset) - if _, err := os.Stat(asset_dir); !errors.Is(err, os.ErrNotExist) { - t.Fatal("failed to delete the asset directory") - } + project_dir := filepath.Join(reg, project) + asset_dir := filepath.Join(project_dir, asset) + if _, err := os.Stat(asset_dir); !errors.Is(err, os.ErrNotExist) { + t.Fatal("failed to delete the asset directory") + } - usage, err := readUsage(project_dir) - if err != nil { - t.Fatalf("failed to read usage after deletion; %v", err) - } - if usage.Total != 0 { - t.Fatal("expected zero usage after asset deletion") - } + usage, err := readUsage(project_dir) + if err != nil { + t.Fatalf("failed to read usage after deletion; %v", err) + } + if usage.Total != 0 { + t.Fatal("expected zero usage after asset deletion") + } - // No-ops if repeated with already-deleted asset. - err = deleteAssetHandler(reqpath, &globals) - if err != nil { - t.Fatalf("failed to delete a project; %v", err) - } + // No-ops if repeated with already-deleted asset. + err = deleteAssetHandler(reqpath, &globals) + if err != nil { + t.Fatalf("failed to delete a project; %v", err) + } - // Checking that inputs are valid. - reqpath, err = dumpRequest("delete_asset", `{ "project": "foo" }`) - if err != nil { - t.Fatalf("failed to dump a request type; %v", err) - } + // Checking that inputs are valid. + reqpath, err = dumpRequest("delete_asset", `{ "project": "foo" }`) + if err != nil { + t.Fatalf("failed to dump a request type; %v", err) + } - err = deleteAssetHandler(reqpath, &globals) - if err == nil || !strings.Contains(err.Error(), "invalid 'asset'") { - t.Fatal("fail to throw for invalid request") - } + err = deleteAssetHandler(reqpath, &globals) + if err == nil || !strings.Contains(err.Error(), "invalid 'asset'") { + t.Fatal("fail to throw for invalid request") + } - // Checking that logs were correctly written. - logs, err := readAllLogs(reg) - if err != nil { - t.Fatalf("failed to read all logs; %v", err) - } + // Checking that logs were correctly written. + logs, err := readAllLogs(reg) + if err != nil { + t.Fatalf("failed to read all logs; %v", err) + } - if len(logs) != 1 || logs[0].Type != "delete-asset" || - logs[0].Project == nil || *(logs[0].Project) != project || - logs[0].Asset == nil || *(logs[0].Asset) != asset { - t.Fatal("logs are not as expected from asset deletion") - } + if len(logs) != 1 || logs[0].Type != "delete-asset" || + logs[0].Project == nil || *(logs[0].Project) != project || + logs[0].Asset == nil || *(logs[0].Asset) != asset { + t.Fatal("logs are not as expected from asset deletion") + } + }) + + t.Run("forced", func(t *testing.T) { + reg, err := mockRegistryForDeletion(project, asset, []string{ "1", "2" }) + if err != nil { + t.Fatalf("failed to mock up registry; %v", err) + } + + // Nuke the manifest files. + err = os.Remove(filepath.Join(reg, project, asset, "1", manifestFileName)) + if err != nil { + t.Fatal(err) + } + + globals := newGlobalConfiguration(reg) + self, err := identifyUser(reg) + if err != nil { + t.Fatalf("failed to identify self; %v", err) + } + globals.Administrators = append(globals.Administrators, self) + + reqpath, err := dumpRequest("delete_asset", fmt.Sprintf(`{ "project": "%s", "asset": "%s", "force": false }`, project, asset)) + if err != nil { + t.Fatalf("failed to dump a request type; %v", err) + } + err = deleteAssetHandler(reqpath, &globals) + if err == nil || !strings.Contains(err.Error(), "manifest") { + t.Errorf("expected the deletion to fail in the absence of a manifest; %v", err) + } + + reqpath, err = dumpRequest("delete_asset", fmt.Sprintf(`{ "project": "%s", "asset": "%s", "force": true }`, project, asset)) + if err != nil { + t.Fatalf("failed to dump a request type; %v", err) + } + err = deleteAssetHandler(reqpath, &globals) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(reg, project, asset)); err == nil || !errors.Is(err, os.ErrNotExist) { + t.Error("expected the asset deletion to work correctly") + } + }) } func TestDeleteVersion(t *testing.T) { @@ -442,5 +485,74 @@ func TestDeleteVersion(t *testing.T) { t.Fatalf("no logs should be generated after deleting a probational version; %v", logs[0]) } }) + + t.Run("forced", func(t *testing.T) { + version1 := "no_manifest" + version2 := "no_summary" + reg, err := mockRegistryForDeletion(project, asset, []string{ version1, version2 }) + if err != nil { + t.Fatalf("failed to mock up registry; %v", err) + } + globals := newGlobalConfiguration(reg) + + self, err := identifyUser(reg) + if err != nil { + t.Fatalf("failed to identify self; %v", err) + } + globals.Administrators = append(globals.Administrators, self) + + err = os.Remove(filepath.Join(reg, project, asset, version1, manifestFileName)) + if err != nil { + t.Fatal(err) + } + err = os.Remove(filepath.Join(reg, project, asset, version2, summaryFileName)) + if err != nil { + t.Fatal(err) + } + + // Checking we can force our way through without a manifest. + reqpath, err := dumpRequest("delete_version", fmt.Sprintf(`{ "project": "%s", "asset": "%s", "version": "%s", "force": false }`, project, asset, version1)) + if err != nil { + t.Fatalf("failed to dump a request type; %v", err) + } + err = deleteVersionHandler(reqpath, &globals) + if err == nil || !strings.Contains(err.Error(), "manifest") { + t.Error("deletion should have failed without a manifest file") + } + + reqpath, err = dumpRequest("delete_version", fmt.Sprintf(`{ "project": "%s", "asset": "%s", "version": "%s", "force": true }`, project, asset, version1)) + if err != nil { + t.Fatalf("failed to dump a request type; %v", err) + } + err = deleteVersionHandler(reqpath, &globals) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(reg, project, asset, version1)); err == nil || !errors.Is(err, os.ErrNotExist) { + t.Error("expected the version deletion to work correctly") + } + + // Checking we can force our way through without a summary. + reqpath, err = dumpRequest("delete_version", fmt.Sprintf(`{ "project": "%s", "asset": "%s", "version": "%s", "force": false }`, project, asset, version2)) + if err != nil { + t.Fatalf("failed to dump a request type; %v", err) + } + err = deleteVersionHandler(reqpath, &globals) + if err == nil || !strings.Contains(err.Error(), "summary") { + t.Error("deletion should have failed without a summary file") + } + + reqpath, err = dumpRequest("delete_version", fmt.Sprintf(`{ "project": "%s", "asset": "%s", "version": "%s", "force": true }`, project, asset, version2)) + if err != nil { + t.Fatalf("failed to dump a request type; %v", err) + } + err = deleteVersionHandler(reqpath, &globals) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(reg, project, asset, version2)); err == nil || !errors.Is(err, os.ErrNotExist) { + t.Error("expected the version deletion to work correctly") + } + }) } diff --git a/probation.go b/probation.go index 629f4ce..774085f 100644 --- a/probation.go +++ b/probation.go @@ -15,6 +15,7 @@ func baseProbationHandler(reqpath string, globals *globalConfiguration, approve Project *string `json:"project"` Asset *string `json:"asset"` Version *string `json:"version"` + Force *bool `json:"force"` }{} { handle, err := os.ReadFile(reqpath) @@ -144,30 +145,34 @@ func baseProbationHandler(reqpath string, globals *globalConfiguration, approve } } else { - freed, err := computeVersionUsage(version_dir) - if err != nil { - return fmt.Errorf("failed to compute usage for %q; %w", version_dir, err) + force_deletion := incoming.Force != nil && *(incoming.Force) + + version_usage, version_usage_err := computeVersionUsage(version_dir) + if version_usage_err != nil && !force_deletion { + return fmt.Errorf("failed to compute usage for %q; %w", version_dir, version_usage_err) } - err = os.RemoveAll(version_dir) + err := os.RemoveAll(version_dir) if err != nil { return fmt.Errorf("failed to delete %q; %w", version_dir, err) } - usage, err := readUsage(project_dir) - if err != nil { - return fmt.Errorf("failed to read the usage statistics for %q; %w", project_dir, err) - } + if version_usage_err == nil { + project_usage, err := readUsage(project_dir) + if err != nil { + return fmt.Errorf("failed to read the usage statistics for %q; %w", project_dir, err) + } - usage.Total -= freed - if usage.Total < 0 { // just in case. - usage.Total = 0 - } + project_usage.Total -= version_usage + if project_usage.Total < 0 { // just in case. + project_usage.Total = 0 + } - usage_path := filepath.Join(project_dir, usageFileName) - err = dumpJson(usage_path, &usage) - if err != nil { - return fmt.Errorf("failed to update project usage at %q; %w", usage_path, err) + usage_path := filepath.Join(project_dir, usageFileName) + err = dumpJson(usage_path, &project_usage) + if err != nil { + return fmt.Errorf("failed to update project usage at %q; %w", usage_path, err) + } } } diff --git a/probation_test.go b/probation_test.go index 7a1b164..e12a3c0 100644 --- a/probation_test.go +++ b/probation_test.go @@ -251,39 +251,78 @@ func TestRejectProbationHandler(t *testing.T) { } globals := newGlobalConfiguration(reg) - project := "dawn" - asset := "sinnoh" - version := "foo" - err = mockProbationVersion(reg, project, asset, version) - if err != nil { - t.Fatalf("failed to create a mock version; %v", err) - } - - reqpath, err := dumpRequest("reject_probation", fmt.Sprintf(`{ "project": "%s", "asset": "%s", "version": "%s" }`, project, asset, version)) - if err != nil { - t.Fatalf("failed to dump a request type; %v", err) - } - self, err := identifyUser(reg) if err != nil { t.Fatalf("failed to identify self; %v", err) } globals.Administrators = append(globals.Administrators, self) - err = rejectProbationHandler(reqpath, &globals) - if err != nil { - t.Fatalf("failed to reject probation; %v", err) - } - project_dir := filepath.Join(reg, project) - if _, err := os.Stat(filepath.Join(project_dir, asset, version)); err == nil || !errors.Is(err, os.ErrNotExist) { - t.Fatal("failed to delete the probational directory") - } + t.Run("simple", func(t *testing.T) { + project := "dawn" + asset := "sinnoh" + version := "foo" + err := mockProbationVersion(reg, project, asset, version) + if err != nil { + t.Fatalf("failed to create a mock version; %v", err) + } - usage, err := readUsage(project_dir) - if err != nil { - t.Fatalf("failed to read the project usage; %v", err) - } - if usage.Total != 0 { - t.Fatalf("expected the project usage to be zero, not %d", usage.Total) - } + reqpath, err := dumpRequest("reject_probation", fmt.Sprintf(`{ "project": "%s", "asset": "%s", "version": "%s" }`, project, asset, version)) + if err != nil { + t.Fatalf("failed to dump a request type; %v", err) + } + + err = rejectProbationHandler(reqpath, &globals) + if err != nil { + t.Fatalf("failed to reject probation; %v", err) + } + + project_dir := filepath.Join(reg, project) + if _, err := os.Stat(filepath.Join(project_dir, asset, version)); err == nil || !errors.Is(err, os.ErrNotExist) { + t.Fatal("failed to delete the probational directory") + } + + usage, err := readUsage(project_dir) + if err != nil { + t.Fatalf("failed to read the project usage; %v", err) + } + if usage.Total != 0 { + t.Fatalf("expected the project usage to be zero, not %d", usage.Total) + } + }) + + t.Run("forced", func(t *testing.T) { + project := "serena" + asset := "kalos" + version := "bar" + err := mockProbationVersion(reg, project, asset, version) + if err != nil { + t.Fatalf("failed to create a mock version; %v", err) + } + + err = os.Remove(filepath.Join(reg, project, asset, version, manifestFileName)) + if err != nil { + t.Fatal(err) + } + + reqpath, err := dumpRequest("reject_probation", fmt.Sprintf(`{ "project": "%s", "asset": "%s", "version": "%s", "force": false }`, project, asset, version)) + if err != nil { + t.Fatalf("failed to dump a request type; %v", err) + } + err = rejectProbationHandler(reqpath, &globals) + if err == nil || !strings.Contains(err.Error(), "manifest") { + t.Error("expected request to fail when manifest is removed") + } + + reqpath, err = dumpRequest("reject_probation", fmt.Sprintf(`{ "project": "%s", "asset": "%s", "version": "%s", "force": true }`, project, asset, version)) + if err != nil { + t.Fatalf("failed to dump a request type; %v", err) + } + err = rejectProbationHandler(reqpath, &globals) + if err != nil { + t.Error(err) + } + if _, err := os.Stat(filepath.Join(project, asset, version)); err == nil || !errors.Is(err, os.ErrNotExist) { + t.Error("expected probational version directory to be deleted") + } + }) }