Merge pull request #90 from thushan/hotfix/endpoint/model-url-relative-paths

fix: Alternative method of resolving profile paths

Author: thushan

internal/adapter/discovery/http_client.go

@@ -138,11 +138,31 @@ func (c *HTTPModelDiscoveryClient) discoverWithAutoDetection(ctx context.Context
 
 // discoverWithProfile performs discovery using a specific platform profile
 func (c *HTTPModelDiscoveryClient) discoverWithProfile(ctx context.Context, endpoint *domain.Endpoint, platformProfile domain.PlatformProfile, startTime time.Time) ([]*domain.ModelInfo, error) {
-	// Prefer the endpoint's explicitly configured model URL if set, otherwise
-	// fall back to the profile's default discovery path. This allows users to
-	// override discovery URLs for non-standard deployments.
-	// This allows backends like Llamacpp to have overridden discovery URLs (see https://github.com/thushan/olla/issues/86)
-	discoveryURL := endpoint.ModelURLString
+	// For specific profiles like OpenAI-compatible, skip model_url override by default
+	// to avoid breaking existing configurations that rely on profile defaults
+	// Issue #86 fix applies mainly to LlamaCpp, Ollama, etc. where users need custom paths
+	var discoveryURL string
+
+	// Skip model_url override for OpenAI-compatible unless it's a custom path
+	// This prevents issues with endpoints like Docker that have /models returning HTML
+	if platformProfile.GetName() == domain.ProfileOpenAICompatible {
+		if endpoint.ModelURLString != "" {
+			// Only use override if it's not a common problematic path
+			// /models often returns HTML in Docker/OpenWebUI setups, not model data
+			if endpoint.ModelURLString != "/v1/models" && endpoint.ModelURLString != "/models" {
+				discoveryURL = endpoint.ModelURLString
+			}
+			// If it's /models, use profile default (/v1/models) instead
+		}
+		// For all other cases, use profile default
+	} else {
+		// For other profiles (Ollama, LlamaCpp, etc.), respect the override
+		// This fixes Issue #86 where users need custom discovery paths
+		if endpoint.ModelURLString != "" {
+			discoveryURL = endpoint.ModelURLString
+		}
+	}
+
 	if discoveryURL == "" {
 		discoveryURL = platformProfile.GetModelDiscoveryURL(endpoint.URLString)
 	}
@@ -176,7 +196,8 @@ func (c *HTTPModelDiscoveryClient) discoverWithProfile(ctx context.Context, endp
 
 	models, err := platformProfile.ParseModelsResponse(body)
 	if err != nil {
-		return nil, NewDiscoveryError(endpoint.URLString, platformProfile.GetName(), "parse_response", resp.StatusCode, duration, err)
+		// Include the actual discovery URL in the error for debugging
+		return nil, NewDiscoveryError(discoveryURL, platformProfile.GetName(), "parse_response", resp.StatusCode, duration, err)
 	}
 
 	c.updateMetrics(func(m *DiscoveryMetrics) {

internal/adapter/discovery/http_client_test.go

@@ -809,6 +809,88 @@ func TestModelURLFallbackToProfileDefault(t *testing.T) {
 	}
 }
 
+// TestOpenAICompatibleModelURLBehavior verifies special handling for OpenAI-compatible
+// profiles where /v1/models override is ignored to preserve existing configurations.
+func TestOpenAICompatibleModelURLBehavior(t *testing.T) {
+	tests := []struct {
+		name                 string
+		modelURLConfig       string // What user configures
+		expectProfileDefault bool   // Whether we expect the profile default to be used
+		expectedPath         string // Expected discovery path
+	}{
+		{
+			name:                 "OpenAI-compatible with /v1/models uses profile default",
+			modelURLConfig:       "/v1/models",
+			expectProfileDefault: true,
+			expectedPath:         "/v1/models",
+		},
+		{
+			name:                 "OpenAI-compatible with custom path uses override",
+			modelURLConfig:       "/api/v2/models", // Path, will be combined with server URL
+			expectProfileDefault: false,
+			expectedPath:         "/api/v2/models",
+		},
+		{
+			name:                 "OpenAI-compatible with /models ignored (Docker/OpenWebUI case)",
+			modelURLConfig:       "/models", // Often returns HTML, not JSON
+			expectProfileDefault: true,
+			expectedPath:         "/v1/models",
+		},
+		{
+			name:                 "OpenAI-compatible with empty model_url uses profile default",
+			modelURLConfig:       "",
+			expectProfileDefault: true,
+			expectedPath:         "/v1/models",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var requestedPath string
+
+			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				requestedPath = r.URL.Path
+				t.Logf("Server received request at path: %s", r.URL.Path)
+
+				// Only respond to the expected path
+				if r.URL.Path == tt.expectedPath {
+					w.WriteHeader(http.StatusOK)
+					w.Write([]byte(`{"object": "list", "data": [{"id": "test-model", "object": "model"}]}`))
+					return
+				}
+
+				w.WriteHeader(http.StatusNotFound)
+			}))
+			defer server.Close()
+
+			// For the override test, we need to build the full URL first
+			modelURLString := tt.modelURLConfig
+			if tt.modelURLConfig == "/api/v2/models" {
+				modelURLString = server.URL + tt.modelURLConfig
+			}
+
+			// Create endpoint with configured model URL
+			endpoint := createTestEndpointWithModelURL(server.URL, domain.ProfileOpenAICompatible, modelURLString)
+
+			client := NewHTTPModelDiscoveryClientWithDefaults(createTestProfileFactory(t), createTestLogger())
+
+			models, err := client.DiscoverModels(context.Background(), endpoint)
+
+			if err != nil {
+				t.Fatalf("Unexpected error: %v", err)
+			}
+
+			if requestedPath != tt.expectedPath {
+				t.Errorf("Expected path %q to be used, but server received request at %q", tt.expectedPath, requestedPath)
+			}
+
+			if len(models) == 0 {
+				t.Errorf("Expected models to be discovered, got 0")
+			}
+		})
+	}
+}
+
 type serverResponse struct {
 	status int
 	body   string

internal/adapter/discovery/repository.go

@@ -10,6 +10,7 @@ import (
 	"github.com/thushan/olla/internal/adapter/registry/profile"
 	"github.com/thushan/olla/internal/config"
 	"github.com/thushan/olla/internal/core/domain"
+	"github.com/thushan/olla/internal/util"
 )
 
 const (
@@ -140,23 +141,25 @@ func (r *StaticEndpointRepository) LoadFromConfig(ctx context.Context, configs [
 			return fmt.Errorf("invalid endpoint URL %q: %w", cfg.URL, err)
 		}
 
-		healthCheckPath, err := url.Parse(cfg.HealthCheckURL)
+		urlString := endpointURL.String()
+
+		// Build health check and model URLs using ResolveURLPath to preserve
+		// the base URL's path prefix. This handles both relative paths and absolute URLs correctly,
+		// preserving nested paths like http://localhost:12434/engines/llama.cpp/
+		healthCheckURLString := util.ResolveURLPath(urlString, cfg.HealthCheckURL)
+		modelURLString := util.ResolveURLPath(urlString, cfg.ModelURL)
+
+		healthCheckURL, err := url.Parse(healthCheckURLString)
 		if err != nil {
-			return fmt.Errorf("invalid health check URL %q: %w", cfg.HealthCheckURL, err)
+			return fmt.Errorf("invalid health check URL %q: %w", healthCheckURLString, err)
 		}
 
-		modelPath, err := url.Parse(cfg.ModelURL)
+		modelURL, err := url.Parse(modelURLString)
 		if err != nil {
-			return fmt.Errorf("invalid model URL %q: %w", cfg.ModelURL, err)
+			return fmt.Errorf("invalid model URL %q: %w", modelURLString, err)
 		}
 
-		healthCheckURL := endpointURL.ResolveReference(healthCheckPath)
-		modelURL := endpointURL.ResolveReference(modelPath)
-
-		urlString := endpointURL.String()
-		healthCheckPathString := healthCheckPath.String()
-		healthCheckURLString := healthCheckURL.String()
-		modelURLString := modelURL.String()
+		healthCheckPathString := cfg.HealthCheckURL
 
 		newEndpoint := &domain.Endpoint{
 			Name:                  cfg.Name,

internal/adapter/discovery/repository_test.go

@@ -168,3 +168,51 @@ func TestStaticEndpointRepository_EmptyConfig(t *testing.T) {
 		t.Errorf("Expected 0 endpoints for empty config, got %d", len(endpoints))
 	}
 }
+
+func TestStaticEndpointRepository_NestedPathURLs(t *testing.T) {
+	// Test that endpoints with nested paths (like Docker) correctly preserve
+	// the path prefix when building model discovery URLs
+	repo := NewStaticEndpointRepository()
+	ctx := context.Background()
+
+	configs := []config.EndpointConfig{
+		{
+			Name:           "docker-llamacpp",
+			URL:            "http://localhost:12434/engines/llama.cpp/",
+			Type:           "openai-compatible",
+			HealthCheckURL: "/health",
+			ModelURL:       "/v1/models",
+			Priority:       100,
+			CheckInterval:  5 * time.Second,
+			CheckTimeout:   2 * time.Second,
+		},
+	}
+
+	err := repo.LoadFromConfig(ctx, configs)
+	if err != nil {
+		t.Fatalf("LoadFromConfig failed: %v", err)
+	}
+
+	endpoints, err := repo.GetAll(ctx)
+	if err != nil {
+		t.Fatalf("GetAll failed: %v", err)
+	}
+
+	if len(endpoints) != 1 {
+		t.Fatalf("Expected 1 endpoint, got %d", len(endpoints))
+	}
+
+	endpoint := endpoints[0]
+
+	// Verify the model URL string preserves the path prefix
+	expectedModelURL := "http://localhost:12434/engines/llama.cpp/v1/models"
+	if endpoint.ModelURLString != expectedModelURL {
+		t.Errorf("ModelURLString = %q, expected %q", endpoint.ModelURLString, expectedModelURL)
+	}
+
+	// Verify the health check URL string preserves the path prefix
+	expectedHealthURL := "http://localhost:12434/engines/llama.cpp/health"
+	if endpoint.HealthCheckURLString != expectedHealthURL {
+		t.Errorf("HealthCheckURLString = %q, expected %q", endpoint.HealthCheckURLString, expectedHealthURL)
+	}
+}

internal/adapter/registry/profile/configurable_profile.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/thushan/olla/internal/core/domain"
+	"github.com/thushan/olla/internal/util"
 	"github.com/thushan/olla/internal/util/pattern"
 )
 
@@ -51,11 +52,7 @@ func (p *ConfigurableProfile) GetPath(index int) string {
 }
 
 func (p *ConfigurableProfile) GetModelDiscoveryURL(baseURL string) string {
-	// avoid http://localhost//v1/models nonsense
-	if baseURL != "" && baseURL[len(baseURL)-1] == '/' {
-		baseURL = baseURL[:len(baseURL)-1]
-	}
-	return baseURL + p.config.API.ModelDiscoveryPath
+	return util.ResolveURLPath(baseURL, p.config.API.ModelDiscoveryPath)
 }
 
 func (p *ConfigurableProfile) GetHealthCheckPath() string {

internal/util/url.go

@@ -0,0 +1,44 @@
+package util
+
+import (
+	"net/url"
+	"path"
+)
+
+// ResolveURLPath resolves a path or absolute URL against a base URL.
+// If pathOrURL is already an absolute URL (has a scheme like http://), it is returned as-is.
+// Otherwise, pathOrURL is treated as a relative path and joined with the base URL's path,
+// preserving any path prefix in the base URL.
+//
+// This function uses url.Parse() and path.Join() from the standard library to handle
+// URL resolution correctly, avoiding the pitfalls of url.ResolveReference() which treats
+// paths starting with "/" as absolute references per RFC 3986 (replacing the entire path).
+//
+// Examples:
+//   - ResolveURLPath("http://localhost:12434/api/", "/v1/models") -> "http://localhost:12434/api/v1/models"
+//   - ResolveURLPath("http://localhost:12434/api/", "http://other:9000/models") -> "http://other:9000/models"
+func ResolveURLPath(baseURL, pathOrURL string) string {
+	if baseURL == "" {
+		return pathOrURL
+	}
+	if pathOrURL == "" {
+		return baseURL
+	}
+
+	// Check if pathOrURL is already an absolute URL
+	if parsed, err := url.Parse(pathOrURL); err == nil && parsed.IsAbs() {
+		return pathOrURL
+	}
+
+	// Parse base URL
+	base, err := url.Parse(baseURL)
+	if err != nil {
+		// If base URL is invalid, return pathOrURL as fallback
+		return pathOrURL
+	}
+
+	// Use path.Join to preserve the base path prefix when joining with relative paths
+	// and to normalise redundant slashes
+	base.Path = path.Join(base.Path, pathOrURL)
+	return base.String()
+}