From 13359581df51a25ff9f7492419206a9b6e2edbc4 Mon Sep 17 00:00:00 2001 From: caicandong <50507092+CaiCandong@users.noreply.github.com> Date: Wed, 26 Jul 2023 16:52:07 +0800 Subject: [PATCH] refactor improve NoBetterThan (#26126) - The `NoBetterThan` function can only handle comparisons between "pending," "success," "error," and "failure." For any other comparison, we directly return false. This prevents logic errors like the one in #26121. - The callers of the `NoBetterThan` function should also avoid making incomparable calls. --------- Co-authored-by: yp05327 <576951401@qq.com> Co-authored-by: puni9869 <80308335+puni9869@users.noreply.github.com> --- modules/structs/commit_status.go | 23 +++- modules/structs/commit_status_test.go | 174 ++++++++++++++++++++++++++ services/convert/status.go | 2 +- templates/swagger/v1_json.tmpl | 2 +- 4 files changed, 193 insertions(+), 8 deletions(-) create mode 100644 modules/structs/commit_status_test.go diff --git a/modules/structs/commit_status.go b/modules/structs/commit_status.go index ff31f2d2a..de1d8fa56 100644 --- a/modules/structs/commit_status.go +++ b/modules/structs/commit_status.go @@ -4,7 +4,7 @@ package structs // CommitStatusState holds the state of a CommitStatus -// It can be "pending", "success", "error", "failure", and "warning" +// It can be "pending", "success", "error" and "failure" type CommitStatusState string const ( @@ -18,14 +18,25 @@ const ( CommitStatusFailure CommitStatusState = "failure" ) +var commitStatusPriorities = map[CommitStatusState]int{ + CommitStatusError: 0, + CommitStatusFailure: 1, + CommitStatusPending: 2, + CommitStatusSuccess: 3, +} + // NoBetterThan returns true if this State is no better than the given State +// This function only handles the states defined in CommitStatusPriorities func (css CommitStatusState) NoBetterThan(css2 CommitStatusState) bool { - commitStatusPriorities := map[CommitStatusState]int{ - CommitStatusError: 0, - CommitStatusFailure: 1, - CommitStatusPending: 2, - CommitStatusSuccess: 3, + // NoBetterThan only handles the 4 states above + if _, exist := commitStatusPriorities[css]; !exist { + return false } + + if _, exist := commitStatusPriorities[css2]; !exist { + return false + } + return commitStatusPriorities[css] <= commitStatusPriorities[css2] } diff --git a/modules/structs/commit_status_test.go b/modules/structs/commit_status_test.go new file mode 100644 index 000000000..f06808534 --- /dev/null +++ b/modules/structs/commit_status_test.go @@ -0,0 +1,174 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package structs + +import ( + "testing" +) + +func TestNoBetterThan(t *testing.T) { + type args struct { + css CommitStatusState + css2 CommitStatusState + } + var unExpectedState CommitStatusState + tests := []struct { + name string + args args + want bool + }{ + { + name: "success is no better than success", + args: args{ + css: CommitStatusSuccess, + css2: CommitStatusSuccess, + }, + want: true, + }, + { + name: "success is no better than pending", + args: args{ + css: CommitStatusSuccess, + css2: CommitStatusPending, + }, + want: false, + }, + { + name: "success is no better than failure", + args: args{ + css: CommitStatusSuccess, + css2: CommitStatusFailure, + }, + want: false, + }, + { + name: "success is no better than error", + args: args{ + css: CommitStatusSuccess, + css2: CommitStatusError, + }, + want: false, + }, + { + name: "pending is no better than success", + args: args{ + css: CommitStatusPending, + css2: CommitStatusSuccess, + }, + want: true, + }, + { + name: "pending is no better than pending", + args: args{ + css: CommitStatusPending, + css2: CommitStatusPending, + }, + want: true, + }, + { + name: "pending is no better than failure", + args: args{ + css: CommitStatusPending, + css2: CommitStatusFailure, + }, + want: false, + }, + { + name: "pending is no better than error", + args: args{ + css: CommitStatusPending, + css2: CommitStatusError, + }, + want: false, + }, + { + name: "failure is no better than success", + args: args{ + css: CommitStatusFailure, + css2: CommitStatusSuccess, + }, + want: true, + }, + { + name: "failure is no better than pending", + args: args{ + css: CommitStatusFailure, + css2: CommitStatusPending, + }, + want: true, + }, + { + name: "failure is no better than failure", + args: args{ + css: CommitStatusFailure, + css2: CommitStatusFailure, + }, + want: true, + }, + { + name: "failure is no better than error", + args: args{ + css: CommitStatusFailure, + css2: CommitStatusError, + }, + want: false, + }, + { + name: "error is no better than success", + args: args{ + css: CommitStatusError, + css2: CommitStatusSuccess, + }, + want: true, + }, + { + name: "error is no better than pending", + args: args{ + css: CommitStatusError, + css2: CommitStatusPending, + }, + want: true, + }, + { + name: "error is no better than failure", + args: args{ + css: CommitStatusError, + css2: CommitStatusFailure, + }, + want: true, + }, + { + name: "error is no better than error", + args: args{ + css: CommitStatusError, + css2: CommitStatusError, + }, + want: true, + }, + { + name: "unExpectedState is no better than success", + args: args{ + css: unExpectedState, + css2: CommitStatusSuccess, + }, + want: false, + }, + { + name: "unExpectedState is no better than unExpectedState", + args: args{ + css: unExpectedState, + css2: unExpectedState, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := tt.args.css.NoBetterThan(tt.args.css2) + if result != tt.want { + t.Errorf("NoBetterThan() = %v, want %v", result, tt.want) + } + }) + } +} diff --git a/services/convert/status.go b/services/convert/status.go index c7b6450e2..6cef63c1c 100644 --- a/services/convert/status.go +++ b/services/convert/status.go @@ -48,7 +48,7 @@ func ToCombinedStatus(ctx context.Context, statuses []*git_model.CommitStatus, r retStatus.Statuses = make([]*api.CommitStatus, 0, len(statuses)) for _, status := range statuses { retStatus.Statuses = append(retStatus.Statuses, ToCommitStatus(ctx, status)) - if status.State.NoBetterThan(retStatus.State) { + if retStatus.State == "" || status.State.NoBetterThan(retStatus.State) { retStatus.State = status.State } } diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 7cb9bac95..10ad8f555 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -16514,7 +16514,7 @@ "x-go-package": "code.gitea.io/gitea/modules/structs" }, "CommitStatusState": { - "description": "CommitStatusState holds the state of a CommitStatus\nIt can be \"pending\", \"success\", \"error\", \"failure\", and \"warning\"", + "description": "CommitStatusState holds the state of a CommitStatus\nIt can be \"pending\", \"success\", \"error\" and \"failure\"", "type": "string", "x-go-package": "code.gitea.io/gitea/modules/structs" },