Merge branch 'main' into fix/config-ui-dev-api-proxy

Author: Thundercloud12

backend/core/plugin/plugin_blueprint.go

@@ -20,6 +20,7 @@ package plugin
 import (
 	"encoding/json"
 
+	"github.com/apache/devlake/core/dal"
 	"github.com/apache/devlake/core/errors"
 	"github.com/apache/devlake/core/models"
 )
@@ -92,6 +93,23 @@ type ProjectMapper interface {
 	MapProject(projectName string, scopes []Scope) (models.PipelinePlan, errors.Error)
 }
 
+// ProjectDeleteHook is implemented by plugins that persist state whose
+// lifecycle is tied to a DevLake project.
+//
+// BeforeDeleteProject is called inside the core deletion transaction before
+// the project's core records are deleted. Returning an error aborts deletion.
+//
+// Contract:
+// - Database mutations must use only the supplied transaction.
+// - The plugin must not commit or roll back the transaction.
+// - External side effects cannot be rolled back by the database transaction.
+// - Plugins must not depend on hook execution order across plugins.
+// Plugins that do not maintain project-scoped state do not need to implement
+// this interface.
+type ProjectDeleteHook interface {
+	BeforeDeleteProject(tx dal.Transaction, projectName string) errors.Error
+}
+
 // CompositeDataSourcePluginBlueprintV200 is for unit test
 type CompositeDataSourcePluginBlueprintV200 interface {
 	PluginMeta

backend/helpers/pluginhelper/services/blueprint_helper.go

@@ -262,7 +262,18 @@ func (b *BlueprintManager) DeleteBlueprint(id uint64) errors.Error {
 			}
 		}
 	}()
-	err = tx.Delete(&models.BlueprintLabel{}, dal.Where("blueprint_id = ?", id))
+	err = b.DeleteBlueprintInTransaction(tx, id)
+	if err != nil {
+		return err
+	}
+	err = tx.Commit()
+	return err
+}
+
+// DeleteBlueprintInTransaction removes a blueprint and its dependent records using
+// the caller's transaction. The caller owns commit and rollback.
+func (b *BlueprintManager) DeleteBlueprintInTransaction(tx dal.Transaction, id uint64) errors.Error {
+	err := tx.Delete(&models.BlueprintLabel{}, dal.Where("blueprint_id = ?", id))
 	if err != nil {
 		return err
 	}
@@ -274,9 +285,15 @@ func (b *BlueprintManager) DeleteBlueprint(id uint64) errors.Error {
 	if err != nil {
 		return err
 	}
-	errors.Must(tx.Delete(&models.BlueprintConnection{}, dal.Where("blueprint_id = ?", id)))
-	errors.Must(tx.Delete(&models.BlueprintScope{}, dal.Where("blueprint_id = ?", id)))
-	return tx.Commit()
+	err = tx.Delete(&models.BlueprintConnection{}, dal.Where("blueprint_id = ?", id))
+	if err != nil {
+		return err
+	}
+	err = tx.Delete(&models.BlueprintScope{}, dal.Where("blueprint_id = ?", id))
+	if err != nil {
+		return err
+	}
+	return nil
 }
 
 func (b *BlueprintManager) fillBlueprintDetail(blueprint *models.Blueprint) errors.Error {

backend/helpers/pluginhelper/services/blueprint_helper_test.go

@@ -0,0 +1,54 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements.  See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License.  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package services
+
+import (
+	"testing"
+
+	"github.com/apache/devlake/core/errors"
+	mockdal "github.com/apache/devlake/mocks/core/dal"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+)
+
+func TestDeleteBlueprintInTransaction(t *testing.T) {
+	t.Run("uses the caller transaction for every delete", func(t *testing.T) {
+		tx := mockdal.NewTransaction(t)
+		tx.On("Delete", mock.Anything, mock.Anything).Return(nil)
+		tx.On("Delete", mock.Anything).Return(nil)
+
+		manager := &BlueprintManager{}
+		err := manager.DeleteBlueprintInTransaction(tx, 42)
+
+		assert.NoError(t, err)
+		assert.Len(t, tx.Calls, 4)
+	})
+
+	t.Run("returns dependent deletion failures", func(t *testing.T) {
+		expected := errors.Default.New("unable to delete blueprint")
+		tx := mockdal.NewTransaction(t)
+		tx.On("Delete", mock.AnythingOfType("*models.Blueprint")).Return(expected).Once()
+		tx.On("Delete", mock.Anything, mock.Anything).Return(nil)
+
+		manager := &BlueprintManager{}
+		err := manager.DeleteBlueprintInTransaction(tx, 42)
+
+		assert.ErrorIs(t, err, expected)
+		assert.Len(t, tx.Calls, 2)
+	})
+}

backend/plugins/github/tasks/pr_collector.go

@@ -129,6 +129,7 @@ func CollectApiPullRequests(taskCtx plugin.SubTaskContext) errors.Error {
 					res.Body.Close()
 					return []json.RawMessage{body}, nil
 				},
+				AfterResponse: ignoreHTTPStatus404,
 			},
 		},
 	})

backend/plugins/github/tasks/shared_test.go

@@ -0,0 +1,126 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements.  See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License.  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tasks
+
+import (
+	"bytes"
+	"io"
+	"net/http"
+	"testing"
+
+	"github.com/apache/devlake/core/errors"
+	"github.com/apache/devlake/helpers/pluginhelper/api"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func makeTestResponse(statusCode int) *http.Response {
+	return &http.Response{
+		StatusCode: statusCode,
+		Body:       io.NopCloser(bytes.NewBufferString("")),
+		Request:    &http.Request{},
+	}
+}
+
+func TestIgnoreHTTPStatus404(t *testing.T) {
+	tests := []struct {
+		name       string
+		statusCode int
+		wantIgnore bool
+		wantUnauth bool
+		wantNil    bool
+	}{
+		{
+			name:       "404 returns ErrIgnoreAndContinue",
+			statusCode: http.StatusNotFound,
+			wantIgnore: true,
+		},
+		{
+			name:       "401 returns unauthorized error",
+			statusCode: http.StatusUnauthorized,
+			wantUnauth: true,
+		},
+		{
+			name:       "200 returns nil",
+			statusCode: http.StatusOK,
+			wantNil:    true,
+		},
+		{
+			name:       "403 returns nil",
+			statusCode: http.StatusForbidden,
+			wantNil:    true,
+		},
+		{
+			name:       "500 returns nil",
+			statusCode: http.StatusInternalServerError,
+			wantNil:    true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			res := makeTestResponse(tt.statusCode)
+			err := ignoreHTTPStatus404(res)
+			if tt.wantIgnore {
+				assert.Equal(t, api.ErrIgnoreAndContinue, err)
+			} else if tt.wantUnauth {
+				require.NotNil(t, err)
+				assert.Equal(t, errors.Unauthorized, err.GetType())
+			} else if tt.wantNil {
+				assert.Nil(t, err)
+			}
+		})
+	}
+}
+
+func TestIgnoreHTTPStatus422(t *testing.T) {
+	tests := []struct {
+		name       string
+		statusCode int
+		wantIgnore bool
+		wantNil    bool
+	}{
+		{
+			name:       "422 returns ErrIgnoreAndContinue",
+			statusCode: http.StatusUnprocessableEntity,
+			wantIgnore: true,
+		},
+		{
+			name:       "200 returns nil",
+			statusCode: http.StatusOK,
+			wantNil:    true,
+		},
+		{
+			name:       "404 returns nil",
+			statusCode: http.StatusNotFound,
+			wantNil:    true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			res := makeTestResponse(tt.statusCode)
+			err := ignoreHTTPStatus422(res)
+			if tt.wantIgnore {
+				assert.Equal(t, api.ErrIgnoreAndContinue, err)
+			} else if tt.wantNil {
+				assert.Nil(t, err)
+			}
+		})
+	}
+}

backend/plugins/jira/api/connection_api.go

@@ -19,7 +19,9 @@ package api
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
+	"io"
 	"net/http"
 	"net/url"
 	"strings"
@@ -39,8 +41,13 @@ type JiraTestConnResponse struct {
 }
 
 func testConnection(ctx context.Context, connection models.JiraConn) (*JiraTestConnResponse, errors.Error) {
-	// validate
-	if vld != nil {
+	// OAuth2 is plugin-specific and is not in MultiAuth's oneof, so validate it
+	// separately. Other auth methods keep the existing StructExcept path.
+	if connection.IsOAuth2() {
+		if err := connection.ValidateConnection(&connection, vld); err != nil {
+			return nil, err
+		}
+	} else if vld != nil {
 		e := vld.StructExcept(connection, "BasicAuth", "AccessToken")
 		if e != nil {
 			return nil, errors.Convert(e)
@@ -74,10 +81,12 @@ func testConnection(ctx context.Context, connection models.JiraConn) (*JiraTestC
 		return nil, errors.NotFound.New(fmt.Sprintf("Seems like an invalid Endpoint URL, please try %s", restUrl.String()))
 	}
 	if res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden {
-		return nil, errors.HttpStatus(res.StatusCode).New("Please check your credential")
+		// 400 not 401: Config UI treats HTTP 401 as a DevLake session expiry and
+		// navigates to /login, wiping the connection form.
+		return nil, errors.BadInput.New(withJiraHTTPDetail(res, "Please check your credential"))
 	}
 	if res.StatusCode != http.StatusOK {
-		return nil, errors.HttpStatus(res.StatusCode).New(fmt.Sprintf("%s unexpected status code: %d", serverInfoFail, res.StatusCode))
+		return nil, errors.HttpStatus(res.StatusCode).New(withJiraHTTPDetail(res, fmt.Sprintf("%s unexpected status code: %d", serverInfoFail, res.StatusCode)))
 	}
 
 	resBody := &models.JiraServerInfo{}
@@ -101,13 +110,16 @@ func testConnection(ctx context.Context, connection models.JiraConn) (*JiraTestC
 	}
 	getStatusFail += ": [ " + res.Request.URL.String() + " ]"
 
-	errMsg := ""
-	if res.StatusCode == http.StatusUnauthorized {
-		return nil, errors.HttpStatus(res.StatusCode).New("Please check your credential")
+	if res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden {
+		return nil, errors.BadInput.New(withJiraHTTPDetail(res, fmt.Sprintf(
+			"Jira accepted the connection (serverInfo OK) but GET %s returned %d",
+			res.Request.URL.String(),
+			res.StatusCode,
+		)))
 	}
 
 	if res.StatusCode != http.StatusOK {
-		return nil, errors.HttpStatus(res.StatusCode).New(fmt.Sprintf("%s Unexpected [%s] status code: %d %s", getStatusFail, res.Request.URL, res.StatusCode, errMsg))
+		return nil, errors.HttpStatus(res.StatusCode).New(withJiraHTTPDetail(res, fmt.Sprintf("%s Unexpected [%s] status code: %d", getStatusFail, res.Request.URL, res.StatusCode)))
 	}
 	connection = connection.Sanitize()
 	body := JiraTestConnResponse{}
@@ -222,3 +234,62 @@ func ListConnections(input *plugin.ApiResourceInput) (*plugin.ApiResourceOutput,
 func GetConnection(input *plugin.ApiResourceInput) (*plugin.ApiResourceOutput, errors.Error) {
 	return dsHelper.ConnApi.GetDetail(input)
 }
+
+func withJiraHTTPDetail(res *http.Response, msg string) string {
+	if detail := jiraHTTPErrorDetail(res); detail != "" {
+		return msg + ": " + detail
+	}
+	return msg
+}
+
+// jiraHTTPErrorDetail reads Jira's error payload. Classic REST uses
+// errorMessages; the Atlassian OAuth gateway often uses message and/or
+// WWW-Authenticate (e.g. insufficient_scope).
+func jiraHTTPErrorDetail(res *http.Response) string {
+	if res == nil || res.Body == nil {
+		return ""
+	}
+	defer res.Body.Close()
+	raw, err := io.ReadAll(io.LimitReader(res.Body, 4096))
+	if err != nil {
+		return ""
+	}
+	if parsed := parseJiraErrorBody(strings.TrimSpace(string(raw))); parsed != "" {
+		return parsed
+	}
+	if auth := strings.TrimSpace(res.Header.Get("WWW-Authenticate")); auth != "" {
+		return auth
+	}
+	text := strings.TrimSpace(string(raw))
+	if text == "" || strings.HasPrefix(text, "<") {
+		return ""
+	}
+	const maxLen = 500
+	if len(text) > maxLen {
+		return text[:maxLen] + "..."
+	}
+	return text
+}
+
+func parseJiraErrorBody(text string) string {
+	if text == "" || text[0] != '{' {
+		return ""
+	}
+	var payload struct {
+		ErrorMessages []string `json:"errorMessages"`
+		Message       string   `json:"message"`
+	}
+	if json.Unmarshal([]byte(text), &payload) != nil {
+		return ""
+	}
+	parts := make([]string, 0, len(payload.ErrorMessages)+1)
+	for _, m := range payload.ErrorMessages {
+		if m = strings.TrimSpace(m); m != "" {
+			parts = append(parts, m)
+		}
+	}
+	if m := strings.TrimSpace(payload.Message); m != "" {
+		parts = append(parts, m)
+	}
+	return strings.Join(parts, "; ")
+}