fix(#303): make receive hook failure lead to 400 not 500

api/openapi-v3-spec.json

@@ -2032,8 +2032,9 @@
             "type": "object",
             "description": "Map of string (group name e.g. some-owner) of strings (list of usernames), one username for each group is required.",
             "example": {
-              "some-owner": {
-              }
+              "some-owner": [
+                "someotheruser"
+              ]
             },
             "additionalProperties": {
               "type": "array",

internal/acorn/errors/githookerror/error.go

@@ -0,0 +1,14 @@
+package githookerror
+
+import "strings"
+
+const hookError = "pre-receive hook declined"
+
+// Is checks that an error is a git hook error.
+//
+// These errors occur during push operations.
+//
+// Unfortunately, we have to decide this by the error message, as go-git just uses fmt.Errorf().
+func Is(err error) bool {
+	return err != nil && strings.Contains(err.Error(), hookError)
+}

internal/service/updater/owners.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"github.com/Interhyp/metadata-service/api"
+	"github.com/Interhyp/metadata-service/internal/acorn/errors/githookerror"
 	"github.com/Interhyp/metadata-service/internal/acorn/errors/nochangeserror"
 	"github.com/Interhyp/metadata-service/internal/acorn/repository"
 	"github.com/Interhyp/metadata-service/internal/repository/notifier"
@@ -22,6 +23,9 @@ func (s *Impl) WriteOwner(ctx context.Context, ownerAlias string, owner openapi.
 				result.JiraIssue = "" // cannot know
 				return nil
 			}
+			if githookerror.Is(err) {
+				return s.httpErrorFromHook(err, owner.JiraIssue)
+			}
 			return err
 		}
 		result = ownerWritten
@@ -47,6 +51,9 @@ func (s *Impl) DeleteOwner(ctx context.Context, ownerAlias string, deletionInfo
 				// there were no actual changes, this is acceptable
 				return nil
 			}
+			if githookerror.Is(err) {
+				return s.httpErrorFromHook(err, deletionInfo.JiraIssue)
+			}
 			return err
 		}
 

internal/service/updater/repositories.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"github.com/Interhyp/metadata-service/api"
+	"github.com/Interhyp/metadata-service/internal/acorn/errors/githookerror"
 	"github.com/Interhyp/metadata-service/internal/acorn/errors/nochangeserror"
 	"github.com/Interhyp/metadata-service/internal/acorn/repository"
 	"github.com/Interhyp/metadata-service/internal/repository/notifier"
@@ -35,6 +36,9 @@ func (s *Impl) WriteRepository(ctx context.Context, key string, repository opena
 					result.JiraIssue = "" // cannot know, could be multiple issues for the affected files
 					return nil
 				}
+				if githookerror.Is(err) {
+					return s.httpErrorFromHook(err, repository.JiraIssue)
+				}
 				return err
 			}
 			result = repositoryWritten
@@ -46,6 +50,9 @@ func (s *Impl) WriteRepository(ctx context.Context, key string, repository opena
 					result.JiraIssue = "" // cannot know
 					return nil
 				}
+				if githookerror.Is(err) {
+					return s.httpErrorFromHook(err, repository.JiraIssue)
+				}
 				return err
 			}
 			result = repositoryWritten
@@ -71,6 +78,9 @@ func (s *Impl) DeleteRepository(ctx context.Context, key string, deletionInfo op
 				// there were no actual changes, this is acceptable
 				return nil
 			}
+			if githookerror.Is(err) {
+				return s.httpErrorFromHook(err, deletionInfo.JiraIssue)
+			}
 			return err
 		}
 

internal/service/updater/services.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"github.com/Interhyp/metadata-service/api"
+	"github.com/Interhyp/metadata-service/internal/acorn/errors/githookerror"
 	"github.com/Interhyp/metadata-service/internal/acorn/errors/nochangeserror"
 	"github.com/Interhyp/metadata-service/internal/acorn/repository"
 	"github.com/Interhyp/metadata-service/internal/repository/notifier"
@@ -24,6 +25,9 @@ func (s *Impl) WriteService(ctx context.Context, serviceName string, service ope
 					result.JiraIssue = "" // cannot know, could be multiple issues for the affected files
 					return nil
 				}
+				if githookerror.Is(err) {
+					return s.httpErrorFromHook(err, service.JiraIssue)
+				}
 				return err
 			}
 			result = serviceWritten
@@ -46,6 +50,9 @@ func (s *Impl) WriteService(ctx context.Context, serviceName string, service ope
 					result.JiraIssue = "" // cannot know
 					return nil
 				}
+				if githookerror.Is(err) {
+					return s.httpErrorFromHook(err, service.JiraIssue)
+				}
 				return err
 			}
 			result = serviceWritten
@@ -71,6 +78,9 @@ func (s *Impl) DeleteService(ctx context.Context, serviceName string, deletionIn
 				// there were no actual changes, this is acceptable
 				return nil
 			}
+			if githookerror.Is(err) {
+				return s.httpErrorFromHook(err, deletionInfo.JiraIssue)
+			}
 			return err
 		}
 

internal/service/updater/updater.go

@@ -2,12 +2,14 @@ package updater
 
 import (
 	"context"
+	"fmt"
 	"github.com/Interhyp/metadata-service/api"
 	"github.com/Interhyp/metadata-service/internal/acorn/config"
 	"github.com/Interhyp/metadata-service/internal/acorn/repository"
 	"github.com/Interhyp/metadata-service/internal/acorn/service"
 	auzerolog "github.com/StephanHCB/go-autumn-logging-zerolog"
 	librepo "github.com/StephanHCB/go-backend-service-common/acorns/repository"
+	"github.com/StephanHCB/go-backend-service-common/api/apierrors"
 	"github.com/StephanHCB/go-backend-service-common/web/middleware/requestid"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/rs/zerolog/log"
@@ -268,3 +270,10 @@ func equalExceptCacheInfo[T openapi.ServiceDto | openapi.OwnerDto | openapi.Repo
 	}
 	return reflect.DeepEqual(clean(&first), clean(&second))
 }
+
+func (s *Impl) httpErrorFromHook(err error, jiraIssue string) error {
+	return apierrors.NewBadRequestError("push.receive.hook.declined",
+		fmt.Sprintf("git hook declined the commit - most likely your JIRA issue (%s) does not exist, has wrong type, or wrong status", jiraIssue),
+		err,
+		s.Timestamp.Now())
+}

test/acceptance/ownerctl_acc_test.go

@@ -255,6 +255,25 @@ func TestPOSTOwner_GitServerDown(t *testing.T) {
 	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
 }
 
+func TestPOSTOwner_GitHookDeclined(t *testing.T) {
+	tstReset()
+
+	docs.Given("Given an authenticated admin user")
+	token := tstValidAdminToken()
+
+	docs.When("When they request the creation of a valid owner, but supply an invalid issue")
+	body := tstOwner()
+	body.JiraIssue = "INVALID-12345"
+	response, err := tstPerformPost("/rest/api/v1/owners/post-owner-receive-hook", token, &body)
+
+	docs.Then("Then the request fails and the error response is as expected")
+	tstAssert(t, response, err, http.StatusBadRequest, "receive-hook-declined.json")
+
+	docs.Then("And the local metadata repository clone has been reset to its original state")
+	require.Equal(t, 0, len(metadataImpl.FilesWritten))
+	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
+}
+
 // update full owner
 
 func TestPUTOwner_Success(t *testing.T) {
@@ -488,6 +507,25 @@ func TestPUTOwner_GitServerDown(t *testing.T) {
 	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
 }
 
+func TestPUTOwner_GitHookDeclined(t *testing.T) {
+	tstReset()
+
+	docs.Given("Given an authenticated admin user")
+	token := tstValidAdminToken()
+
+	docs.When("When they request an update of an owner, but supply an invalid issue")
+	body := tstOwner()
+	body.JiraIssue = "INVALID-12345"
+	response, err := tstPerformPut("/rest/api/v1/owners/some-owner", token, &body)
+
+	docs.Then("Then the request fails and the error response is as expected")
+	tstAssert(t, response, err, http.StatusBadRequest, "receive-hook-declined.json")
+
+	docs.Then("And the local metadata repository clone has been reset to its original state")
+	require.Equal(t, 0, len(metadataImpl.FilesWritten))
+	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
+}
+
 // patch owner
 
 func TestPATCHOwner_Success(t *testing.T) {
@@ -703,6 +741,25 @@ func TestPATCHOwner_GitServerDown(t *testing.T) {
 	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
 }
 
+func TestPATCHOwner_GitHookDeclined(t *testing.T) {
+	tstReset()
+
+	docs.Given("Given an authenticated admin user")
+	token := tstValidAdminToken()
+
+	docs.When("When they request a patch of an owner, but supply an invalid issue")
+	body := tstOwnerPatch()
+	body.JiraIssue = "INVALID-12345"
+	response, err := tstPerformPatch("/rest/api/v1/owners/some-owner", token, &body)
+
+	docs.Then("Then the request fails and the error response is as expected")
+	tstAssert(t, response, err, http.StatusBadRequest, "receive-hook-declined.json")
+
+	docs.Then("And the local metadata repository clone has been reset to its original state")
+	require.Equal(t, 0, len(metadataImpl.FilesWritten))
+	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
+}
+
 // delete owner
 
 func TestDELETEOwner_Success(t *testing.T) {
@@ -868,3 +925,22 @@ func TestDELETEOwner_GitServerDown(t *testing.T) {
 	require.Equal(t, 0, len(metadataImpl.FilesWritten))
 	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
 }
+
+func TestDELETEOwner_GitHookDeclined(t *testing.T) {
+	tstReset()
+
+	docs.Given("Given an authenticated admin user")
+	token := tstValidAdminToken()
+
+	docs.When("When they request to delete an owner, but supply an invalid issue")
+	body := tstDelete()
+	body.JiraIssue = "INVALID-12345"
+	response, err := tstPerformDelete("/rest/api/v1/owners/deleteme", token, &body)
+
+	docs.Then("Then the request fails and the error response is as expected")
+	tstAssert(t, response, err, http.StatusBadRequest, "receive-hook-declined.json")
+
+	docs.Then("And the local metadata repository clone has been reset to its original state")
+	require.Equal(t, 0, len(metadataImpl.FilesWritten))
+	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
+}

test/acceptance/repositoryctl_acc_test.go

@@ -308,6 +308,25 @@ func TestPOSTRepository_GitServerDown(t *testing.T) {
 	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
 }
 
+func TestPOSTRepository_GitHookDeclined(t *testing.T) {
+	tstReset()
+
+	docs.Given("Given an authenticated admin user")
+	token := tstValidAdminToken()
+
+	docs.When("When they request the creation of a valid repository, but supply an invalid issue")
+	body := tstRepository()
+	body.JiraIssue = "INVALID-12345"
+	response, err := tstPerformPost("/rest/api/v1/repositories/new-repository.api", token, &body)
+
+	docs.Then("Then the request fails and the error response is as expected")
+	tstAssert(t, response, err, http.StatusBadRequest, "receive-hook-declined.json")
+
+	docs.Then("And the local metadata repository clone has been reset to its original state")
+	require.Equal(t, 0, len(metadataImpl.FilesWritten))
+	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
+}
+
 // update full repository
 
 func TestPUTRepository_Success(t *testing.T) {
@@ -542,6 +561,25 @@ func TestPUTRepository_GitServerDown(t *testing.T) {
 	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
 }
 
+func TestPUTRepository_GitHookDeclined(t *testing.T) {
+	tstReset()
+
+	docs.Given("Given an authenticated admin user")
+	token := tstValidAdminToken()
+
+	docs.When("When they request an update of a repository, but supply an invalid issue")
+	body := tstRepository()
+	body.JiraIssue = "INVALID-12345"
+	response, err := tstPerformPut("/rest/api/v1/repositories/karma-wrapper.helm-chart", token, &body)
+
+	docs.Then("Then the request fails and the error response is as expected")
+	tstAssert(t, response, err, http.StatusBadRequest, "receive-hook-declined.json")
+
+	docs.Then("And the local metadata repository clone has been reset to its original state")
+	require.Equal(t, 0, len(metadataImpl.FilesWritten))
+	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
+}
+
 func TestPUTRepository_ChangeOwner(t *testing.T) {
 	tstReset()
 
@@ -830,6 +868,25 @@ func TestPATCHRepository_GitServerDown(t *testing.T) {
 	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
 }
 
+func TestPATCHRepository_GitHookDeclined(t *testing.T) {
+	tstReset()
+
+	docs.Given("Given an authenticated admin user")
+	token := tstValidAdminToken()
+
+	docs.When("When they attempt to patch a repository, but supply an invalid issue")
+	body := tstRepositoryPatch()
+	body.JiraIssue = "INVALID-12345"
+	response, err := tstPerformPatch("/rest/api/v1/repositories/karma-wrapper.helm-chart", token, &body)
+
+	docs.Then("Then the request fails and the error response is as expected")
+	tstAssert(t, response, err, http.StatusBadRequest, "receive-hook-declined.json")
+
+	docs.Then("And the local metadata repository clone has been reset to its original state")
+	require.Equal(t, 0, len(metadataImpl.FilesWritten))
+	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
+}
+
 func TestPATCHRepository_ChangeOwner(t *testing.T) {
 	tstReset()
 
@@ -1032,6 +1089,25 @@ func TestDELETERepository_GitServerDown(t *testing.T) {
 	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
 }
 
+func TestDELETERepository_GitHookDeclined(t *testing.T) {
+	tstReset()
+
+	docs.Given("Given an authenticated admin user")
+	token := tstValidAdminToken()
+
+	docs.When("When they request to delete a repository, but supply an invalid issue")
+	body := tstDelete()
+	body.JiraIssue = "INVALID-12345"
+	response, err := tstPerformDelete("/rest/api/v1/repositories/karma-wrapper.helm-chart", token, &body)
+
+	docs.Then("Then the request fails and the error response is as expected")
+	tstAssert(t, response, err, http.StatusBadRequest, "receive-hook-declined.json")
+
+	docs.Then("And the local metadata repository clone has been reset to its original state")
+	require.Equal(t, 0, len(metadataImpl.FilesWritten))
+	require.Equal(t, 0, len(metadataImpl.FilesCommitted))
+}
+
 func TestDELETERepository_Referenced(t *testing.T) {
 	tstReset()