Refactor APIHandler interface and LoadAPIHandler function

Author: ShocOne

internal/apihandlers/jamfpro/jamfpro_api_handler.go

@@ -48,6 +48,9 @@ import (
 	"strings"
 
 	_ "embed"
+
+	"github.com/deploymenttheory/go-api-http-client/internal/logger"
+	"go.uber.org/zap"
 )
 
 // Endpoint constants represent the URL suffixes used for Jamf API token interactions.
@@ -124,19 +127,19 @@ func (j *JamfAPIHandler) GetBaseDomain() string {
 	return DefaultBaseDomain
 }
 
-// ConstructAPIResourceEndpoint returns the full URL for a Jamf API resource endpoint path.
-func (j *JamfAPIHandler) ConstructAPIResourceEndpoint(endpointPath string, logger Logger) string {
+// ConstructAPIResourceEndpoint constructs the full URL for a Jamf API resource endpoint path and logs the URL.
+func (j *JamfAPIHandler) ConstructAPIResourceEndpoint(endpointPath string, log logger.Logger) string {
 	baseDomain := j.GetBaseDomain()
 	url := fmt.Sprintf("https://%s%s%s", j.InstanceName, baseDomain, endpointPath)
-	logger.Info("Request will be made to API   URL:", "URL", url)
+	log.Info("Constructed API resource endpoint URL", zap.String("URL", url))
 	return url
 }
 
-// ConstructAPIAuthEndpoint returns the full URL for a Jamf API auth endpoint path.
-func (j *JamfAPIHandler) ConstructAPIAuthEndpoint(endpointPath string, logger Logger) string {
+// ConstructAPIAuthEndpoint constructs the full URL for a Jamf API auth endpoint path and logs the URL.
+func (j *JamfAPIHandler) ConstructAPIAuthEndpoint(endpointPath string, log logger.Logger) string {
 	baseDomain := j.GetBaseDomain()
 	url := fmt.Sprintf("https://%s%s%s", j.InstanceName, baseDomain, endpointPath)
-	logger.Info("Request will be made to API authentication URL:", "URL", url)
+	log.Info("Constructed API authentication URL", zap.String("URL", url))
 	return url
 }
 
@@ -148,36 +151,36 @@ func (j *JamfAPIHandler) ConstructAPIAuthEndpoint(endpointPath string, logger Lo
 // - For url endpoints starting with "/api", it defaults to "application/json" for the JamfPro API.
 // If the endpoint does not match any of the predefined patterns, "application/json" is used as a fallback.
 // This method logs the decision process at various stages for debugging purposes.
-func (u *JamfAPIHandler) GetContentTypeHeader(endpoint string, logger Logger) string {
+func (u *JamfAPIHandler) GetContentTypeHeader(endpoint string, log logger.Logger) string {
 	// Dynamic lookup from configuration should be the first priority
 	for key, config := range configMap {
 		if strings.HasPrefix(endpoint, key) {
 			if config.ContentType != nil {
-				logger.Debug("Content-Type for endpoint found in configMap", "endpoint", endpoint, "content_type", *config.ContentType)
+				log.Debug("Content-Type for endpoint found in configMap", zap.String("endpoint", endpoint), zap.String("content_type", *config.ContentType))
 				return *config.ContentType
 			}
-			logger.Debug("Content-Type for endpoint is nil in configMap, handling as special case", "endpoint", endpoint)
+			log.Debug("Content-Type for endpoint is nil in configMap, handling as special case", zap.String("endpoint", endpoint))
 			// If a nil ContentType is an expected case, do not set Content-Type header.
 			return "" // Return empty to indicate no Content-Type should be set.
 		}
 	}
 
 	// If no specific configuration is found, then check for standard URL patterns.
 	if strings.Contains(endpoint, "/JSSResource") {
-		logger.Debug("Content-Type for endpoint defaulting to XML for Classic API", "endpoint", endpoint)
+		log.Debug("Content-Type for endpoint defaulting to XML for Classic API", zap.String("endpoint", endpoint))
 		return "application/xml" // Classic API uses XML
 	} else if strings.Contains(endpoint, "/api") {
-		logger.Debug("Content-Type for endpoint defaulting to JSON for JamfPro API", "endpoint", endpoint)
+		log.Debug("Content-Type for endpoint defaulting to JSON for JamfPro API", zap.String("endpoint", endpoint))
 		return "application/json" // JamfPro API uses JSON
 	}
 
 	// Fallback to JSON if no other match is found.
-	logger.Debug("Content-Type for endpoint not found in configMap or standard patterns, using default JSON", "endpoint", endpoint)
+	log.Debug("Content-Type for endpoint not found in configMap or standard patterns, using default JSON", zap.String("endpoint", endpoint))
 	return "application/json"
 }
 
 // MarshalRequest encodes the request body according to the endpoint for the API.
-func (u *JamfAPIHandler) MarshalRequest(body interface{}, method string, endpoint string, logger Logger) ([]byte, error) {
+func (u *JamfAPIHandler) MarshalRequest(body interface{}, method string, endpoint string, log logger.Logger) ([]byte, error) {
 	var (
 		data []byte
 		err  error
@@ -199,47 +202,47 @@ func (u *JamfAPIHandler) MarshalRequest(body interface{}, method string, endpoin
 		}
 
 		if method == "POST" || method == "PUT" {
-			logger.Debug("XML Request Body:", "Body", string(data))
+			log.Debug("XML Request Body", zap.String("Body", string(data)))
 		}
 
 	case "json":
 		data, err = json.Marshal(body)
 		if err != nil {
-			logger.Error("Failed marshaling JSON request", "error", err)
+			log.Error("Failed marshaling JSON request", zap.Error(err))
 			return nil, err
 		}
 
 		if method == "POST" || method == "PUT" || method == "PATCH" {
-			logger.Debug("JSON Request Body:", string(data))
+			log.Debug("JSON Request Body", zap.String("Body", string(data)))
 		}
 	}
 
 	return data, nil
 }
 
 // UnmarshalResponse decodes the response body from XML or JSON format depending on the Content-Type header.
-func (u *JamfAPIHandler) UnmarshalResponse(resp *http.Response, out interface{}, logger Logger) error {
+func (u *JamfAPIHandler) UnmarshalResponse(resp *http.Response, out interface{}, log logger.Logger) error {
 	// Handle DELETE method
 	if resp.Request.Method == "DELETE" {
 		if resp.StatusCode >= 200 && resp.StatusCode < 300 {
 			return nil
 		} else {
-			return fmt.Errorf("DELETE request failed with status code: %d", resp.StatusCode)
+			return log.Error("DELETE request failed", zap.Int("Status Code", resp.StatusCode))
 		}
 	}
 
 	bodyBytes, err := io.ReadAll(resp.Body)
 	if err != nil {
-		logger.Error("Failed reading response body", "error", err)
+		log.Error("Failed reading response body", zap.Error(err))
 		return err
 	}
 
 	// Log the raw response body and headers
-	logger.Trace("Raw HTTP Response:", string(bodyBytes))
-	logger.Debug("Unmarshaling response", "status", resp.Status)
+	log.Debug("Raw HTTP Response", zap.String("Body", string(bodyBytes)))
+	log.Debug("Unmarshaling response", zap.String("status", resp.Status))
 
 	// Log headers when in debug mode
-	logger.Debug("HTTP Response Headers:", resp.Header)
+	log.Debug("HTTP Response Headers", zap.Any("Headers", resp.Header))
 
 	// Check the Content-Type and Content-Disposition headers
 	contentType := resp.Header.Get("Content-Type")
@@ -253,7 +256,7 @@ func (u *JamfAPIHandler) UnmarshalResponse(resp *http.Response, out interface{},
 	// If content type is HTML, extract the error message
 	if strings.Contains(contentType, "text/html") {
 		errMsg := ExtractErrorMessageFromHTML(string(bodyBytes))
-		logger.Warn("Received HTML content", "error_message", errMsg, "status_code", resp.StatusCode)
+		log.Warn("Received HTML content", "error_message", errMsg, "status_code", resp.StatusCode)
 		return &APIError{
 			StatusCode: resp.StatusCode,
 			Message:    errMsg,
@@ -266,14 +269,14 @@ func (u *JamfAPIHandler) UnmarshalResponse(resp *http.Response, out interface{},
 		if strings.Contains(contentType, "application/json") {
 			description, err := ParseJSONErrorResponse(bodyBytes)
 			if err != nil {
-				u.logger.Error("Failed to parse JSON error response", "error", err)
+				log.Error("Failed to parse JSON error response", "error", err)
 				return fmt.Errorf("received non-success status code: %d and failed to parse error response", resp.StatusCode)
 			}
 			return fmt.Errorf("received non-success status code: %d, error: %s", resp.StatusCode, description)
 		}
 
 		// If the response is not JSON or another error occurs, return a generic error message
-		u.logger.Error("Received non-success status code", "status_code", resp.StatusCode)
+		log.Error("Received non-success status code", "status_code", resp.StatusCode)
 		return fmt.Errorf("received non-success status code: %d", resp.StatusCode)
 	}
 
@@ -293,12 +296,12 @@ func (u *JamfAPIHandler) UnmarshalResponse(resp *http.Response, out interface{},
 		// If unmarshalling fails, check if the content might be HTML
 		if strings.Contains(string(bodyBytes), "<html>") {
 			errMsg := ExtractErrorMessageFromHTML(string(bodyBytes))
-			logger.Warn("Received HTML content instead of expected format", "error_message", errMsg, "status_code", resp.StatusCode)
+			log.Warn("Received HTML content instead of expected format", "error_message", errMsg, "status_code", resp.StatusCode)
 			return fmt.Errorf(errMsg)
 		}
 
 		// Log the error and return it
-		logger.Error("Failed to unmarshal response", "error", err)
+		log.Error("Failed to unmarshal response", "error", err)
 		return fmt.Errorf("failed to unmarshal response: %v", err)
 	}
 
@@ -331,7 +334,7 @@ func (u *JamfAPIHandler) GetAcceptHeader() string {
 }
 
 // MarshalMultipartFormData takes a map with form fields and file paths and returns the encoded body and content type.
-func (u *JamfAPIHandler) MarshalMultipartRequest(fields map[string]string, files map[string]string) ([]byte, string, error) {
+func (u *JamfAPIHandler) MarshalMultipartRequest(fields map[string]string, files map[string]string, log logger.Logger) ([]byte, string, error) {
 	body := &bytes.Buffer{}
 	writer := multipart.NewWriter(body)
 

internal/httpclient/api_handler.go

@@ -2,47 +2,46 @@
 package httpclient
 
 import (
-	"fmt"
 	"net/http"
 
 	"github.com/deploymenttheory/go-api-http-client/internal/apihandlers/jamfpro"
+	"github.com/deploymenttheory/go-api-http-client/internal/logger"
+	"go.uber.org/zap"
 )
 
 // APIHandler is an interface for encoding, decoding, and determining content types for different API implementations.
 // It encapsulates behavior for encoding and decoding requests and responses.
 type APIHandler interface {
 	GetBaseDomain() string
-	ConstructAPIResourceEndpoint(endpointPath string) string
-	ConstructAPIAuthEndpoint(endpointPath string, logger Logger) string
-	MarshalRequest(body interface{}, method string, endpoint string, logger Logger) ([]byte, error)
-	MarshalMultipartRequest(fields map[string]string, files map[string]string, logger Logger) ([]byte, string, error)
-	UnmarshalResponse(resp *http.Response, out interface{}, logger Logger) error
-	GetContentTypeHeader(method string, logger Logger) string
-	GetAcceptHeader(logger Logger) string
+	ConstructAPIResourceEndpoint(endpointPath string, log logger.Logger) string
+	ConstructAPIAuthEndpoint(endpointPath string, log logger.Logger) string
+	MarshalRequest(body interface{}, method string, endpoint string, log logger.Logger) ([]byte, error)
+	MarshalMultipartRequest(fields map[string]string, files map[string]string, log logger.Logger) ([]byte, string, error)
+	UnmarshalResponse(resp *http.Response, out interface{}, log logger.Logger) error
+	GetContentTypeHeader(method string, log logger.Logger) string
+	GetAcceptHeader() string
 }
 
 // LoadAPIHandler returns an APIHandler based on the provided API type.
 // 'apiType' parameter could be "jamf" or "graph" to specify which API handler to load.
-func LoadAPIHandler(config Config, apiType string) (APIHandler, error) {
+func LoadAPIHandler(apiType string, log logger.Logger) (APIHandler, error) {
 	var apiHandler APIHandler
 	switch apiType {
 	case "jamfpro":
-		// Assuming GetAPIHandler returns a JamfAPIHandler
 		apiHandler = &jamfpro.JamfAPIHandler{
 			// Initialize with necessary parameters
 		}
+		log.Info("API handler loaded successfully", zap.String("APIType", apiType))
+
 	/*case "graph":
-	// Assuming GetAPIHandler returns a GraphAPIHandler
 	apiHandler = &graph.GraphAPIHandler{
-		// Initialize with necessary parameters
-	}*/
+					// Initialize with necessary parameters
+	}
+	log.Info("API handler loaded successfully", zap.String("APIType", apiType))
+	*/
 	default:
-		return nil, fmt.Errorf("unsupported API type: %s", apiType)
+		return nil, log.Error("Unsupported API type", zap.String("APIType", apiType))
 	}
 
-	// Set the logger level for the handler if needed
-	logger := NewDefaultLogger() // Or use config.Logger if it's not nil
-	logger.SetLevel(config.LogLevel)
-
 	return apiHandler, nil
 }

internal/httpclient/http_client.go

@@ -8,7 +8,6 @@ like the baseURL, authentication details, and an embedded standard HTTP client.
 package httpclient
 
 import (
-	"fmt"
 	"net/http"
 	"sync"
 	"time"
@@ -67,75 +66,73 @@ type Client struct {
 	httpClient                 *http.Client
 	tokenLock                  sync.Mutex
 	config                     Config
-	logger                     logger.Logger
+	Logger                     logger.Logger
 	ConcurrencyMgr             *ConcurrencyManager
 	PerfMetrics                PerformanceMetrics
 }
 
 // BuildClient creates a new HTTP client with the provided configuration.
 func BuildClient(config Config) (*Client, error) {
-	// Use the Logger interface type for the logger variable
-	var logger logger.Logger
-	if config.Logger == nil {
-		logger = NewDefaultLogger()
-	} else {
-		logger = config.Logger
-	}
+	// Initialize the logger using the NewDefaultLogger function from the logger package.
+	log := logger.NewDefaultLogger()
 
-	// Set the logger's level based on the provided configuration if present
-	logger.SetLevel(config.LogLevel)
+	// Set the logger's level based on the provided configuration.
+	log.SetLevel(config.LogLevel)
 
-	// Validate LogLevel
-	if config.LogLevel < LogLevelNone || config.LogLevel > LogLevelDebug {
-		return nil, fmt.Errorf("invalid LogLevel setting: %d", config.LogLevel)
+	if config.LogLevel < logger.LogLevelDebug || config.LogLevel > logger.LogLevelFatal {
+		return nil, log.Error("Invalid LogLevel setting", zap.Int("Provided LogLevel", int(config.LogLevel)))
 	}
 
 	// Use the APIType from the config to determine which API handler to load
-	apiHandler, err := LoadAPIHandler(config, config.APIType)
+	apiHandler, err := LoadAPIHandler(config.APIType, log)
 	if err != nil {
-		logger.Error("Failed to load API handler", zap.String("APIType", config.APIType), zap.Error(err))
-		return nil, err // Return the original error without wrapping it in fmt.Errorf
+		return nil, log.Error("Failed to load API handler", zap.String("APIType", config.APIType), zap.Error(err))
 	}
 
-	logger.Info("Initializing new HTTP client", zap.String("InstanceName", config.InstanceName), zap.String("APIType", config.APIType), zap.Int("LogLevel", int(config.LogLevel)))
+	log.Info("Initializing new HTTP client",
+		zap.String("InstanceName", config.InstanceName), // Using zap.String for structured logging
+		zap.String("APIType", config.APIType),           // Using zap.String for structured logging
+		zap.Int("LogLevel", int(config.LogLevel)),       // Using zap.Int to log LogLevel as an integer
+	)
+
 	// Validate and set default values for the configuration
 	if config.InstanceName == "" {
-		return nil, fmt.Errorf("instanceName cannot be empty")
+		return nil, log.Error("InstanceName cannot be empty")
 	}
 
 	if config.MaxRetryAttempts < 0 {
 		config.MaxRetryAttempts = DefaultMaxRetryAttempts
-		logger.Info("MaxRetryAttempts was negative, set to default value", zap.Int("MaxRetryAttempts", DefaultMaxRetryAttempts))
+		log.Info("MaxRetryAttempts was negative, set to default value", zap.Int("MaxRetryAttempts", DefaultMaxRetryAttempts))
 	}
 
 	if config.MaxConcurrentRequests <= 0 {
 		config.MaxConcurrentRequests = DefaultMaxConcurrentRequests
-		logger.Info("MaxConcurrentRequests was negative or zero, set to default value", zap.Int("MaxConcurrentRequests", DefaultMaxConcurrentRequests))
+		log.Info("MaxConcurrentRequests was negative or zero, set to default value", zap.Int("MaxConcurrentRequests", DefaultMaxConcurrentRequests))
 	}
 
 	if config.TokenRefreshBufferPeriod < 0 {
 		config.TokenRefreshBufferPeriod = DefaultTokenBufferPeriod
-		logger.Info("TokenRefreshBufferPeriod was negative, set to default value", zap.Duration("TokenRefreshBufferPeriod", DefaultTokenBufferPeriod))
+		log.Info("TokenRefreshBufferPeriod was negative, set to default value", zap.Duration("TokenRefreshBufferPeriod", DefaultTokenBufferPeriod))
 	}
 
 	if config.TotalRetryDuration <= 0 {
 		config.TotalRetryDuration = DefaultTotalRetryDuration
-		logger.Info("TotalRetryDuration was negative or zero, set to default value", zap.Duration("TotalRetryDuration", DefaultTotalRetryDuration))
+		log.Info("TotalRetryDuration was negative or zero, set to default value", zap.Duration("TotalRetryDuration", DefaultTotalRetryDuration))
 	}
 
 	if config.TokenRefreshBufferPeriod == 0 {
 		config.TokenRefreshBufferPeriod = DefaultTokenBufferPeriod
-		logger.Info("TokenRefreshBufferPeriod not set, set to default value", zap.Duration("TokenRefreshBufferPeriod", DefaultTokenBufferPeriod))
+		log.Info("TokenRefreshBufferPeriod not set, set to default value", zap.Duration("TokenRefreshBufferPeriod", DefaultTokenBufferPeriod))
 	}
 
 	if config.TotalRetryDuration == 0 {
 		config.TotalRetryDuration = DefaultTotalRetryDuration
-		logger.Info("TotalRetryDuration not set, set to default value", zap.Duration("TotalRetryDuration", DefaultTotalRetryDuration))
+		log.Info("TotalRetryDuration not set, set to default value", zap.Duration("TotalRetryDuration", DefaultTotalRetryDuration))
 	}
 
 	if config.CustomTimeout == 0 {
 		config.CustomTimeout = DefaultTimeout
-		logger.Info("CustomTimeout not set, set to default value", zap.Duration("CustomTimeout", DefaultTimeout))
+		log.Info("CustomTimeout not set, set to default value", zap.Duration("CustomTimeout", DefaultTimeout))
 	}
 
 	// Determine the authentication method
@@ -145,7 +142,7 @@ func BuildClient(config Config) (*Client, error) {
 	} else if config.Auth.ClientID != "" && config.Auth.ClientSecret != "" {
 		AuthMethod = "oauth"
 	} else {
-		return nil, fmt.Errorf("invalid AuthConfig")
+		return nil, log.Error("Invalid AuthConfig", zap.String("Username", config.Auth.Username), zap.String("ClientID", config.Auth.ClientID))
 	}
 
 	client := &Client{
@@ -154,21 +151,20 @@ func BuildClient(config Config) (*Client, error) {
 		AuthMethod:     AuthMethod,
 		httpClient:     &http.Client{Timeout: config.CustomTimeout},
 		config:         config,
-		logger:         logger,
-		ConcurrencyMgr: NewConcurrencyManager(config.MaxConcurrentRequests, logger, true),
+		Logger:         log,
+		ConcurrencyMgr: NewConcurrencyManager(config.MaxConcurrentRequests, log, true),
 		PerfMetrics:    PerformanceMetrics{},
 	}
 
-	// Get auth token
+	// Authenticate and check token validity.
 	_, err = client.ValidAuthTokenCheck()
 	if err != nil {
-		logger.Error("Failed to validate or obtain auth token", zap.Error(err))
-		return nil, fmt.Errorf("failed to validate auth: %w", err)
+		return nil, log.Error("Failed to validate or obtain auth token", zap.Error(err))
 	}
 
 	go client.StartMetricEvaluation()
 
-	logger.Info("New client initialized", zap.String("InstanceName", client.InstanceName), zap.String("AuthMethod", AuthMethod), zap.Int("MaxRetryAttempts", config.MaxRetryAttempts), zap.Int("MaxConcurrentRequests", config.MaxConcurrentRequests), zap.Bool("EnableDynamicRateLimiting", config.EnableDynamicRateLimiting))
+	log.Info("New client initialized", zap.String("InstanceName", client.InstanceName), zap.String("AuthMethod", AuthMethod), zap.Int("MaxRetryAttempts", config.MaxRetryAttempts), zap.Int("MaxConcurrentRequests", config.MaxConcurrentRequests), zap.Bool("EnableDynamicRateLimiting", config.EnableDynamicRateLimiting))
 
 	return client, nil