Просмотр исходного кода

feat(api): warn on Diagnostics when deprecated StartActionByGet* is called

Calls to StartActionByGet and StartActionByGetAndWait now raise a
`deprecated_api_call` warning on the Diagnostics page (and the nav issue
count), once per method, naming the replacement method. This helps admins
find integrations that still use them before they are removed (#1028).

Add runtime issues to configissues: unlike Report, they are kept when the
issue list is rebuilt on config reload or entity changes, and are cleared
on restart.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jamesread 18 часов назад
Родитель
Сommit
3daed0c319

+ 4 - 0
service/internal/api/api.go

@@ -336,6 +336,8 @@ func (api *oliveTinAPI) StartActionAndWait(ctx ctx.Context, req *connect.Request
 }
 
 func (api *oliveTinAPI) StartActionByGet(ctx ctx.Context, req *connect.Request[apiv1.StartActionByGetRequest]) (*connect.Response[apiv1.StartActionByGetResponse], error) {
+	reportDeprecatedAPICall("StartActionByGet", "StartAction")
+
 	binding := api.executor.FindBindingByID(req.Msg.ActionId)
 	if binding == nil || binding.Action == nil {
 		return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", req.Msg.ActionId))
@@ -387,6 +389,8 @@ func (api *oliveTinAPI) startActionByGetAndWaitLogEntry(binding *executor.Action
 }
 
 func (api *oliveTinAPI) StartActionByGetAndWait(ctx ctx.Context, req *connect.Request[apiv1.StartActionByGetAndWaitRequest]) (*connect.Response[apiv1.StartActionByGetAndWaitResponse], error) {
+	reportDeprecatedAPICall("StartActionByGetAndWait", "StartActionAndWait")
+
 	binding := api.executor.FindBindingByID(req.Msg.ActionId)
 	if binding == nil || binding.Action == nil {
 		return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", req.Msg.ActionId))

+ 21 - 0
service/internal/api/api_deprecated.go

@@ -0,0 +1,21 @@
+package api
+
+import (
+	"fmt"
+
+	"github.com/OliveTin/OliveTin/internal/configissues"
+)
+
+// reportDeprecatedAPICall raises a warning on the Diagnostics page (once per
+// method, until OliveTin restarts) when a deprecated API method is called, so
+// admins can find and update the integrations still using it.
+func reportDeprecatedAPICall(method, replacement string) {
+	configissues.ReportRuntime(configissues.Issue{
+		Severity: configissues.SeverityWarning,
+		Code:     configissues.CodeDeprecatedAPICall,
+		Message: fmt.Sprintf(
+			"The deprecated API method %s was called. Use %s instead, as %s will be removed in a future version. See https://docs.olivetin.app/api/start_action.html#api-deprecated-byget",
+			method, replacement, method),
+		Source: method,
+	})
+}

+ 82 - 0
service/internal/api/api_deprecated_test.go

@@ -0,0 +1,82 @@
+package api
+
+import (
+	"context"
+	"testing"
+
+	"connectrpc.com/connect"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	apiv1 "github.com/OliveTin/OliveTin/gen/olivetin/api/v1"
+	config "github.com/OliveTin/OliveTin/internal/config"
+	"github.com/OliveTin/OliveTin/internal/configissues"
+	"github.com/OliveTin/OliveTin/internal/executor"
+)
+
+func deprecatedAPICallSources(t *testing.T, client interface {
+	GetDiagnostics(context.Context, *connect.Request[apiv1.GetDiagnosticsRequest]) (*connect.Response[apiv1.GetDiagnosticsResponse], error)
+}) map[string]bool {
+	t.Helper()
+
+	res, err := client.GetDiagnostics(context.Background(), connect.NewRequest(&apiv1.GetDiagnosticsRequest{}))
+	require.NoError(t, err)
+
+	sources := map[string]bool{}
+	for _, issue := range res.Msg.ConfigIssues {
+		if issue.Code == configissues.CodeDeprecatedAPICall {
+			assert.Equal(t, configissues.SeverityWarning, issue.Severity)
+			sources[issue.Source] = true
+		}
+	}
+	return sources
+}
+
+func TestDeprecatedStartActionByGetRaisesDiagnosticsWarning(t *testing.T) {
+	configissues.ClearRuntime()
+	t.Cleanup(configissues.ClearRuntime)
+
+	cfg := config.DefaultConfig()
+	cfg.Actions = []*config.Action{{Title: "Ping", ID: "ping", Shell: "echo ping"}}
+
+	ex := executor.DefaultExecutor(cfg)
+	ex.RebuildActionMap()
+	testServer, client := getNewTestServerAndClientWithExecutor(cfg, ex)
+	defer testServer.Close()
+
+	assert.Empty(t, deprecatedAPICallSources(t, client))
+
+	//nolint:staticcheck // Deliberately calling the deprecated RPC.
+	_, err := client.StartActionByGet(context.Background(), connect.NewRequest(&apiv1.StartActionByGetRequest{ActionId: "ping"}))
+	require.NoError(t, err)
+
+	//nolint:staticcheck // Deliberately calling the deprecated RPC.
+	_, err = client.StartActionByGetAndWait(context.Background(), connect.NewRequest(&apiv1.StartActionByGetAndWaitRequest{ActionId: "ping"}))
+	require.NoError(t, err)
+
+	sources := deprecatedAPICallSources(t, client)
+	assert.True(t, sources["StartActionByGet"])
+	assert.True(t, sources["StartActionByGetAndWait"])
+
+	// Config reloads and entity changes rebuild the issue list; the warnings must survive.
+	ex.RebuildActionMap()
+	sources = deprecatedAPICallSources(t, client)
+	assert.True(t, sources["StartActionByGet"])
+	assert.True(t, sources["StartActionByGetAndWait"])
+}
+
+func TestStartActionDoesNotRaiseDeprecatedWarning(t *testing.T) {
+	configissues.ClearRuntime()
+	t.Cleanup(configissues.ClearRuntime)
+
+	cfg := config.DefaultConfig()
+	cfg.Actions = []*config.Action{{Title: "Ping", ID: "ping", Shell: "echo ping"}}
+
+	testServer, client := getNewTestServerAndClient(cfg)
+	defer testServer.Close()
+
+	_, err := client.StartAction(context.Background(), connect.NewRequest(&apiv1.StartActionRequest{BindingId: "ping"}))
+	require.NoError(t, err)
+
+	assert.Empty(t, deprecatedAPICallSources(t, client))
+}

+ 3 - 2
service/internal/configcheck/rebuild.go

@@ -17,16 +17,17 @@ import (
 )
 
 // Rebuild clears and rebuilds the issue list from the current config, entity
-// state, sticky load-time reports, and any extra issues provided by callers
+// state, sticky load-time reports, runtime reports, and any extra issues provided by callers
 // (for example literal argument default validation from the executor).
 func Rebuild(cfg *config.Config, extra ...configissues.Issue) {
 	if cfg == nil {
-		configissues.Replace(extra)
+		configissues.Replace(append(configissues.CopyRuntime(), extra...))
 		return
 	}
 
 	collected := make([]configissues.Issue, 0)
 	collected = append(collected, configissues.CopySticky()...)
+	collected = append(collected, configissues.CopyRuntime()...)
 	collected = append(collected, collectActionGroupIssues(cfg)...)
 	collected = append(collected, collectAclReferenceIssues(cfg)...)
 	collected = append(collected, collectArgumentIssues(cfg)...)

+ 1 - 0
service/internal/configissues/issue.go

@@ -22,6 +22,7 @@ const (
 	CodeCronEntityBinding      = "cron_entity_binding"
 	CodeWatcherPath            = "watcher_path"
 	CodeAclUnknown             = "acl_unknown"
+	CodeDeprecatedAPICall      = "deprecated_api_call"
 )
 
 // Issue is a configuration warning or error surfaced on Diagnostics.

+ 47 - 0
service/internal/configissues/runtime.go

@@ -0,0 +1,47 @@
+package configissues
+
+import "sync"
+
+var (
+	runtimeMu     sync.Mutex
+	runtimeIssues []Issue
+)
+
+// ReportRuntime records an issue discovered while OliveTin is running, rather
+// than from the configuration (for example, a call to a deprecated API
+// method). Unlike Report, runtime issues are kept when the issue list is
+// rebuilt on config reload or entity changes. They are cleared on restart.
+func ReportRuntime(issue Issue) {
+	addRuntime(issue)
+	Report(issue)
+}
+
+func addRuntime(issue Issue) {
+	runtimeMu.Lock()
+	defer runtimeMu.Unlock()
+
+	fingerprint := issueFingerprint(issue)
+	for _, existing := range runtimeIssues {
+		if issueFingerprint(existing) == fingerprint {
+			return
+		}
+	}
+
+	runtimeIssues = append(runtimeIssues, issue)
+}
+
+// CopyRuntime returns a copy of the runtime issues.
+func CopyRuntime() []Issue {
+	runtimeMu.Lock()
+	defer runtimeMu.Unlock()
+	out := make([]Issue, len(runtimeIssues))
+	copy(out, runtimeIssues)
+	return out
+}
+
+// ClearRuntime removes all runtime issues. Intended for tests.
+func ClearRuntime() {
+	runtimeMu.Lock()
+	defer runtimeMu.Unlock()
+	runtimeIssues = nil
+}

+ 30 - 0
service/internal/configissues/runtime_test.go

@@ -0,0 +1,30 @@
+package configissues_test
+
+import (
+	"testing"
+
+	"github.com/OliveTin/OliveTin/internal/configissues"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestReportRuntimeDeduplicatesAndLists(t *testing.T) {
+	configissues.ClearRuntime()
+	configissues.Replace(nil)
+	t.Cleanup(func() {
+		configissues.ClearRuntime()
+		configissues.Replace(nil)
+	})
+
+	issue := configissues.Issue{
+		Severity: configissues.SeverityWarning,
+		Code:     configissues.CodeDeprecatedAPICall,
+		Message:  "deprecated",
+		Source:   "StartActionByGet",
+	}
+
+	configissues.ReportRuntime(issue)
+	configissues.ReportRuntime(issue)
+
+	assert.Len(t, configissues.CopyRuntime(), 1)
+	assert.Equal(t, 1, configissues.Count())
+}