fix(catalog): apply default orderBy for MCP server pagination (kubeflow#2592)

When filterQuery is provided without an explicit orderBy, the API layer
set OrderBy/SortOrder to pointers to empty strings, bypassing the
nil-based defaults ("id"/"ASC"). This caused non-deterministic results
and empty subsequent pages with cursor-based pagination.

Only set the pagination pointers when values are non-empty, so the
defaults apply correctly.

Assisted-by: Claude Code <noreply@anthropic.com>

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

Author: Al-Pragliola

catalog/internal/catalog/mcpcatalog/db_mcp.go

@@ -107,8 +107,12 @@ func (d *dbMCPCatalogImpl) ListMCPServers(ctx context.Context, params ListMCPSer
 	orderBy := strings.ToUpper(string(params.OrderBy))
 	sortOrder := strings.ToUpper(string(params.SortOrder))
 	listOptions.Pagination.PageSize = &params.PageSize
-	listOptions.Pagination.OrderBy = &orderBy
-	listOptions.Pagination.SortOrder = &sortOrder
+	if orderBy != "" {
+		listOptions.Pagination.OrderBy = &orderBy
+	}
+	if sortOrder != "" {
+		listOptions.Pagination.SortOrder = &sortOrder
+	}
 	if params.NextPageToken != nil {
 		listOptions.Pagination.NextPageToken = params.NextPageToken
 	}

catalog/internal/catalog/mcpcatalog/db_mcp_test.go

@@ -586,6 +586,103 @@ func TestGetMCPServerTool_PassesToolNameFilter(t *testing.T) {
 	assert.Equal(t, "list_problems", *toolRepo.capturedListOptions.ToolName)
 }
 
+// --- Pagination default tests ---
+
+func TestListMCPServers_EmptyOrderByUsesDefault(t *testing.T) {
+	// When OrderBy and SortOrder are not specified (empty strings from the API layer),
+	// the pagination must still apply default ordering ("id" / "ASC") so that
+	// cursor-based pagination works correctly on subsequent pages.
+	repo := &mockMCPServerRepo{listResult: emptyList()}
+	cat := newTestCatalog(repo, nil)
+
+	_, err := cat.ListMCPServers(context.Background(), ListMCPServersParams{
+		FilterQuery: "license='Apache 2.0'",
+		PageSize:    10,
+		// OrderBy and SortOrder are zero values (empty strings)
+	})
+	require.NoError(t, err)
+
+	pagination := repo.capturedOptions.Pagination
+	// The pagination must resolve to the defaults ("id" / "ASC").
+	// Either OrderBy is nil (so GetOrderBy falls through to DefaultOrderBy),
+	// or it is set to the explicit default value — but it must NOT be a
+	// pointer to an empty string, which would skip the ORDER BY clause.
+	orderBy := pagination.GetOrderBy()
+	sortOrder := pagination.GetSortOrder()
+	assert.Equal(t, "id", orderBy, "expected default orderBy 'id' when param is empty")
+	assert.Equal(t, "ASC", sortOrder, "expected default sortOrder 'ASC' when param is empty")
+}
+
+func TestListMCPServers_EmptyOrderByPaginationNotNilEmptyString(t *testing.T) {
+	// Regression: when OrderBy/SortOrder are empty, they must NOT be set as
+	// pointers to empty strings. The downstream paginator skips ORDER BY when
+	// both are empty, but still applies the cursor WHERE clause, causing
+	// non-deterministic results and empty second pages.
+	repo := &mockMCPServerRepo{listResult: emptyList()}
+	cat := newTestCatalog(repo, nil)
+
+	_, err := cat.ListMCPServers(context.Background(), ListMCPServersParams{
+		FilterQuery: "license='Apache 2.0'",
+		PageSize:    1,
+		// OrderBy and SortOrder deliberately left as zero values
+	})
+	require.NoError(t, err)
+
+	pagination := repo.capturedOptions.Pagination
+
+	// If OrderBy is non-nil, it must not point to an empty string.
+	if pagination.OrderBy != nil {
+		assert.NotEmpty(t, *pagination.OrderBy, "OrderBy must not be a pointer to an empty string")
+	}
+	// If SortOrder is non-nil, it must not point to an empty string.
+	if pagination.SortOrder != nil {
+		assert.NotEmpty(t, *pagination.SortOrder, "SortOrder must not be a pointer to an empty string")
+	}
+}
+
+func TestListMCPServers_ExplicitOrderByIsPreserved(t *testing.T) {
+	// When OrderBy/SortOrder are explicitly provided, they should be passed through.
+	repo := &mockMCPServerRepo{listResult: emptyList()}
+	cat := newTestCatalog(repo, nil)
+
+	_, err := cat.ListMCPServers(context.Background(), ListMCPServersParams{
+		FilterQuery: "license='Apache 2.0'",
+		PageSize:    10,
+		OrderBy:     "CREATE_TIME",
+		SortOrder:   "DESC",
+	})
+	require.NoError(t, err)
+
+	pagination := repo.capturedOptions.Pagination
+	assert.Equal(t, "CREATE_TIME", pagination.GetOrderBy())
+	assert.Equal(t, "DESC", pagination.GetSortOrder())
+}
+
+func TestListMCPServerTools_EmptyOrderByUsesDefault(t *testing.T) {
+	// Same bug applies to ListMCPServerTools — empty OrderBy/SortOrder must
+	// resolve to defaults for correct cursor-based pagination.
+	serverEntity := &models.MCPServerImpl{
+		ID:         serverID(1),
+		Attributes: &models.MCPServerAttributes{Name: serverName("test-server")},
+	}
+	repo := &mockMCPServerRepo{getResult: serverEntity}
+	toolRepo := &mockMCPServerToolRepo{listResult: toolList()}
+	cat := newTestCatalogWithToolRepo(repo, toolRepo, nil)
+
+	_, err := cat.ListMCPServerTools(context.Background(), "1", ListMCPServerToolsParams{
+		PageSize: 10,
+		// OrderBy and SortOrder are zero values (empty strings)
+	})
+	require.NoError(t, err)
+
+	require.NotNil(t, toolRepo.capturedListOptions)
+	pagination := toolRepo.capturedListOptions.Pagination
+	orderBy := pagination.GetOrderBy()
+	sortOrder := pagination.GetSortOrder()
+	assert.Equal(t, "id", orderBy, "expected default orderBy 'id' when param is empty")
+	assert.Equal(t, "ASC", sortOrder, "expected default sortOrder 'ASC' when param is empty")
+}
+
 func TestGetMCPServer_IncludeToolsUsesAccurateCount(t *testing.T) {
 	serverEntity := &models.MCPServerImpl{
 		ID:         serverID(1),

internal/db/models/pagination.go

@@ -29,15 +29,15 @@ func (p *Pagination) GetNextPageToken() string {
 }
 
 func (p *Pagination) GetOrderBy() string {
-	if p.OrderBy == nil {
+	if p.OrderBy == nil || *p.OrderBy == "" {
 		return DefaultOrderBy
 	}
 
 	return *p.OrderBy
 }
 
 func (p *Pagination) GetSortOrder() string {
-	if p.SortOrder == nil {
+	if p.SortOrder == nil || *p.SortOrder == "" {
 		return DefaultSortOrder
 	}
 

internal/db/models/pagination_test.go

@@ -0,0 +1,53 @@
+package models
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestGetOrderBy_NilReturnsDefault verifies that a nil OrderBy returns the default.
+func TestGetOrderBy_NilReturnsDefault(t *testing.T) {
+	p := &Pagination{}
+	assert.Equal(t, DefaultOrderBy, p.GetOrderBy(), "nil OrderBy should return default")
+}
+
+// TestGetOrderBy_EmptyStringReturnsDefault verifies that a pointer to an empty
+// string still returns the default OrderBy. This is the root cause of
+// RHOAIENG-57798: the API layer sets OrderBy to a pointer to "" which bypasses
+// the nil check and results in no ORDER BY clause, breaking cursor-based pagination.
+func TestGetOrderBy_EmptyStringReturnsDefault(t *testing.T) {
+	empty := ""
+	p := &Pagination{OrderBy: &empty}
+	assert.Equal(t, DefaultOrderBy, p.GetOrderBy(),
+		"empty string OrderBy should return default 'id', not empty string")
+}
+
+// TestGetOrderBy_ExplicitValuePreserved verifies that an explicit value is returned as-is.
+func TestGetOrderBy_ExplicitValuePreserved(t *testing.T) {
+	val := "CREATE_TIME"
+	p := &Pagination{OrderBy: &val}
+	assert.Equal(t, "CREATE_TIME", p.GetOrderBy())
+}
+
+// TestGetSortOrder_NilReturnsDefault verifies that a nil SortOrder returns the default.
+func TestGetSortOrder_NilReturnsDefault(t *testing.T) {
+	p := &Pagination{}
+	assert.Equal(t, DefaultSortOrder, p.GetSortOrder(), "nil SortOrder should return default")
+}
+
+// TestGetSortOrder_EmptyStringReturnsDefault verifies that a pointer to an empty
+// string still returns the default SortOrder. Same root cause as RHOAIENG-57798.
+func TestGetSortOrder_EmptyStringReturnsDefault(t *testing.T) {
+	empty := ""
+	p := &Pagination{SortOrder: &empty}
+	assert.Equal(t, DefaultSortOrder, p.GetSortOrder(),
+		"empty string SortOrder should return default 'ASC', not empty string")
+}
+
+// TestGetSortOrder_ExplicitValuePreserved verifies that an explicit value is returned as-is.
+func TestGetSortOrder_ExplicitValuePreserved(t *testing.T) {
+	val := "DESC"
+	p := &Pagination{SortOrder: &val}
+	assert.Equal(t, "DESC", p.GetSortOrder())
+}