Improves Azure Devops webhook error handling

server/events/event_parser.go

@@ -253,7 +253,6 @@ type EventParsing interface {
 	// baseRepo is the repo that the pull request will be merged into.
 	// user is the pull request author.
 	// pullNum is the number of the pull request that triggered the webhook.
-	// *** Add tests and handle linking multiple pull requests to a work item.
 	ParseAzureDevopsWorkItemCommentedEvent(comment *azuredevops.WorkItem, baseURL *string) (pullRefs []PullRef, err error)
 
 	// ParseAzureDevopsPull parses the response from the Azure Devops API endpoint (not
@@ -835,23 +834,19 @@ type PullRef struct {
 // pullNumStr is retrieved from a URI that resembles:
 // vstfs:///Git/PullRequestId/a7573007-bbb3-4341-b726-0c4148a07853%2f3411ebc1-d5aa-464f-9615-0b527bc66719%2f22
 // where the pull request number is 22, following the %2f near tne end
-// Must set a default error value that will be returned if there are no valid
-// pull request relations in the event
 func (e *EventParser) ParseAzureDevopsWorkItemCommentedEvent(comment *azuredevops.WorkItem, baseURL *string) (pullRefs []PullRef, err error) {
-	defaultErrorStr := "no valid pull request relations in event payload"
-	err = errors.New(defaultErrorStr)
 	for _, relation := range comment.Relations {
 		ref := &PullRef{}
 		if uri := relation.GetURL(); strings.Contains(uri, "vstfs:///Git/PullRequestId/") {
 			var parsed *url.URL
 			parsed, err = url.Parse(uri)
 			if err != nil {
-				return pullRefs, err
+				continue
 			}
 			pullNumStr := strings.Split(parsed.Path, "/")[5]
 			ref.PullNum, err = strconv.Atoi(pullNumStr)
 			if err != nil {
-				return pullRefs, err
+				continue
 			}
 
 			// Retrieve the linked pull request to get baseRepo and user
@@ -863,7 +858,7 @@ func (e *EventParser) ParseAzureDevopsWorkItemCommentedEvent(comment *azuredevop
 			httpClient.Timeout = 10 * time.Second
 			client, err := azuredevops.NewClient(httpClient)
 			if err != nil {
-				return pullRefs, err
+				continue
 			}
 			if baseURL != nil {
 				if !strings.HasSuffix(*baseURL, "/") {
@@ -871,20 +866,20 @@ func (e *EventParser) ParseAzureDevopsWorkItemCommentedEvent(comment *azuredevop
 				}
 				parsed, err = url.Parse(*baseURL)
 				if err != nil {
-					return pullRefs, err
+					continue
 				}
 				client.BaseURL = *parsed
 			}
 			opts := azuredevops.PullRequestListOptions{}
 			pr, _, err := client.PullRequests.Get(context.Background(), e.AzureDevopsOrg, e.AzureDevopsProject, ref.PullNum, &opts)
 			if err != nil {
-				return pullRefs, err
+				continue
 			}
 			createdBy := pr.GetCreatedBy()
 			ref.User = models.User{Username: createdBy.GetUniqueName()}
 			ref.BaseRepo, err = e.ParseAzureDevopsRepo(pr.GetRepository())
 			if err != nil {
-				return pullRefs, err
+				continue
 			}
 			pullRefs = append(pullRefs, *ref)
 		}

server/events/event_parser_test.go

@@ -1139,19 +1139,19 @@ func TestParseAzureDevopsWorkItemCommentedEvent(t *testing.T) {
 	resource := testEvent.Resource.(azuredevops.WorkItem)
 	resource.Relations = nil
 	_, err := parser.ParseAzureDevopsWorkItemCommentedEvent(&resource, nil)
-	ErrEquals(t, "no valid pull request relations in event payload", err)
+	Ok(t, err)
 
 	testEvent = deepcopy.Copy(event).(azuredevops.Event)
 	resource = testEvent.Resource.(azuredevops.WorkItem)
 	resource.Relations[0] = nil
 	_, err = parser.ParseAzureDevopsWorkItemCommentedEvent(&resource, nil)
-	ErrEquals(t, "no valid pull request relations in event payload", err)
+	Ok(t, err)
 
 	testEvent = deepcopy.Copy(event).(azuredevops.Event)
 	resource = testEvent.Resource.(azuredevops.WorkItem)
-	resource.Relations[0].URL = azuredevops.String("")
+	resource.Relations[0].URL = azuredevops.String("vstfs:///Git/PullRequestId/a7573007-bbb3-4341-b726-0c4148a07853%2f3411ebc1-d5aa-464f-9615-0b527bc66719%2fwat")
 	_, err = parser.ParseAzureDevopsWorkItemCommentedEvent(&resource, nil)
-	ErrEquals(t, "no valid pull request relations in event payload", err)
+	ErrEquals(t, "strconv.Atoi: parsing \"wat\": invalid syntax", err)
 
 	// this should be successful
 	// This parse function requires a mock HTTP server to return the ADPull fixture

server/events_controller.go

@@ -226,11 +226,11 @@ func (e *EventsController) handleAzureDevopsPost(w http.ResponseWriter, r *http.
 	e.Logger.Debug("request valid")
 
 	azuredevopsReqID := "Request-Id=" + r.Header.Get("Request-Id")
-	event, _ := azuredevops.ParseWebHook(payload)
-	/*event, ok := webhook.(*azuredevops.Event)
-	if !ok {
-		e.respond(w, logging.Debug, http.StatusBadRequest, "Error unmarshaling webhook payload %s", azuredevopsReqID)
-	}*/
+	event, err := azuredevops.ParseWebHook(payload)
+	if err != nil {
+		e.respond(w, logging.Error, http.StatusBadRequest, "Failed parsing webhook: %v %s", err, azuredevopsReqID)
+		return
+	}
 	switch event.PayloadType {
 	case azuredevops.WorkItemCommentedEvent:
 		e.Logger.Debug("handling as comment event")
@@ -239,7 +239,7 @@ func (e *EventsController) handleAzureDevopsPost(w http.ResponseWriter, r *http.
 		e.Logger.Debug("handling as pull request event")
 		e.HandleAzureDevopsPullRequestEvent(w, event, azuredevopsReqID)
 	default:
-		e.respond(w, logging.Debug, http.StatusOK, "Ignoring unsupported event %s", azuredevopsReqID)
+		e.respond(w, logging.Debug, http.StatusOK, "Ignoring unsupported event: %v %s", event.PayloadType, azuredevopsReqID)
 	}
 }
 
@@ -477,15 +477,22 @@ func (e *EventsController) HandleAzureDevopsCommentEvent(w http.ResponseWriter,
 	}
 	strippedComment := bluemonday.StrictPolicy().SanitizeBytes([]byte(*comment))
 
-	if len(workItem.Relations) == 0 {
+	pullRefs, err := e.Parser.ParseAzureDevopsWorkItemCommentedEvent(workItem, nil)
+
+	if err != nil && len(pullRefs) == 0 {
+		e.respond(w, logging.Error, http.StatusBadRequest, "Failed parsing event: %v %s", err, azuredevopsReqID)
+		return
+	}
+
+	if len(pullRefs) == 0 {
 		e.respond(w, logging.Debug, http.StatusOK, "Ignoring comment event since no pull request is linked to work item; Request-Id = %s", azuredevopsReqID)
 		return
 	}
-	pullRefs, err := e.Parser.ParseAzureDevopsWorkItemCommentedEvent(workItem, nil)
 
-	if err != nil {
-		e.respond(w, logging.Error, http.StatusBadRequest, "Failed parsing event: %v %s", err, azuredevopsReqID)
-		return
+	if err != nil && len(pullRefs) > 0 {
+		e.respond(w, logging.Warn, http.StatusOK, "Failed parsing one of the linked pull requests: %v %s", err, azuredevopsReqID)
+	} else {
+		e.respond(w, logging.Info, http.StatusOK, "Successfully parsed workitem.commented event; Request-Id = %s", azuredevopsReqID)
 	}
 
 	for _, ref := range pullRefs {