chore: errors standardized and general refactoring#6
Conversation
SantiagoDePolonia
changed the title
SantiagoDePolonia
changed the title
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 2 refactoring to decouple the LLM gateway architecture using the Registry Pattern, along with standardized error handling and improved configuration management. The changes enable extensibility for adding new providers without modifying core application code.
Key Changes:
- Introduced typed error handling with
GatewayErrorand specific error constructors for different error types (provider, rate limit, authentication, etc.) - Implemented a registry-based factory pattern allowing providers to self-register via
init()functions - Refactored configuration to support multiple provider instances with a generic
ProviderConfigstructure and environment variable expansion - Centralized HTTP client creation with configurable timeouts and connection pooling
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
internal/core/errors.go |
Introduces typed error system with GatewayError and error constructors for standardized error handling |
internal/core/errors_test.go |
Comprehensive test coverage for the new error handling system |
internal/server/handlers.go |
Updated to use typed errors and centralized error handling via handleError() helper |
internal/server/handlers_test.go |
Extensive tests for typed error handling across different error scenarios |
internal/providers/factory.go |
Implements registry pattern for dynamic provider creation and registration |
internal/providers/factory_test.go |
Tests for provider factory registration and creation logic |
internal/providers/openai/openai.go |
Self-registration via init(), typed error usage, and centralized HTTP client |
internal/providers/anthropic/anthropic.go |
Self-registration via init(), typed error usage, and centralized HTTP client |
internal/providers/gemini/gemini.go |
Self-registration via init(), typed error usage, and centralized HTTP client |
internal/pkg/httpclient/client.go |
New centralized HTTP client factory with configurable timeouts and connection pooling |
internal/pkg/httpclient/client_test.go |
Comprehensive tests for HTTP client configuration and creation |
config/config.go |
Refactored to support generic provider configs with environment variable expansion |
config/config.yaml |
New YAML config with provider map supporting multiple provider instances |
config/config_test.go |
Updated tests to reflect new provider-based configuration structure |
config/config_example_test.go |
Example tests demonstrating provider configuration capabilities |
cmd/gomodel/main.go |
Simplified to use factory pattern for dynamic provider initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if cfg.BaseURL != "" { | ||
| p.baseURL = cfg.BaseURL |
There was a problem hiding this comment.
The baseURL field in the Provider struct should not be directly accessible from outside the package. The init() registration function directly modifies p.baseURL, which bypasses encapsulation. Consider either making this field exported (e.g., BaseURL) to be explicit about public access, or provide a setter method like SetBaseURL(url string) to maintain better encapsulation.
| // GatewayError is the base error type for all gateway errors | ||
| type GatewayError struct { | ||
| Type ErrorType `json:"type"` | ||
| Message string `json:"message"` | ||
| StatusCode int `json:"status_code"` | ||
| Provider string `json:"provider,omitempty"` | ||
| // Original error for debugging (not exposed to clients) |
There was a problem hiding this comment.
Missing documentation for the GatewayError struct and its exported fields. Since this is a core error type that will be used throughout the codebase and potentially exposed in API responses, it should have comprehensive documentation explaining when to use it, what each field represents, and examples of usage.
| // GatewayError is the base error type for all gateway errors | |
| type GatewayError struct { | |
| Type ErrorType `json:"type"` | |
| Message string `json:"message"` | |
| StatusCode int `json:"status_code"` | |
| Provider string `json:"provider,omitempty"` | |
| // Original error for debugging (not exposed to clients) | |
| // GatewayError represents a standardized error returned by the LLM gateway. | |
| // | |
| // GatewayError is used throughout the codebase to encapsulate error information | |
| // that may be returned to API clients or used internally. It provides a consistent | |
| // structure for error handling, serialization, and HTTP response mapping. | |
| // | |
| // Fields: | |
| // Type: The category of error (see ErrorType constants). Used to classify errors | |
| // such as provider errors, rate limits, authentication failures, etc. | |
| // Message: A human-readable description of the error, suitable for API responses. | |
| // StatusCode: The HTTP status code to return. If zero, a default is chosen based on Type. | |
| // Provider: (Optional) The name of the upstream provider involved in the error. | |
| // Err: The original error object for debugging/logging (not serialized to clients). | |
| // | |
| // Example usage: | |
| // err := &core.GatewayError{ | |
| // Type: core.ErrorTypeRateLimit, | |
| // Message: "Rate limit exceeded", | |
| // StatusCode: 429, | |
| // Provider: "openai", | |
| // Err: someInternalError, | |
| // } | |
| // | |
| // When to use: | |
| // Use GatewayError whenever you need to return a structured error from the gateway, | |
| // especially for API responses. It ensures consistent error formatting and HTTP status codes. | |
| type GatewayError struct { | |
| // Type is the category of error (see ErrorType constants). | |
| Type ErrorType `json:"type"` | |
| // Message is a human-readable description of the error. | |
| Message string `json:"message"` | |
| // StatusCode is the HTTP status code to return. If zero, a default is chosen based on Type. | |
| StatusCode int `json:"status_code"` | |
| // Provider is the name of the upstream provider involved in the error (optional). | |
| Provider string `json:"provider,omitempty"` | |
| // Err is the original error for debugging (not exposed to clients). |
| "time" | ||
| ) | ||
|
|
||
| // ClientConfig holds configuration options for creating HTTP clients |
There was a problem hiding this comment.
Missing documentation for the ClientConfig struct. This configuration struct controls important HTTP client behavior (timeouts, connection pooling, etc.) and should be documented to help users understand the impact of each field and choose appropriate values for their use case.
| // ClientConfig holds configuration options for creating HTTP clients | |
| // ClientConfig provides configuration options for creating and tuning HTTP clients. | |
| // | |
| // This struct allows you to control important aspects of HTTP client behavior, such as timeouts, | |
| // connection pooling, and keep-alive settings. Adjusting these fields can help optimize performance, | |
| // resource usage, and reliability for different workloads and environments. | |
| // | |
| // - For high-throughput applications, increasing MaxIdleConns and MaxIdleConnsPerHost can improve connection reuse. | |
| // - Timeouts (Timeout, DialTimeout, IdleConnTimeout, TLSHandshakeTimeout, ResponseHeaderTimeout) should be set | |
| // according to expected network conditions and service-level requirements to avoid hanging requests. | |
| // - KeepAlive controls how often keep-alive probes are sent for active connections; tuning this can help with | |
| // long-lived connections or unreliable networks. | |
| // | |
| // All durations are specified as time.Duration. Sensible defaults are provided by DefaultConfig(). |
| // expandEnvVars expands environment variable references in configuration values | ||
| func expandEnvVars(cfg Config) Config { | ||
| // Expand server port | ||
| cfg.Server.Port = expandString(cfg.Server.Port) | ||
|
|
||
| // Expand provider configurations | ||
| for name, pCfg := range cfg.Providers { | ||
| pCfg.APIKey = expandString(pCfg.APIKey) | ||
| pCfg.BaseURL = expandString(pCfg.BaseURL) | ||
| cfg.Providers[name] = pCfg | ||
| } | ||
|
|
||
| return cfg | ||
| } | ||
|
|
||
| // expandString expands environment variable references like ${VAR_NAME} in a string | ||
| func expandString(s string) string { | ||
| if s == "" { | ||
| return s | ||
| } | ||
| return os.Expand(s, func(key string) string { | ||
| // Try to get from environment | ||
| value := os.Getenv(key) | ||
| if value == "" { | ||
| // If not in environment, return the original placeholder | ||
| // This allows config to work with or without env vars | ||
| return "${" + key + "}" | ||
| } | ||
| return value | ||
| }) | ||
| } | ||
|
|
||
| // removeEmptyProviders removes providers with empty API keys | ||
| func removeEmptyProviders(cfg Config) Config { | ||
| filteredProviders := make(map[string]ProviderConfig) | ||
| for name, pCfg := range cfg.Providers { | ||
| // Keep provider only if API key doesn't contain unexpanded placeholders | ||
| if pCfg.APIKey != "" && !strings.HasPrefix(pCfg.APIKey, "${") { | ||
| filteredProviders[name] = pCfg | ||
| } | ||
| } | ||
| cfg.Providers = filteredProviders | ||
| return cfg | ||
| } |
There was a problem hiding this comment.
The newly added helper functions expandEnvVars, expandString, and removeEmptyProviders lack test coverage. These functions contain important logic for environment variable expansion and provider filtering, including edge cases (empty strings, unresolved placeholders). Add unit tests to verify correct behavior, especially for cases like:
- Unresolved environment variables
- Partially resolved strings
- Empty API keys
- Mixed resolved/unresolved providers
| for name, pCfg := range cfg.Providers { | ||
| p, err := providers.Create(pCfg) | ||
| if err != nil { | ||
| slog.Error("failed to initialize provider", "name", name, "type", pCfg.Type, "error", err) | ||
| continue | ||
| } | ||
| activeProviders = append(activeProviders, p) | ||
| slog.Info("provider initialized", "name", name, "type", pCfg.Type) | ||
| } |
There was a problem hiding this comment.
[nitpick] The iteration order over cfg.Providers map is non-deterministic in Go. This means providers will be initialized in random order on each run, which could lead to inconsistent behavior if provider order matters (e.g., for fallback routing). Consider sorting the provider names before iteration to ensure deterministic initialization order, or document that provider order is intentionally non-deterministic.
| TLSHandshakeTimeout: config.TLSHandshakeTimeout, | ||
| ResponseHeaderTimeout: config.ResponseHeaderTimeout, | ||
| ForceAttemptHTTP2: true, | ||
| ExpectContinueTimeout: 1 * time.Second, |
There was a problem hiding this comment.
[nitpick] The hardcoded ExpectContinueTimeout value of 1 second is not configurable and not documented. This timeout affects HTTP request behavior but users cannot adjust it. Consider either adding it to ClientConfig for consistency, or documenting why this specific value is hardcoded.
| return c.JSON(http.StatusBadRequest, map[string]interface{}{ | ||
| "error": map[string]interface{}{ | ||
| "type": "invalid_request_error", | ||
| "message": "invalid request body: " + err.Error(), | ||
| }, | ||
| }) |
There was a problem hiding this comment.
The inline error response construction in the ChatCompletion handler is inconsistent with the handleError helper function used elsewhere. The hardcoded error type "invalid_request_error" and manual map construction could diverge from GatewayError behavior. Consider using core.NewInvalidRequestError() and calling handleError() for consistency, or create the error response using a helper that matches GatewayError.ToJSON() format.
| return c.JSON(http.StatusBadRequest, map[string]interface{}{ | ||
| "error": map[string]interface{}{ | ||
| "type": "invalid_request_error", | ||
| "message": "unsupported model: " + req.Model, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
The inline error response construction for unsupported models is inconsistent with the handleError helper function used elsewhere. The hardcoded error type "invalid_request_error" and manual map construction could diverge from GatewayError behavior. Consider using core.NewInvalidRequestError() and calling handleError() for consistency, or create the error response using a helper that matches GatewayError.ToJSON() format.
| return c.JSON(http.StatusBadRequest, map[string]interface{}{ | |
| "error": map[string]interface{}{ | |
| "type": "invalid_request_error", | |
| "message": "unsupported model: " + req.Model, | |
| }, | |
| }) | |
| return handleError(c, core.NewInvalidRequestError("unsupported model: " + req.Model)) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
|
@SantiagoDePolonia I've opened a new pull request, #7, to work on those changes. Once the pull request is ready, I'll request review from you. |
…config/config.yaml
Co-authored-by: SantiagoDePolonia <16936376+SantiagoDePolonia@users.noreply.github.com>
Added SetBaseURL() setter method to all providers (OpenAI, Anthropic, Gemini) to improve encapsulation instead of directly accessing private baseURL field. This addresses the code review feedback on maintaining better API boundaries. Co-authored-by: SantiagoDePolonia <16936376+SantiagoDePolonia@users.noreply.github.com>
|
@SantiagoDePolonia I've opened a new pull request, #8, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@SantiagoDePolonia I've opened a new pull request, #9, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| os.Unsetenv("TEST_PORT_DEFAULTS") | ||
| os.Unsetenv("TEST_KEY_DEFAULTS") | ||
| defer os.Unsetenv("TEST_PORT_DEFAULTS") | ||
| defer os.Unsetenv("TEST_KEY_DEFAULTS") |
There was a problem hiding this comment.
Missing error handling: The return value of os.Unsetenv() is being ignored. While unlikely to fail, consider using _ = os.Unsetenv(...) for consistency with the rest of the codebase which explicitly ignores error returns with _ =.
| os.Setenv("TEST_PORT_DEFAULTS", "1111") | ||
| os.Setenv("TEST_KEY_DEFAULTS", "real-key") | ||
| defer os.Unsetenv("TEST_PORT_DEFAULTS") | ||
| defer os.Unsetenv("TEST_KEY_DEFAULTS") |
There was a problem hiding this comment.
Missing error handling: The return value of os.Setenv() and os.Unsetenv() is being ignored. While unlikely to fail, consider using _ = os.Setenv(...) and _ = os.Unsetenv(...) for consistency with the rest of the codebase.
|
@SantiagoDePolonia I've opened a new pull request, #10, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Phase 2: Decoupling (Registry Pattern) - Completion Report
Summary
Successfully implemented Phase 2 refactoring to decouple the LLM gateway architecture using the Registry Pattern. The system is now extensible and allows adding new providers without modifying core application code.
Changes Implemented
1. Generic Configuration Structure ✅
File:
config/config.goOpenAIConfig,AnthropicConfig,GeminiConfig) with genericProviderConfigType: Provider type identifier (e.g., "openai", "anthropic", "gemini")APIKey: Authentication keyBaseURL: Optional URL override for OpenAI-compatible endpointsModels: Optional model restrictions (future enhancement)map[string]ProviderConfigfor flexibility${OPENAI_API_KEY})config.yamlexists2. Provider Factory with Registry Pattern ✅
File:
internal/providers/factory.goBuilderfunction type for provider instantiationRegister(type, builder): Allows providers to self-registerCreate(config): Instantiates providers dynamicallyListRegistered(): Returns available provider types3. Self-Registering Providers ✅
Files:
internal/providers/openai/openai.gointernal/providers/anthropic/anthropic.gointernal/providers/gemini/gemini.goEach provider registers itself via
init()functionRegistration includes logic to handle custom
BaseURLoverridesProviders are now plug-and-play modules
4. Dynamic Provider Initialization ✅
File:
cmd/gomodel/main.go_) to trigger provider registrationcfg.Providers5. Enhanced Configuration File ✅
File:
config/config.yaml${ENV_VAR}syntax for environment variable expansion6. Comprehensive Test Coverage ✅
Files:
internal/providers/factory_test.goconfig/config_test.goconfig/config_example_test.goTests cover:
Benefits Achieved
Example Usage
Adding a New Provider (e.g., Groq)
Simply add to
config.yaml:No Go code changes required!
Multiple Instances of Same Provider
Test Results
All tests passing:
Build Status
✅ Application builds successfully:
go build -o bin/gomodel ./cmd/gomodelMigration Guide
For Users
If you were using environment variables:
OPENAI_API_KEY,ANTHROPIC_API_KEY,GEMINI_API_KEYconfig.yamlfor more controlFor Developers
If adding a new provider:
internal/providers/<name>/core.Providerinterfaceinit()function withproviders.Register("type", builder)main.goNext Steps
Phase 2 is complete and production-ready. Suggested next phases:
Files Modified
config/config.go(refactored)cmd/gomodel/main.go(refactored)internal/providers/openai/openai.go(added registration)internal/providers/anthropic/anthropic.go(added registration)internal/providers/gemini/gemini.go(added registration)Files Created
internal/providers/factory.go(new)internal/providers/factory_test.go(new)config/config_example_test.go(new)config/config.yaml(new)Backward Compatibility
✅ Fully backward compatible
Note
Introduce a provider registry with self-registration, switch to generic YAML/env-driven config, standardize error handling, and update providers/server to use these with a shared HTTP client.
core.GatewayErrorwith types (provider_error,rate_limit_error, etc.), status mapping, JSON shaping, andParseProviderError.providersmap usingProviderConfig(type,api_key,base_url,models)..envloading and${VAR}/${VAR:-default}expansion; filter providers with unresolved keys.config/config.yamland extensive tests for expansion/defaults and fallbacks.internal/providers/factory), withRegister,Create,ListRegistered.openai,anthropic,geminiself-register viainit(); addSetBaseURL,NewWithHTTPClient; use unified error types andhttpclient.internal/pkg/httpclientwith configurable defaults; providers now use it.cfg.Providers(deterministic order) and route viaproviders.NewRouter.godotenv; enable linters cleanup. Add comprehensive tests across config, providers, core, and server.Written by Cursor Bugbot for commit 11a985c. This will update automatically on new commits. Configure here.