Merge pull request #1670 from kubeflow/main

[pull] main from kubeflow:main

Author: openshift-merge-bot[bot]

catalog/internal/catalog/mcpcatalog/service/mcp_server.go

@@ -472,7 +472,7 @@ func (r *MCPServerRepositoryImpl) applyCustomOrdering(query *gorm.DB, listOption
 
 	// Handle NAME ordering
 	if orderBy == "NAME" {
-		return pagination.ApplyNameOrdering(query, contextTable, listOptions.GetSortOrder(), listOptions.GetNextPageToken(), listOptions.GetPageSize())
+		return pagination.ApplyNameOrdering(query, contextTable, listOptions.GetSortOrder(), listOptions.GetNextPageToken(), listOptions.GetPageSize(), false)
 	}
 
 	// Fall back to standard pagination

catalog/internal/catalog/modelcatalog/service/catalog_model.go

@@ -470,7 +470,7 @@ func (r *CatalogModelRepositoryImpl) applyCustomOrdering(query *gorm.DB, listOpt
 
 	// Handle NAME ordering specially (catalog-specific)
 	if orderBy == "NAME" {
-		return catpagination.ApplyNameOrdering(query, contextTable, listOptions.GetSortOrder(), listOptions.GetNextPageToken(), listOptions.GetPageSize())
+		return catpagination.ApplyNameOrdering(query, contextTable, listOptions.GetSortOrder(), listOptions.GetNextPageToken(), listOptions.GetPageSize(), true)
 	}
 
 	subquery, sortColumn := r.sortValueQuery(listOptions, contextTable+".id")
@@ -536,7 +536,11 @@ func (r *CatalogModelRepositoryImpl) applyCursorPagination(query *gorm.DB, curso
 func (r *CatalogModelRepositoryImpl) createPaginationToken(lastItem schema.Context, listOptions *models.CatalogModelListOptions) string {
 	// Handle NAME ordering (catalog-specific)
 	if listOptions.GetOrderBy() == "NAME" {
-		return catpagination.CreateNamePaginationToken(lastItem.ID, &lastItem.Name)
+		displayName := lastItem.Name
+		if _, after, ok := strings.Cut(lastItem.Name, ":"); ok {
+			displayName = after
+		}
+		return catpagination.CreateNamePaginationToken(lastItem.ID, &displayName)
 	}
 
 	sortValueQuery, column := r.sortValueQuery(listOptions)

catalog/internal/catalog/modelcatalog/service/catalog_model_test.go

@@ -830,27 +830,38 @@ func TestCatalogModelRepository(t *testing.T) {
 	})
 
 	t.Run("TestNameOrdering", func(t *testing.T) {
-		// Create test models with specific names for ordering
-		testModels := []string{
-			"zebra-model",
-			"alpha-model",
-			"beta-model",
-			"gamma-model",
-			"delta-model",
-		}
-
-		var savedModels []models.CatalogModel
-		for _, name := range testModels {
+		// Create models from two different sources so stored names have source prefixes.
+		// This validates that sorting is by display name (after the colon) not the raw stored name.
+		// Without the fix, "source-z:alpha-model" would sort after "source-a:zebra-model".
+		type modelSpec struct {
+			displayName string
+			sourceID    string
+		}
+		testModels := []modelSpec{
+			// source-z prefix ensures raw-name sort would put alpha/beta last
+			{displayName: "alpha-model", sourceID: "source-z"},
+			{displayName: "beta-model", sourceID: "source-z"},
+			// source-a prefix ensures raw-name sort would put gamma/zebra first
+			{displayName: "gamma-model", sourceID: "source-a"},
+			{displayName: "zebra-model", sourceID: "source-a"},
+			{displayName: "delta-model", sourceID: "source-m"},
+			// colon in display name: stored as "source-a:epsilon:v2", display name is "epsilon:v2"
+			{displayName: "epsilon:v2", sourceID: "source-a"},
+		}
+
+		for _, spec := range testModels {
 			catalogModel := &models.CatalogModelImpl{
 				Attributes: &models.CatalogModelAttributes{
-					Name:       apiutils.Of(name),
-					ExternalID: apiutils.Of(name + "-ext"),
+					Name:       apiutils.Of(spec.displayName),
+					ExternalID: apiutils.Of(spec.displayName + "-ord-ext"),
+				},
+				Properties: &[]dbmodels.Properties{
+					{Name: "source_id", StringValue: apiutils.Of(spec.sourceID)},
 				},
 			}
 
-			savedModel, err := repo.Save(catalogModel)
+			_, err := repo.Save(catalogModel)
 			require.NoError(t, err)
-			savedModels = append(savedModels, savedModel)
 		}
 
 		// Test NAME ordering ASC
@@ -864,35 +875,48 @@ func TestCatalogModelRepository(t *testing.T) {
 		require.NoError(t, err)
 		require.NotNil(t, result)
 
-		// Extract our test model names from results
+		// Extract our test model display names from results (repo returns stored namespaced names)
+		displayNames := map[string]string{
+			"source-z:alpha-model": "alpha-model",
+			"source-z:beta-model":  "beta-model",
+			"source-a:gamma-model": "gamma-model",
+			"source-a:zebra-model": "zebra-model",
+			"source-m:delta-model": "delta-model",
+			// display name contains a colon: stored name is "source-a:epsilon:v2"
+			"source-a:epsilon:v2": "epsilon:v2",
+		}
 		var foundNames []string
 		for _, model := range result.Items {
-			name := *model.GetAttributes().Name
-			if name == "zebra-model" || name == "alpha-model" || name == "beta-model" ||
-				name == "gamma-model" || name == "delta-model" {
-				foundNames = append(foundNames, name)
+			storedName := *model.GetAttributes().Name
+			if displayName, ok := displayNames[storedName]; ok {
+				foundNames = append(foundNames, displayName)
 			}
 		}
 
 		// Verify we found all our test models
-		require.GreaterOrEqual(t, len(foundNames), 5, "Should find all test models")
+		require.GreaterOrEqual(t, len(foundNames), 6, "Should find all test models")
 
-		// Verify ASC ordering: alpha < beta < delta < gamma < zebra
+		// Verify ASC ordering by display name: alpha < beta < delta < epsilon:v2 < gamma < zebra
+		// Without the fix, source-z models (alpha, beta) would appear after source-a models (gamma, zebra).
+		// Also verifies multi-colon display names ("epsilon:v2") sort correctly.
 		alphaIdx := findIndex(foundNames, "alpha-model")
 		betaIdx := findIndex(foundNames, "beta-model")
 		deltaIdx := findIndex(foundNames, "delta-model")
+		epsilonIdx := findIndex(foundNames, "epsilon:v2")
 		gammaIdx := findIndex(foundNames, "gamma-model")
 		zebraIdx := findIndex(foundNames, "zebra-model")
 
 		require.NotEqual(t, -1, alphaIdx, "alpha-model not found")
 		require.NotEqual(t, -1, betaIdx, "beta-model not found")
 		require.NotEqual(t, -1, deltaIdx, "delta-model not found")
+		require.NotEqual(t, -1, epsilonIdx, "epsilon:v2 not found")
 		require.NotEqual(t, -1, gammaIdx, "gamma-model not found")
 		require.NotEqual(t, -1, zebraIdx, "zebra-model not found")
 
 		assert.Less(t, alphaIdx, betaIdx, "alpha should come before beta in ASC")
 		assert.Less(t, betaIdx, deltaIdx, "beta should come before delta in ASC")
-		assert.Less(t, deltaIdx, gammaIdx, "delta should come before gamma in ASC")
+		assert.Less(t, deltaIdx, epsilonIdx, "delta should come before epsilon:v2 in ASC")
+		assert.Less(t, epsilonIdx, gammaIdx, "epsilon:v2 should come before gamma in ASC")
 		assert.Less(t, gammaIdx, zebraIdx, "gamma should come before zebra in ASC")
 
 		// Test NAME ordering DESC
@@ -909,22 +933,23 @@ func TestCatalogModelRepository(t *testing.T) {
 		// Extract our test model names from DESC results
 		foundNames = []string{}
 		for _, model := range result.Items {
-			name := *model.GetAttributes().Name
-			if name == "zebra-model" || name == "alpha-model" || name == "beta-model" ||
-				name == "gamma-model" || name == "delta-model" {
-				foundNames = append(foundNames, name)
+			storedName := *model.GetAttributes().Name
+			if displayName, ok := displayNames[storedName]; ok {
+				foundNames = append(foundNames, displayName)
 			}
 		}
 
-		// Verify DESC ordering: zebra > gamma > delta > beta > alpha
+		// Verify DESC ordering: zebra > gamma > epsilon:v2 > delta > beta > alpha
 		alphaIdxDesc := findIndex(foundNames, "alpha-model")
 		betaIdxDesc := findIndex(foundNames, "beta-model")
 		deltaIdxDesc := findIndex(foundNames, "delta-model")
+		epsilonIdxDesc := findIndex(foundNames, "epsilon:v2")
 		gammaIdxDesc := findIndex(foundNames, "gamma-model")
 		zebraIdxDesc := findIndex(foundNames, "zebra-model")
 
 		assert.Less(t, zebraIdxDesc, gammaIdxDesc, "zebra should come before gamma in DESC")
-		assert.Less(t, gammaIdxDesc, deltaIdxDesc, "gamma should come before delta in DESC")
+		assert.Less(t, gammaIdxDesc, epsilonIdxDesc, "gamma should come before epsilon:v2 in DESC")
+		assert.Less(t, epsilonIdxDesc, deltaIdxDesc, "epsilon:v2 should come before delta in DESC")
 		assert.Less(t, deltaIdxDesc, betaIdxDesc, "delta should come before beta in DESC")
 		assert.Less(t, betaIdxDesc, alphaIdxDesc, "beta should come before alpha in DESC")
 	})
@@ -1023,6 +1048,86 @@ func TestCatalogModelRepository(t *testing.T) {
 		}
 	})
 
+	t.Run("TestNameOrderingMultiColonPagination", func(t *testing.T) {
+		// Regression test: display names containing a colon (e.g. "llama:7b" stored as "source:llama:7b")
+		// caused cursor-based pagination to break because SUBSTRING(name FROM STRPOS(name, ':') + 1)
+		// returns everything after the first colon (e.g. "llama:7b"), while strings.Cut does the same,
+		// keeping cursor values consistent with ORDER BY values.
+		multiColonModels := []struct {
+			displayName string
+			sourceID    string
+		}{
+			{"mc-model:aa", "source-x"},
+			{"mc-model:bb", "source-x"},
+			{"mc-model:cc", "source-x"},
+			{"mc-model:dd", "source-x"},
+		}
+
+		for _, spec := range multiColonModels {
+			catalogModel := &models.CatalogModelImpl{
+				Attributes: &models.CatalogModelAttributes{
+					Name:       apiutils.Of(spec.displayName),
+					ExternalID: apiutils.Of(spec.displayName + "-mc-ext"),
+				},
+				Properties: &[]dbmodels.Properties{
+					{Name: "source_id", StringValue: apiutils.Of(spec.sourceID)},
+				},
+			}
+			_, err := repo.Save(catalogModel)
+			require.NoError(t, err)
+		}
+
+		// Paginate through all 4 models 2 at a time; verify ordering is stable across pages.
+		listOptions := models.CatalogModelListOptions{
+			Pagination: dbmodels.Pagination{
+				OrderBy:   apiutils.Of("NAME"),
+				SortOrder: apiutils.Of("ASC"),
+				PageSize:  apiutils.Of(int32(2)),
+			},
+		}
+
+		var allFound []string
+		currentToken := (*string)(nil)
+		for range 10 {
+			if currentToken != nil {
+				listOptions.Pagination.NextPageToken = currentToken
+			}
+			page, err := repo.List(listOptions)
+			require.NoError(t, err)
+
+			for _, model := range page.Items {
+				name := *model.GetAttributes().Name
+				if strings.HasPrefix(name, "source-x:mc-model:") {
+					allFound = append(allFound, name)
+				}
+			}
+
+			if page.NextPageToken == "" || len(allFound) >= 4 {
+				break
+			}
+			currentToken = &page.NextPageToken
+		}
+
+		require.GreaterOrEqual(t, len(allFound), 4, "Should find all multi-colon models")
+
+		// Verify ASC ordering is preserved across pages
+		expectedOrder := []string{
+			"source-x:mc-model:aa",
+			"source-x:mc-model:bb",
+			"source-x:mc-model:cc",
+			"source-x:mc-model:dd",
+		}
+		lastIndex := -1
+		for _, expected := range expectedOrder {
+			idx := findIndex(allFound, expected)
+			assert.NotEqual(t, -1, idx, "Should find %s", expected)
+			if idx != -1 {
+				assert.Greater(t, idx, lastIndex, "%s should appear after previous models in ASC order", expected)
+				lastIndex = idx
+			}
+		}
+	})
+
 	t.Run("TestDeleteBySource", func(t *testing.T) {
 		// Setup: Create models with different source IDs
 		sourceID1 := "test_source_1"

catalog/internal/db/pagination/pagination.go

@@ -37,17 +37,24 @@ func CreateNamePaginationToken(entityID int32, name *string) string {
 //   - sortOrder: The sort order ("ASC" or "DESC")
 //   - nextPageToken: Optional pagination token for cursor-based pagination
 //   - pageSize: The page size (0 means no limit)
+//   - stripSourcePrefix: When true, strips the "sourceId:" prefix before sorting.
+//     Only models use prefixed names; artifacts and MCP servers do not.
 //
 // Returns the modified query with NAME ordering and pagination applied.
-func ApplyNameOrdering(query *gorm.DB, tableName string, sortOrder string, nextPageToken string, pageSize int32) *gorm.DB {
+func ApplyNameOrdering(query *gorm.DB, tableName string, sortOrder string, nextPageToken string, pageSize int32, stripSourcePrefix bool) *gorm.DB {
 	// Normalize sort order
 	order := "ASC"
 	if sortOrder == "DESC" {
 		order = "DESC"
 	}
 
-	// Apply name-based ordering with ID as tie-breaker
-	query = query.Order(fmt.Sprintf("%s.name %s, %s.id ASC", tableName, order, tableName))
+	var nameExpr string
+	if stripSourcePrefix {
+		nameExpr = fmt.Sprintf("COALESCE(NULLIF(SUBSTRING(%s.name FROM STRPOS(%s.name, ':') + 1), ''), %s.name)", tableName, tableName, tableName)
+	} else {
+		nameExpr = fmt.Sprintf("%s.name", tableName)
+	}
+	query = query.Order(fmt.Sprintf("%s %s, %s.id ASC", nameExpr, order, tableName))
 
 	// Handle cursor-based pagination for NAME
 	if nextPageToken != "" {
@@ -56,13 +63,15 @@ func ApplyNameOrdering(query *gorm.DB, tableName string, sortOrder string, nextP
 			_ = query.AddError(fmt.Errorf("invalid nextPageToken: %w", err))
 			return query
 		}
-		// Cursor pagination based on name (string comparison)
+		// Cursor pagination based on display name (string comparison).
+		// cursor.Value holds the display name (after the colon prefix).
 		cmp := ">"
 		if order == "DESC" {
 			cmp = "<"
 		}
-		// Use proper string comparison with name and ID as tie-breaker
-		query = query.Where(fmt.Sprintf("(%s.name %s ? OR (%s.name = ? AND %s.id > ?))", tableName, cmp, tableName, tableName),
+		// Use proper string comparison with display name and ID as tie-breaker
+		query = query.Where(
+			fmt.Sprintf("(%s %s ? OR (%s = ? AND %s.id > ?))", nameExpr, cmp, nameExpr, tableName),
 			cursor.Value, cursor.Value, cursor.ID)
 	}
 

catalog/internal/db/service/catalog_artifact.go

@@ -164,7 +164,7 @@ func (r *CatalogArtifactRepositoryImpl) List(listOptions models.CatalogArtifactL
 	// Handle NAME ordering specially (catalog-specific) to avoid string-to-integer cast issues
 	if orderBy == "NAME" {
 		artifactTable := utils.GetTableName(query, &schema.Artifact{})
-		query = catpagination.ApplyNameOrdering(query, artifactTable, sortOrder, nextPageToken, pageSize)
+		query = catpagination.ApplyNameOrdering(query, artifactTable, sortOrder, nextPageToken, pageSize, false)
 	} else if _, isAllowedColumn := catpagination.CatalogOrderByColumns[orderBy]; isAllowedColumn {
 		// Handle standard allowed columns (ID, CREATE_TIME, LAST_UPDATE_TIME)
 		pagination := &dbmodels.Pagination{
@@ -383,7 +383,7 @@ func (r *CatalogArtifactRepositoryImpl) applyCustomOrdering(query *gorm.DB, list
 
 	// Handle NAME ordering specially (catalog-specific)
 	if orderBy == "NAME" {
-		return catpagination.ApplyNameOrdering(query, artifactTable, listOptions.GetSortOrder(), listOptions.GetNextPageToken(), listOptions.GetPageSize()), nil
+		return catpagination.ApplyNameOrdering(query, artifactTable, listOptions.GetSortOrder(), listOptions.GetNextPageToken(), listOptions.GetPageSize(), false), nil
 	}
 
 	subquery, sortColumn, err := r.sortValueQuery(listOptions, artifactTable+".id")

clients/ui/frontend/package-lock.json

@@ -1,12 +1,11 @@
 import * as React from 'react';
-import { DashboardEmptyTableView, Table } from 'mod-arch-shared';
+import { DashboardEmptyTableView, Table, ManageColumnsModal } from 'mod-arch-shared';
 import { Button, Spinner } from '@patternfly/react-core';
 import { ColumnsIcon } from '@patternfly/react-icons';
 import { OuterScrollContainer } from '@patternfly/react-table';
 import { CatalogPerformanceMetricsArtifact } from '~/app/modelCatalogTypes';
 import { ModelCatalogContext } from '~/app/context/modelCatalog/ModelCatalogContext';
 import { getActiveLatencyFieldName } from '~/app/pages/modelCatalog/utils/modelCatalogUtils';
-import { ManageColumnsModal } from '~/app/shared/components/manageColumns/ManageColumnsModal';
 import HardwareConfigurationTableRow from './HardwareConfigurationTableRow';
 import HardwareConfigurationFilterToolbar from './HardwareConfigurationFilterToolbar';
 import { useHardwareConfigColumns, ControlledTableSortProps } from './useHardwareConfigColumns';