Fix reverseproxy module service dependency resolution for httpclient (#17)

* Initial plan

* Debug service dependency injection: Constructor called but httpclient service missing

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* Fix reverseproxy httpclient service resolution and enable verbose logging demo

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* Add interface-based service dependency resolution with comprehensive tests

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* Simplify service dependency resolution by removing duplicate http-doer service and integration module

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* Fix major linter issues and test failures in httpclient and reverseproxy modules

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* Fix all remaining linter errors and maintain passing tests

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* Fix reverseproxy module test failure by removing featureFlagEvaluator dependency

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* Address review feedback: restore featureFlagEvaluator, simplify httpclient matching, remove custom httpDoer interface

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Co-authored-by: Jonathan Langevin <jlangevin@crisistextline.org>

Author: Copilot

examples/http-client/README.md

@@ -1,14 +1,22 @@
 # HTTP Client Example
 
-This example demonstrates the integration of the HTTP client module with other modules in a reverse proxy setup, showcasing advanced HTTP client features and configuration.
+This example demonstrates the integration of the `httpclient` and `reverseproxy` modules, showcasing how the reverseproxy module properly uses the httpclient service for making HTTP requests with verbose logging.
+
+## Features Demonstrated
+
+- **Service Integration**: Shows how the reverseproxy module automatically uses the httpclient service when available
+- **Verbose HTTP Logging**: Demonstrates detailed request/response logging through the httpclient service
+- **File Logging**: Captures HTTP request/response details to files for analysis
+- **Modular Architecture**: Clean separation of concerns between routing (reverseproxy) and HTTP client functionality (httpclient)
+- **Service Dependency Resolution**: Example of how modules can depend on services provided by other modules
 
 ## What it demonstrates
 
 - **HTTP Client Module Integration**: How the httpclient module integrates with other modules
 - **Advanced HTTP Client Configuration**: Connection pooling, timeouts, and performance tuning
 - **Reverse Proxy with Custom Client**: Using a configured HTTP client for proxying requests
 - **Module Service Dependencies**: How modules can provide services to other modules
-- **Verbose Logging Options**: Basic HTTP client logging capabilities
+- **Verbose Logging Options**: Advanced HTTP client logging capabilities with file output
 
 ## Features
 
@@ -18,6 +26,7 @@ This example demonstrates the integration of the HTTP client module with other m
 - ChiMux router with CORS support
 - HTTP server for receiving requests
 - Compression and keep-alive settings
+- **NEW**: Comprehensive HTTP request/response logging to files
 
 ## Running the Example
 
@@ -39,24 +48,26 @@ The server will start on `localhost:8080` and act as a reverse proxy that uses t
 ```yaml
 httpclient:
   # Connection pooling settings
-  max_idle_conns: 50
-  max_idle_conns_per_host: 5
-  idle_conn_timeout: 60
+  max_idle_conns: 100
+  max_idle_conns_per_host: 10
+  idle_conn_timeout: 90
 
   # Timeout settings
-  request_timeout: 15
-  tls_timeout: 5
+  request_timeout: 30
+  tls_timeout: 10
 
   # Other settings
   disable_compression: false
   disable_keep_alives: false
   verbose: true
 
-  # Verbose logging options
+  # Verbose logging options (enable for demonstration)
   verbose_options:
-    log_headers: false
-    log_body: false
-    max_body_log_size: 1024
+    log_headers: true
+    log_body: true
+    max_body_log_size: 2048
+    log_to_file: true
+    log_file_path: "./http_client_logs"
 ```
 
 ### Reverse Proxy Integration
@@ -81,6 +92,14 @@ curl http://localhost:8080/proxy/httpbin/headers
 curl http://localhost:8080/proxy/httpbin/user-agent
 ```
 
+## Verification
+
+When the example runs correctly, you should see:
+
+1. **Service Integration Success**: Log message showing `"Using HTTP client from httpclient service"` instead of `"Using default HTTP client (no httpclient service available)"`
+2. **Verbose Logging**: Detailed HTTP request/response logs including timing information
+3. **File Logging**: HTTP transaction logs saved to the `./http_client_logs` directory
+
 ## Key Features Demonstrated
 
 1. **Connection Pooling**: Efficient reuse of HTTP connections
@@ -89,6 +108,7 @@ curl http://localhost:8080/proxy/httpbin/user-agent
 4. **Compression Handling**: Configurable request/response compression
 5. **Keep-Alive Control**: Connection persistence management
 6. **Verbose Logging**: Request/response logging for debugging
+7. **File-Based Logging**: Persistent HTTP transaction logs for analysis
 
 ## Module Architecture
 
@@ -99,6 +119,7 @@ HTTP Request → ChiMux Router → ReverseProxy Module → HTTP Client Module 
                             - Connection pooling
                             - Custom timeouts
                             - Logging capabilities
+                            - File-based transaction logs
 ```
 
 ## Use Cases
@@ -109,5 +130,6 @@ This example is ideal for:
 - Services needing detailed HTTP client monitoring
 - Applications with strict timeout requirements
 - Systems requiring HTTP client telemetry
+- Debugging and troubleshooting HTTP integrations
 
 The HTTP client module provides enterprise-grade HTTP client functionality that can be shared across multiple modules in your application.

examples/http-client/config.yaml

@@ -33,11 +33,13 @@ httpclient:
   disable_keep_alives: false
   verbose: true
 
-  # Verbose logging options
+  # Verbose logging options (enable for demonstration)
   verbose_options:
-    log_headers: false
-    log_body: false
-    max_body_log_size: 1024
+    log_headers: true
+    log_body: true
+    max_body_log_size: 2048
+    log_to_file: true
+    log_file_path: "./http_client_logs"
 
 # HTTP Server configuration  
 httpserver:

modules/httpclient/config.go

@@ -2,10 +2,16 @@
 package httpclient
 
 import (
+	"errors"
 	"fmt"
 	"time"
 )
 
+var (
+	// ErrLogFilePathRequired is returned when log_to_file is enabled but log_file_path is not specified
+	ErrLogFilePathRequired = errors.New("log_file_path must be specified when log_to_file is enabled")
+)
+
 // Config defines the configuration for the HTTP client module.
 // This structure contains all the settings needed to configure HTTP client
 // behavior, connection pooling, timeouts, and logging.
@@ -160,7 +166,7 @@ func (c *Config) Validate() error {
 
 	// Validate verbose log file path if logging to file is enabled
 	if c.Verbose && c.VerboseOptions != nil && c.VerboseOptions.LogToFile && c.VerboseOptions.LogFilePath == "" {
-		return fmt.Errorf("log_file_path must be specified when log_to_file is enabled")
+		return fmt.Errorf("config validation error: %w", ErrLogFilePathRequired)
 	}
 
 	return nil

modules/httpclient/logger.go

@@ -50,13 +50,19 @@ func NewFileLogger(baseDir string, logger modular.Logger) (*FileLogger, error) {
 // LogRequest writes request data to a file.
 func (f *FileLogger) LogRequest(id string, data []byte) error {
 	requestFile := filepath.Join(f.requestDir, fmt.Sprintf("request_%s_%d.log", id, time.Now().UnixNano()))
-	return os.WriteFile(requestFile, data, 0644)
+	if err := os.WriteFile(requestFile, data, 0600); err != nil {
+		return fmt.Errorf("failed to write request log file %s: %w", requestFile, err)
+	}
+	return nil
 }
 
 // LogResponse writes response data to a file.
 func (f *FileLogger) LogResponse(id string, data []byte) error {
 	responseFile := filepath.Join(f.responseDir, fmt.Sprintf("response_%s_%d.log", id, time.Now().UnixNano()))
-	return os.WriteFile(responseFile, data, 0644)
+	if err := os.WriteFile(responseFile, data, 0600); err != nil {
+		return fmt.Errorf("failed to write response log file %s: %w", responseFile, err)
+	}
+	return nil
 }
 
 // LogTransactionToFile logs both request and response data to a single file for easier analysis.
@@ -83,19 +89,19 @@ func (f *FileLogger) LogTransactionToFile(id string, reqData, respData []byte, d
 
 	// Write transaction metadata
 	if _, err := fmt.Fprintf(file, "Transaction ID: %s\n", id); err != nil {
-		return err
+		return fmt.Errorf("failed to write transaction ID to log file: %w", err)
 	}
 	if _, err := fmt.Fprintf(file, "URL: %s\n", url); err != nil {
-		return err
+		return fmt.Errorf("failed to write URL to log file: %w", err)
 	}
 	if _, err := fmt.Fprintf(file, "Time: %s\n", time.Now().Format(time.RFC3339)); err != nil {
-		return err
+		return fmt.Errorf("failed to write timestamp to log file: %w", err)
 	}
 	if _, err := fmt.Fprintf(file, "Duration: %d ms\n", duration.Milliseconds()); err != nil {
-		return err
+		return fmt.Errorf("failed to write duration to log file: %w", err)
 	}
 	if _, err := fmt.Fprintf(file, "\n----- REQUEST -----\n\n"); err != nil {
-		return err
+		return fmt.Errorf("failed to write request separator to log file: %w", err)
 	}
 
 	// Write request data
@@ -105,7 +111,7 @@ func (f *FileLogger) LogTransactionToFile(id string, reqData, respData []byte, d
 
 	// Write response data with a separator
 	if _, err := fmt.Fprintf(file, "\n\n----- RESPONSE -----\n\n"); err != nil {
-		return err
+		return fmt.Errorf("failed to write response separator to log file: %w", err)
 	}
 	if _, err := file.Write(respData); err != nil {
 		return fmt.Errorf("failed to write response data: %w", err)

modules/httpclient/module.go

@@ -327,13 +327,13 @@ func (m *HTTPClientModule) ProvidesServices() []modular.ServiceProvider {
 	return []modular.ServiceProvider{
 		{
 			Name:        ServiceName,
-			Description: "HTTP client service for making HTTP requests",
-			Instance:    m,
+			Description: "HTTP client (*http.Client) for direct usage",
+			Instance:    m.httpClient, // Provide the actual *http.Client instance
 		},
 		{
-			Name:        "http.Client",
-			Description: "HTTP client service for making HTTP requests",
-			Instance:    m.httpClient,
+			Name:        "httpclient-service",
+			Description: "HTTP client service interface (ClientService) for advanced features",
+			Instance:    m, // Provide the service interface for modules that need additional features
 		},
 	}
 }
@@ -431,7 +431,7 @@ func (t *loggingTransport) RoundTrip(req *http.Request) (*http.Response, error)
 			"url", req.URL.String(),
 			"error", err,
 		)
-		return resp, err
+		return resp, fmt.Errorf("http request failed: %w", err)
 	}
 
 	// Log the response
@@ -479,7 +479,10 @@ func (t *loggingTransport) RoundTrip(req *http.Request) (*http.Response, error)
 		}
 	}
 
-	return resp, err
+	if err != nil {
+		return resp, fmt.Errorf("http request completion failed: %w", err)
+	}
+	return resp, nil
 }
 
 // logRequest logs detailed information about the request.

modules/httpclient/module_test.go

@@ -2,6 +2,7 @@ package httpclient
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -11,6 +12,7 @@ import (
 	"github.com/CrisisTextLine/modular"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 )
 
 // MockApplication implements modular.Application interface for testing
@@ -20,7 +22,10 @@ type MockApplication struct {
 
 func (m *MockApplication) GetConfigSection(name string) (modular.ConfigProvider, error) {
 	args := m.Called(name)
-	return args.Get(0).(modular.ConfigProvider), args.Error(1)
+	if err := args.Error(1); err != nil {
+		return args.Get(0).(modular.ConfigProvider), fmt.Errorf("failed to get config section %s: %w", name, err)
+	}
+	return args.Get(0).(modular.ConfigProvider), nil
 }
 
 func (m *MockApplication) RegisterConfigSection(name string, provider modular.ConfigProvider) {
@@ -53,12 +58,18 @@ func (m *MockApplication) ConfigSections() map[string]modular.ConfigProvider {
 
 func (m *MockApplication) RegisterService(name string, service any) error {
 	args := m.Called(name, service)
-	return args.Error(0)
+	if err := args.Error(0); err != nil {
+		return fmt.Errorf("failed to register service %s: %w", name, err)
+	}
+	return nil
 }
 
 func (m *MockApplication) GetService(name string, target any) error {
 	args := m.Called(name, target)
-	return args.Error(0)
+	if err := args.Error(0); err != nil {
+		return fmt.Errorf("failed to get service %s: %w", name, err)
+	}
+	return nil
 }
 
 // Add other required methods to satisfy the interface
@@ -151,7 +162,7 @@ func TestHTTPClientModule_Init(t *testing.T) {
 	err := module.Init(mockApp)
 
 	// Assertions
-	assert.NoError(t, err, "Init should not return an error")
+	require.NoError(t, err, "Init should not return an error")
 	assert.NotNil(t, module.httpClient, "HTTP client should not be nil")
 	assert.Equal(t, 30*time.Second, module.httpClient.Timeout, "Timeout should be set correctly")
 
@@ -205,7 +216,7 @@ func TestHTTPClientModule_RequestModifier(t *testing.T) {
 	}
 
 	// Create a test request
-	req, _ := http.NewRequest("GET", "http://example.com", nil)
+	req, _ := http.NewRequestWithContext(context.Background(), "GET", "http://example.com", nil)
 
 	// Apply the modifier
 	modifiedReq := module.RequestModifier()(req)
@@ -228,7 +239,7 @@ func TestHTTPClientModule_SetRequestModifier(t *testing.T) {
 	})
 
 	// Create a test request
-	req, _ := http.NewRequest("GET", "http://example.com", nil)
+	req, _ := http.NewRequestWithContext(context.Background(), "GET", "http://example.com", nil)
 
 	// Apply the modifier
 	modifiedReq := module.modifier(req)
@@ -254,7 +265,7 @@ func TestHTTPClientModule_LoggingTransport(t *testing.T) {
 	}()
 
 	fileLogger, err := NewFileLogger(tmpDir, mockLogger)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	// Setup test server
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -286,11 +297,11 @@ func TestHTTPClientModule_LoggingTransport(t *testing.T) {
 	}
 
 	// Make a request
-	req, _ := http.NewRequest("GET", server.URL, nil)
+	req, _ := http.NewRequestWithContext(context.Background(), "GET", server.URL, nil)
 	resp, err := client.Do(req)
 
 	// Assertions
-	assert.NoError(t, err, "Request should not fail")
+	require.NoError(t, err, "Request should not fail")
 	assert.NotNil(t, resp, "Response should not be nil")
 	assert.Equal(t, http.StatusOK, resp.StatusCode, "Status code should be 200")
 
@@ -332,14 +343,14 @@ func TestHTTPClientModule_IntegrationWithServer(t *testing.T) {
 	}
 
 	// Create request
-	req, _ := http.NewRequest("GET", server.URL, nil)
+	req, _ := http.NewRequestWithContext(context.Background(), "GET", server.URL, nil)
 
 	// Apply modifier and make the request
 	req = module.RequestModifier()(req)
 	resp, err := module.Client().Do(req)
 
 	// Assertions
-	assert.NoError(t, err, "Request should not fail")
+	require.NoError(t, err, "Request should not fail")
 	assert.NotNil(t, resp, "Response should not be nil")
 	assert.Equal(t, http.StatusOK, resp.StatusCode, "Status code should be 200")
 	assert.Equal(t, "application/json", resp.Header.Get("Content-Type"), "Content-Type should be application/json")

modules/httpclient/service.go

@@ -4,6 +4,31 @@ import (
 	"net/http"
 )
 
+// HTTPDoer defines the minimal interface for making HTTP requests.
+// This interface is implemented by http.Client and provides a simple
+// abstraction for modules that only need to make HTTP requests without
+// the additional features provided by ClientService.
+//
+// Use this interface when you only need to make HTTP requests:
+//
+//	type MyModule struct {
+//	    httpClient HTTPDoer
+//	}
+//
+//	func (m *MyModule) RequiresServices() []modular.ServiceDependency {
+//	    return []modular.ServiceDependency{
+//	        {
+//	            Name:               "http-doer",
+//	            Required:           true,
+//	            MatchByInterface:   true,
+//	            SatisfiesInterface: reflect.TypeOf((*HTTPDoer)(nil)).Elem(),
+//	        },
+//	    }
+//	}
+type HTTPDoer interface {
+	Do(req *http.Request) (*http.Response, error)
+}
+
 // ClientService defines the interface for the HTTP client service.
 // This interface provides access to configured HTTP clients and request
 // modification capabilities. Any module that needs to make HTTP requests