diff --git a/cmd/generate_changelog/incoming/1906.txt b/cmd/generate_changelog/incoming/1906.txt new file mode 100644 index 00000000..82f5680a --- /dev/null +++ b/cmd/generate_changelog/incoming/1906.txt @@ -0,0 +1,7 @@ +### PR [#1906](https://github.com/danielmiessler/Fabric/pull/1906) by [ksylvan](https://github.com/ksylvan): Code Quality: Optimize HTTP client reuse + simplify error formatting + +- Refactor: optimize HTTP client reuse and simplify error formatting +- Simplify error wrapping by removing redundant Sprintf calls in CLI +- Pass HTTP client to FetchModelsDirectly to enable connection reuse +- Store persistent HTTP client instance inside the OpenAI provider struct +- Update compatible AI providers to match the new function signature diff --git a/internal/cli/output.go b/internal/cli/output.go index da13e1a3..6c44c12d 100644 --- a/internal/cli/output.go +++ b/internal/cli/output.go @@ -14,19 +14,19 @@ import ( func CopyToClipboard(message string) (err error) { if err = clipboard.WriteAll(message); err != nil { - err = fmt.Errorf("%s", fmt.Sprintf(i18n.T("could_not_copy_to_clipboard"), err)) + err = fmt.Errorf(i18n.T("could_not_copy_to_clipboard"), err) } return } func CreateOutputFile(message string, fileName string) (err error) { if _, err = os.Stat(fileName); err == nil { - err = fmt.Errorf("%s", fmt.Sprintf(i18n.T("file_already_exists_not_overwriting"), fileName)) + err = fmt.Errorf(i18n.T("file_already_exists_not_overwriting"), fileName) return } var file *os.File if file, err = os.Create(fileName); err != nil { - err = fmt.Errorf("%s", fmt.Sprintf(i18n.T("error_creating_file"), err)) + err = fmt.Errorf(i18n.T("error_creating_file"), err) return } defer file.Close() @@ -34,7 +34,7 @@ func CreateOutputFile(message string, fileName string) (err error) { message += "\n" } if _, err = file.WriteString(message); err != nil { - err = fmt.Errorf("%s", fmt.Sprintf(i18n.T("error_writing_to_file"), err)) + err = fmt.Errorf(i18n.T("error_writing_to_file"), err) } else { debuglog.Log("\n\n[Output also written to %s]\n", fileName) } @@ -51,13 +51,13 @@ func CreateAudioOutputFile(audioData []byte, fileName string) (err error) { // File existence check is now done in the CLI layer before TTS generation var file *os.File if file, err = os.Create(fileName); err != nil { - err = fmt.Errorf("%s", fmt.Sprintf(i18n.T("error_creating_audio_file"), err)) + err = fmt.Errorf(i18n.T("error_creating_audio_file"), err) return } defer file.Close() if _, err = file.Write(audioData); err != nil { - err = fmt.Errorf("%s", fmt.Sprintf(i18n.T("error_writing_audio_data"), err)) + err = fmt.Errorf(i18n.T("error_writing_audio_data"), err) } // No redundant output message here - the CLI layer handles success messaging return diff --git a/internal/plugins/ai/openai/direct_models.go b/internal/plugins/ai/openai/direct_models.go index 847fb956..1c5eb973 100644 --- a/internal/plugins/ai/openai/direct_models.go +++ b/internal/plugins/ai/openai/direct_models.go @@ -30,7 +30,8 @@ const maxResponseSize = 10 * 1024 * 1024 // 10MB // standard OpenAI SDK method fails due to a nonstandard format. This is useful // for providers that return a direct array of models (e.g., GitHub Models) or // other OpenAI-compatible implementations. -func FetchModelsDirectly(ctx context.Context, baseURL, apiKey, providerName string) ([]string, error) { +// If httpClient is nil, a new client with default settings will be created. +func FetchModelsDirectly(ctx context.Context, baseURL, apiKey, providerName string, httpClient *http.Client) ([]string, error) { if ctx == nil { ctx = context.Background() } @@ -52,10 +53,12 @@ func FetchModelsDirectly(ctx context.Context, baseURL, apiKey, providerName stri req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", apiKey)) req.Header.Set("Accept", "application/json") - // TODO: Consider reusing a single http.Client instance (e.g., as a field on Client) instead of allocating a new one for - // each request. - client := &http.Client{ - Timeout: 10 * time.Second, + // Reuse provided HTTP client, or create a new one if not provided + client := httpClient + if client == nil { + client = &http.Client{ + Timeout: 10 * time.Second, + } } resp, err := client.Do(req) if err != nil { diff --git a/internal/plugins/ai/openai/openai.go b/internal/plugins/ai/openai/openai.go index efd767e6..db48f856 100644 --- a/internal/plugins/ai/openai/openai.go +++ b/internal/plugins/ai/openai/openai.go @@ -3,8 +3,10 @@ package openai import ( "context" "fmt" + "net/http" "slices" "strings" + "time" "github.com/danielmiessler/fabric/internal/chat" "github.com/danielmiessler/fabric/internal/domain" @@ -65,6 +67,7 @@ type Client struct { ApiBaseURL *plugins.SetupQuestion ApiClient *openai.Client ImplementsResponses bool // Whether this provider supports the Responses API + httpClient *http.Client } // SetResponsesAPIEnabled configures whether to use the Responses API @@ -79,6 +82,11 @@ func (o *Client) configure() (ret error) { } client := openai.NewClient(opts...) o.ApiClient = &client + + // Initialize HTTP client for direct API calls (reused across requests) + o.httpClient = &http.Client{ + Timeout: 10 * time.Second, + } return } @@ -96,7 +104,7 @@ func (o *Client) ListModels() (ret []string, err error) { // Some providers (e.g., GitHub Models) return non-standard response formats // that the SDK fails to parse. debuglog.Debug(debuglog.Basic, "SDK Models.List failed for %s: %v, falling back to direct API fetch\n", o.GetName(), err) - return FetchModelsDirectly(context.Background(), o.ApiBaseURL.Value, o.ApiKey.Value, o.GetName()) + return FetchModelsDirectly(context.Background(), o.ApiBaseURL.Value, o.ApiKey.Value, o.GetName(), o.httpClient) } func (o *Client) SendStream( diff --git a/internal/plugins/ai/openai/openai_models_test.go b/internal/plugins/ai/openai/openai_models_test.go index 69ed8081..8903a904 100644 --- a/internal/plugins/ai/openai/openai_models_test.go +++ b/internal/plugins/ai/openai/openai_models_test.go @@ -20,7 +20,7 @@ func TestFetchModelsDirectly_DirectArray(t *testing.T) { })) defer srv.Close() - models, err := FetchModelsDirectly(context.Background(), srv.URL, "test-key", "TestProvider") + models, err := FetchModelsDirectly(context.Background(), srv.URL, "test-key", "TestProvider", nil) assert.NoError(t, err) assert.Equal(t, 1, len(models)) assert.Equal(t, "github-model", models[0]) @@ -36,7 +36,7 @@ func TestFetchModelsDirectly_OpenAIFormat(t *testing.T) { })) defer srv.Close() - models, err := FetchModelsDirectly(context.Background(), srv.URL, "test-key", "TestProvider") + models, err := FetchModelsDirectly(context.Background(), srv.URL, "test-key", "TestProvider", nil) assert.NoError(t, err) assert.Equal(t, 1, len(models)) assert.Equal(t, "openai-model", models[0]) @@ -52,7 +52,7 @@ func TestFetchModelsDirectly_EmptyArray(t *testing.T) { })) defer srv.Close() - models, err := FetchModelsDirectly(context.Background(), srv.URL, "test-key", "TestProvider") + models, err := FetchModelsDirectly(context.Background(), srv.URL, "test-key", "TestProvider", nil) assert.NoError(t, err) assert.Equal(t, 0, len(models)) } diff --git a/internal/plugins/ai/openai_compatible/direct_models_call.go b/internal/plugins/ai/openai_compatible/direct_models_call.go index c9cbc8c3..5e6cb9f9 100644 --- a/internal/plugins/ai/openai_compatible/direct_models_call.go +++ b/internal/plugins/ai/openai_compatible/direct_models_call.go @@ -9,5 +9,5 @@ import ( // DirectlyGetModels is used to fetch models directly from the API when the // standard OpenAI SDK method fails due to a nonstandard format. func (c *Client) DirectlyGetModels(ctx context.Context) ([]string, error) { - return openai.FetchModelsDirectly(ctx, c.ApiBaseURL.Value, c.ApiKey.Value, c.GetName()) + return openai.FetchModelsDirectly(ctx, c.ApiBaseURL.Value, c.ApiKey.Value, c.GetName(), nil) } diff --git a/internal/plugins/ai/openai_compatible/providers_config.go b/internal/plugins/ai/openai_compatible/providers_config.go index 4de08701..492933ab 100644 --- a/internal/plugins/ai/openai_compatible/providers_config.go +++ b/internal/plugins/ai/openai_compatible/providers_config.go @@ -47,7 +47,7 @@ func (c *Client) ListModels() ([]string, error) { } // TODO: Handle context properly in Fabric by accepting and propagating a context.Context // instead of creating a new one here. - return openai.FetchModelsDirectly(context.Background(), c.modelsURL, c.Client.ApiKey.Value, c.GetName()) + return openai.FetchModelsDirectly(context.Background(), c.modelsURL, c.Client.ApiKey.Value, c.GetName(), nil) } // First try the standard OpenAI SDK approach diff --git a/internal/plugins/db/fsdb/patterns.go b/internal/plugins/db/fsdb/patterns.go index d1536add..4edd2562 100644 --- a/internal/plugins/db/fsdb/patterns.go +++ b/internal/plugins/db/fsdb/patterns.go @@ -65,7 +65,9 @@ func (o *PatternsEntity) loadPattern(source string) (pattern *Pattern, err error } // Use the resolved absolute path to get the pattern - pattern, _ = o.getFromFile(absPath) + if pattern, err = o.getFromFile(absPath); err != nil { + return nil, fmt.Errorf("could not load pattern from file %s: %w", absPath, err) + } } else { // Otherwise, get the pattern from the database pattern, err = o.getFromDB(source)