From 53b4539476338a51f45096e81282a14929d647e6 Mon Sep 17 00:00:00 2001 From: oppiliappan Date: Wed, 15 Oct 2025 10:05:57 +0100 Subject: [PATCH] appview: improve patch validation Change-Id: sknqtnunznmnxorrrzwokoynpynpqwov automatically adds a newline to patches that are missing one. --- appview/pulls/pulls.go | 40 +++++++++++++++------------------- appview/state/router.go | 1 + appview/validator/patch.go | 25 +++++++++++++++++++++ knotserver/xrpc/merge_check.go | 2 ++ patchutil/patchutil.go | 25 +++++++++++++++------ patchutil/patchutil_test.go | 25 +++++++++++---------- 6 files changed, 77 insertions(+), 41 deletions(-) create mode 100644 appview/validator/patch.go diff --git a/appview/pulls/pulls.go b/appview/pulls/pulls.go index c8bc36cf..dde7cb99 100644 --- a/appview/pulls/pulls.go +++ b/appview/pulls/pulls.go @@ -23,6 +23,7 @@ import ( "tangled.org/core/appview/pages" "tangled.org/core/appview/pages/markup" "tangled.org/core/appview/reporesolver" + "tangled.org/core/appview/validator" "tangled.org/core/appview/xrpcclient" "tangled.org/core/idresolver" "tangled.org/core/patchutil" @@ -47,6 +48,7 @@ type Pulls struct { notifier notify.Notifier enforcer *rbac.Enforcer logger *slog.Logger + validator *validator.Validator } func New( @@ -58,6 +60,7 @@ func New( config *config.Config, notifier notify.Notifier, enforcer *rbac.Enforcer, + validator *validator.Validator, logger *slog.Logger, ) *Pulls { return &Pulls{ @@ -70,6 +73,7 @@ func New( notifier: notifier, enforcer: enforcer, logger: logger, + validator: validator, } } @@ -965,7 +969,8 @@ func (s *Pulls) handleBranchBasedPull( patch := comparison.FormatPatchRaw combined := comparison.CombinedPatchRaw - if !patchutil.IsPatchValid(patch) { + if err := s.validator.ValidatePatch(&patch); err != nil { + s.logger.Error("failed to validate patch", "err", err) s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.") return } @@ -982,7 +987,8 @@ func (s *Pulls) handleBranchBasedPull( } func (s *Pulls) handlePatchBasedPull(w http.ResponseWriter, r *http.Request, f *reporesolver.ResolvedRepo, user *oauth.User, title, body, targetBranch, patch string, isStacked bool) { - if !patchutil.IsPatchValid(patch) { + if err := s.validator.ValidatePatch(&patch); err != nil { + s.logger.Error("patch validation failed", "err", err) s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.") return } @@ -1073,7 +1079,8 @@ func (s *Pulls) handleForkBasedPull(w http.ResponseWriter, r *http.Request, f *r patch := comparison.FormatPatchRaw combined := comparison.CombinedPatchRaw - if !patchutil.IsPatchValid(patch) { + if err := s.validator.ValidatePatch(&patch); err != nil { + s.logger.Error("failed to validate patch", "err", err) s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.") return } @@ -1337,7 +1344,8 @@ func (s *Pulls) ValidatePatch(w http.ResponseWriter, r *http.Request) { return } - if patch == "" || !patchutil.IsPatchValid(patch) { + if err := s.validator.ValidatePatch(&patch); err != nil { + s.logger.Error("faield to validate patch", "err", err) s.pages.Notice(w, "patch-error", "Invalid patch format. Please provide a valid git diff or format-patch.") return } @@ -1754,23 +1762,6 @@ func (s *Pulls) resubmitFork(w http.ResponseWriter, r *http.Request) { s.resubmitPullHelper(w, r, f, user, pull, patch, combined, sourceRev) } -// validate a resubmission against a pull request -func validateResubmittedPatch(pull *models.Pull, patch string) error { - if patch == "" { - return fmt.Errorf("Patch is empty.") - } - - if patch == pull.LatestPatch() { - return fmt.Errorf("Patch is identical to previous submission.") - } - - if !patchutil.IsPatchValid(patch) { - return fmt.Errorf("Invalid patch format. Please provide a valid diff.") - } - - return nil -} - func (s *Pulls) resubmitPullHelper( w http.ResponseWriter, r *http.Request, @@ -1787,11 +1778,16 @@ func (s *Pulls) resubmitPullHelper( return } - if err := validateResubmittedPatch(pull, patch); err != nil { + if err := s.validator.ValidatePatch(&patch); err != nil { s.pages.Notice(w, "resubmit-error", err.Error()) return } + if patch == pull.LatestPatch() { + s.pages.Notice(w, "resubmit-error", "Patch is identical to previous submission.") + return + } + // validate sourceRev if branch/fork based if pull.IsBranchBased() || pull.IsForkBased() { if sourceRev == pull.LatestSha() { diff --git a/appview/state/router.go b/appview/state/router.go index ef44896d..c430b962 100644 --- a/appview/state/router.go +++ b/appview/state/router.go @@ -277,6 +277,7 @@ func (s *State) PullsRouter(mw *middleware.Middleware) http.Handler { s.config, s.notifier, s.enforcer, + s.validator, log.SubLogger(s.logger, "pulls"), ) return pulls.Router(mw) diff --git a/appview/validator/patch.go b/appview/validator/patch.go new file mode 100644 index 00000000..eeac9c34 --- /dev/null +++ b/appview/validator/patch.go @@ -0,0 +1,25 @@ +package validator + +import ( + "fmt" + "strings" + + "tangled.org/core/patchutil" +) + +func (v *Validator) ValidatePatch(patch *string) error { + if patch == nil || *patch == "" { + return fmt.Errorf("patch is empty") + } + + // add newline if not present to diff style patches + if !patchutil.IsFormatPatch(*patch) && !strings.HasSuffix(*patch, "\n") { + *patch = *patch + "\n" + } + + if err := patchutil.IsPatchValid(*patch); err != nil { + return err + } + + return nil +} diff --git a/knotserver/xrpc/merge_check.go b/knotserver/xrpc/merge_check.go index 5a0f45ca..ecac65d8 100644 --- a/knotserver/xrpc/merge_check.go +++ b/knotserver/xrpc/merge_check.go @@ -81,6 +81,8 @@ func (x *Xrpc) MergeCheck(w http.ResponseWriter, r *http.Request) { } } + l.Debug("merge check response", "isConflicted", response.Is_conflicted, "err", response.Error, "conflicts", response.Conflicts) + w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) json.NewEncoder(w).Encode(response) diff --git a/patchutil/patchutil.go b/patchutil/patchutil.go index 311693af..584f1aeb 100644 --- a/patchutil/patchutil.go +++ b/patchutil/patchutil.go @@ -1,6 +1,7 @@ package patchutil import ( + "errors" "fmt" "log" "os" @@ -42,14 +43,20 @@ func ExtractPatches(formatPatch string) ([]types.FormatPatch, error) { // IsPatchValid checks if the given patch string is valid. // It performs very basic sniffing for either git-diff or git-format-patch // header lines. For format patches, it attempts to extract and validate each one. -func IsPatchValid(patch string) bool { +var ( + EmptyPatchError error = errors.New("patch is empty") + GenericPatchError error = errors.New("patch is invalid") + FormatPatchError error = errors.New("patch is not a valid format-patch") +) + +func IsPatchValid(patch string) error { if len(patch) == 0 { - return false + return EmptyPatchError } lines := strings.Split(patch, "\n") if len(lines) < 2 { - return false + return EmptyPatchError } firstLine := strings.TrimSpace(lines[0]) @@ -60,7 +67,7 @@ func IsPatchValid(patch string) bool { strings.HasPrefix(firstLine, "Index: ") || strings.HasPrefix(firstLine, "+++ ") || strings.HasPrefix(firstLine, "@@ ") { - return true + return nil } // check if it's format-patch @@ -70,12 +77,16 @@ func IsPatchValid(patch string) bool { // it's safe to say it's broken. patches, err := ExtractPatches(patch) if err != nil { - return false + return fmt.Errorf("%w: %w", FormatPatchError, err) } - return len(patches) > 0 + if len(patches) == 0 { + return EmptyPatchError + } + + return nil } - return false + return GenericPatchError } func IsFormatPatch(patch string) bool { diff --git a/patchutil/patchutil_test.go b/patchutil/patchutil_test.go index d0d8c1f1..81e3abba 100644 --- a/patchutil/patchutil_test.go +++ b/patchutil/patchutil_test.go @@ -1,6 +1,7 @@ package patchutil import ( + "errors" "reflect" "testing" ) @@ -9,17 +10,17 @@ func TestIsPatchValid(t *testing.T) { tests := []struct { name string patch string - expected bool + expected error }{ { name: `empty patch`, patch: ``, - expected: false, + expected: EmptyPatchError, }, { name: `single line patch`, patch: `single line`, - expected: false, + expected: EmptyPatchError, }, { name: `valid diff patch`, @@ -31,7 +32,7 @@ index abc..def 100644 -old line +new line context`, - expected: true, + expected: nil, }, { name: `valid patch starting with ---`, @@ -41,7 +42,7 @@ index abc..def 100644 -old line +new line context`, - expected: true, + expected: nil, }, { name: `valid patch starting with Index`, @@ -53,7 +54,7 @@ index abc..def 100644 -old line +new line context`, - expected: true, + expected: nil, }, { name: `valid patch starting with +++`, @@ -63,7 +64,7 @@ index abc..def 100644 -old line +new line context`, - expected: true, + expected: nil, }, { name: `valid patch starting with @@`, @@ -72,7 +73,7 @@ index abc..def 100644 +new line context `, - expected: true, + expected: nil, }, { name: `valid format patch`, @@ -90,14 +91,14 @@ index 123456..789012 100644 +new content -- 2.48.1`, - expected: true, + expected: nil, }, { name: `invalid format patch`, patch: `From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001 From: Author This is not a valid patch format`, - expected: false, + expected: FormatPatchError, }, { name: `not a patch at all`, @@ -105,14 +106,14 @@ This is not a valid patch format`, just some random text that isn't a patch`, - expected: false, + expected: GenericPatchError, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { result := IsPatchValid(tt.patch) - if result != tt.expected { + if !errors.Is(result, tt.expected) { t.Errorf("IsPatchValid() = %v, want %v", result, tt.expected) } }) -- 2.43.0