@@ -8,10 +8,14 @@ import (
88 "testing"
99
1010 "github.com/kubeflow/model-registry/catalog/internal/catalog/basecatalog"
11+ "github.com/kubeflow/model-registry/catalog/internal/catalog/mcpcatalog"
12+ mcpcatalogmodels "github.com/kubeflow/model-registry/catalog/internal/catalog/mcpcatalog/models"
13+ mcpservice "github.com/kubeflow/model-registry/catalog/internal/catalog/mcpcatalog/service"
1114 "github.com/kubeflow/model-registry/catalog/internal/catalog/modelcatalog/models"
1215 modelservice "github.com/kubeflow/model-registry/catalog/internal/catalog/modelcatalog/service"
1316 sharedmodels "github.com/kubeflow/model-registry/catalog/internal/db/models"
1417 "github.com/kubeflow/model-registry/catalog/internal/db/service"
18+ "github.com/kubeflow/model-registry/catalog/internal/testhelpers"
1519 model "github.com/kubeflow/model-registry/catalog/pkg/openapi"
1620 "github.com/kubeflow/model-registry/internal/apiutils"
1721 mr_models "github.com/kubeflow/model-registry/internal/db/models"
@@ -31,10 +35,10 @@ func TestDBCatalog(t *testing.T) {
3135 defer cleanup ()
3236
3337 // Get type IDs
34- catalogModelTypeID := GetCatalogModelTypeIDForDBTest (t , sharedDB )
35- modelArtifactTypeID := GetCatalogModelArtifactTypeIDForDBTest (t , sharedDB )
36- metricsArtifactTypeID := GetCatalogMetricsArtifactTypeIDForDBTest (t , sharedDB )
37- catalogSourceTypeID := GetCatalogSourceTypeIDForDBTest (t , sharedDB )
38+ catalogModelTypeID := testhelpers . GetCatalogModelTypeIDForDBTest (t , sharedDB )
39+ modelArtifactTypeID := testhelpers . GetCatalogModelArtifactTypeIDForDBTest (t , sharedDB )
40+ metricsArtifactTypeID := testhelpers . GetCatalogMetricsArtifactTypeIDForDBTest (t , sharedDB )
41+ catalogSourceTypeID := testhelpers . GetCatalogSourceTypeIDForDBTest (t , sharedDB )
3842
3943 // Create repositories
4044 catalogModelRepo := modelservice .NewCatalogModelRepository (sharedDB , catalogModelTypeID )
@@ -1409,10 +1413,10 @@ func TestDBCatalog_GetPerformanceArtifactsWithService(t *testing.T) {
14091413 defer cleanup ()
14101414
14111415 // Get type IDs
1412- catalogModelTypeID := GetCatalogModelTypeIDForDBTest (t , sharedDB )
1413- modelArtifactTypeID := GetCatalogModelArtifactTypeIDForDBTest (t , sharedDB )
1414- metricsArtifactTypeID := GetCatalogMetricsArtifactTypeIDForDBTest (t , sharedDB )
1415- catalogSourceTypeID := GetCatalogSourceTypeIDForDBTest (t , sharedDB )
1416+ catalogModelTypeID := testhelpers . GetCatalogModelTypeIDForDBTest (t , sharedDB )
1417+ modelArtifactTypeID := testhelpers . GetCatalogModelArtifactTypeIDForDBTest (t , sharedDB )
1418+ metricsArtifactTypeID := testhelpers . GetCatalogMetricsArtifactTypeIDForDBTest (t , sharedDB )
1419+ catalogSourceTypeID := testhelpers . GetCatalogSourceTypeIDForDBTest (t , sharedDB )
14161420
14171421 // Create repositories
14181422 catalogModelRepo := modelservice .NewCatalogModelRepository (sharedDB , catalogModelTypeID )
@@ -1529,9 +1533,15 @@ func TestGetFilterOptionsWithNamedQueries(t *testing.T) {
15291533 err := sources .MergeWithNamedQueries ("test" , map [string ]basecatalog.ModelSource {}, namedQueries )
15301534 require .NoError (t , err )
15311535
1532- // Create catalog with mocked dependencies that provide filter options with ranges
1536+ // Use a realistic non-zero TypeID to validate that GetFilterOptions
1537+ // correctly scopes context property queries by type.
1538+ const mockTypeID int32 = 42
15331539 mockServices := service.Services {
1534- PropertyOptionsRepository : & mockPropertyRepositoryWithRanges {},
1540+ CatalogModelRepository : & MockCatalogModelRepository {TypeID : mockTypeID },
1541+ PropertyOptionsRepository : & mockPropertyRepositoryWithRanges {
1542+ t : t ,
1543+ expectedTypeID : mockTypeID ,
1544+ },
15351545 }
15361546 catalog := NewDBCatalog (mockServices , sources )
15371547
@@ -1566,21 +1576,20 @@ func TestGetFilterOptionsWithNamedQueries(t *testing.T) {
15661576 assert .Equal (t , 0.0 , rangeQuery ["memory_usage" ].Value , "Expected 'min' to be replaced with 0.0" )
15671577}
15681578
1569- // Mock repository for testing
1570- type mockPropertyRepository struct {}
1571-
1572- func ( m * mockPropertyRepository ) List ( optionType sharedmodels. PropertyOptionType , limit int32 ) ([]sharedmodels. PropertyOption , error ) {
1573- return []sharedmodels. PropertyOption {}, nil
1579+ // Mock repository that provides filter options with numeric ranges for testing min/max transformation.
1580+ // expectedTypeID, when non-zero, asserts the typeID passed to List for context properties.
1581+ type mockPropertyRepositoryWithRanges struct {
1582+ t * testing. T
1583+ expectedTypeID int32
15741584}
15751585
1576- func (m * mockPropertyRepository ) Refresh (optionType sharedmodels.PropertyOptionType ) error {
1577- return nil
1578- }
1579-
1580- // Mock repository that provides filter options with numeric ranges for testing min/max transformation
1581- type mockPropertyRepositoryWithRanges struct {}
1582-
1583- func (m * mockPropertyRepositoryWithRanges ) List (optionType sharedmodels.PropertyOptionType , limit int32 ) ([]sharedmodels.PropertyOption , error ) {
1586+ func (m * mockPropertyRepositoryWithRanges ) List (optionType sharedmodels.PropertyOptionType , typeID int32 ) ([]sharedmodels.PropertyOption , error ) {
1587+ if m .expectedTypeID != 0 && optionType == sharedmodels .ContextPropertyOptionType {
1588+ require .Equal (m .t , m .expectedTypeID , typeID , "expected context property query to be scoped by typeID" )
1589+ }
1590+ if optionType == sharedmodels .ArtifactPropertyOptionType {
1591+ require .Equal (m .t , int32 (0 ), typeID , "artifact property query should not be scoped by typeID" )
1592+ }
15841593 // Return property options with numeric ranges that match the fields used in the test
15851594 minLatency := int64 (10 )
15861595 maxLatency := int64 (500 )
@@ -1933,10 +1942,10 @@ func TestFindModelsWithRecommendedLatency(t *testing.T) {
19331942 defer cleanup ()
19341943
19351944 // Get type IDs
1936- catalogModelTypeID := GetCatalogModelTypeIDForDBTest (t , sharedDB )
1937- modelArtifactTypeID := GetCatalogModelArtifactTypeIDForDBTest (t , sharedDB )
1938- metricsArtifactTypeID := GetCatalogMetricsArtifactTypeIDForDBTest (t , sharedDB )
1939- catalogSourceTypeID := GetCatalogSourceTypeIDForDBTest (t , sharedDB )
1945+ catalogModelTypeID := testhelpers . GetCatalogModelTypeIDForDBTest (t , sharedDB )
1946+ modelArtifactTypeID := testhelpers . GetCatalogModelArtifactTypeIDForDBTest (t , sharedDB )
1947+ metricsArtifactTypeID := testhelpers . GetCatalogMetricsArtifactTypeIDForDBTest (t , sharedDB )
1948+ catalogSourceTypeID := testhelpers . GetCatalogSourceTypeIDForDBTest (t , sharedDB )
19401949
19411950 // Create repositories
19421951 catalogModelRepo := modelservice .NewCatalogModelRepository (sharedDB , catalogModelTypeID )
@@ -2078,3 +2087,123 @@ func TestFindModelsWithRecommendedLatency(t *testing.T) {
20782087 // Custom properties may or may not be set depending on performance data availability
20792088 }
20802089}
2090+
2091+ // TestGetFilterOptions_NoMCPServerContamination verifies that the model catalog's
2092+ // GetFilterOptions only returns properties from kf.CatalogModel contexts, not
2093+ // properties from kf.MCPServer contexts. This is a regression test for the bug
2094+ // where typeID=0 was passed to propertyOptionsRepository.List(), causing
2095+ // cross-contamination between resource types.
2096+ func TestGetFilterOptions_NoMCPServerContamination (t * testing.T ) {
2097+ sharedDB , cleanup := testutils .SetupPostgresWithMigrations (t , service .DatastoreSpec ())
2098+ defer cleanup ()
2099+
2100+ // Get type IDs for both resource types
2101+ catalogModelTypeID := testhelpers .GetCatalogModelTypeIDForDBTest (t , sharedDB )
2102+ modelArtifactTypeID := testhelpers .GetCatalogModelArtifactTypeIDForDBTest (t , sharedDB )
2103+ metricsArtifactTypeID := testhelpers .GetCatalogMetricsArtifactTypeIDForDBTest (t , sharedDB )
2104+ catalogSourceTypeID := testhelpers .GetCatalogSourceTypeIDForDBTest (t , sharedDB )
2105+ mcpServerTypeID := testhelpers .GetMCPServerTypeIDForDBTest (t , sharedDB )
2106+ mcpServerToolTypeID := testhelpers .GetMCPServerToolTypeIDForDBTest (t , sharedDB )
2107+
2108+ // Create repositories for both resource types
2109+ catalogModelRepo := modelservice .NewCatalogModelRepository (sharedDB , catalogModelTypeID )
2110+ catalogArtifactRepo := service .NewCatalogArtifactRepository (sharedDB , map [string ]int32 {
2111+ service .CatalogModelArtifactTypeName : modelArtifactTypeID ,
2112+ service .CatalogMetricsArtifactTypeName : metricsArtifactTypeID ,
2113+ })
2114+ catalogSourceRepo := service .NewCatalogSourceRepository (sharedDB , catalogSourceTypeID )
2115+ mcpServerRepo := mcpservice .NewMCPServerRepository (sharedDB , mcpServerTypeID )
2116+ mcpServerToolRepo := mcpservice .NewMCPServerToolRepository (sharedDB , mcpServerToolTypeID )
2117+ propertyOptionsRepo := service .NewPropertyOptionsRepository (sharedDB )
2118+
2119+ // Create a catalog model with model-specific properties
2120+ catalogModel := & models.CatalogModelImpl {
2121+ TypeID : apiutils .Of (catalogModelTypeID ),
2122+ Attributes : & models.CatalogModelAttributes {
2123+ Name : apiutils .Of ("cross-test-source:cross-test-model" ),
2124+ ExternalID : apiutils .Of ("cross-test-model-ext" ),
2125+ },
2126+ Properties : & []mr_models.Properties {
2127+ {Name : "source_id" , StringValue : apiutils .Of ("cross-test-source" )},
2128+ {Name : "license" , StringValue : apiutils .Of ("Apache-2.0" )},
2129+ {Name : "provider" , StringValue : apiutils .Of ("TestProvider" )},
2130+ {Name : "maturity" , StringValue : apiutils .Of ("stable" )},
2131+ },
2132+ }
2133+ _ , err := catalogModelRepo .Save (catalogModel )
2134+ require .NoError (t , err )
2135+
2136+ // Create an MCP server with MCP-specific properties
2137+ mcpServer := & mcpcatalogmodels.MCPServerImpl {
2138+ TypeID : apiutils .Of (mcpServerTypeID ),
2139+ Attributes : & mcpcatalogmodels.MCPServerAttributes {
2140+ Name : apiutils .Of ("cross-test-mcp-server" ),
2141+ ExternalID : apiutils .Of ("cross-test-mcp-ext" ),
2142+ },
2143+ Properties : & []mr_models.Properties {
2144+ {Name : "source_id" , StringValue : apiutils .Of ("cross-test-mcp-source" )},
2145+ {Name : "version" , StringValue : apiutils .Of ("1.0.0" )},
2146+ {Name : "base_name" , StringValue : apiutils .Of ("cross-test-mcp-server" )},
2147+ {Name : "deploymentMode" , StringValue : apiutils .Of ("remote" )},
2148+ },
2149+ }
2150+ _ , err = mcpServerRepo .Save (mcpServer )
2151+ require .NoError (t , err )
2152+
2153+ // Refresh the materialized views so property options reflect the new data
2154+ require .NoError (t , propertyOptionsRepo .Refresh (sharedmodels .ContextPropertyOptionType ))
2155+ require .NoError (t , propertyOptionsRepo .Refresh (sharedmodels .ArtifactPropertyOptionType ))
2156+
2157+ // Build model catalog services and call GetFilterOptions.
2158+ // NOTE: catalogModelArtifactRepository and catalogMetricsArtifactRepository are nil
2159+ // because GetFilterOptions does not access them. If GetFilterOptions is ever extended
2160+ // to use artifact repositories, this test will panic and must be updated with stubs.
2161+ svcs := service .NewServices (
2162+ catalogModelRepo ,
2163+ catalogArtifactRepo ,
2164+ nil , // catalogModelArtifactRepository — unused by GetFilterOptions
2165+ nil , // catalogMetricsArtifactRepository — unused by GetFilterOptions
2166+ catalogSourceRepo ,
2167+ propertyOptionsRepo ,
2168+ mcpServerRepo ,
2169+ mcpServerToolRepo ,
2170+ )
2171+ dbCatalog := NewDBCatalog (svcs , nil )
2172+
2173+ filterOptions , err := dbCatalog .GetFilterOptions (context .Background ())
2174+ require .NoError (t , err )
2175+ require .NotNil (t , filterOptions )
2176+ require .NotNil (t , filterOptions .Filters )
2177+
2178+ filters := * filterOptions .Filters
2179+
2180+ // Model-specific properties SHOULD be present
2181+ assert .Contains (t , filters , "license" , "model property 'license' should be present" )
2182+ assert .Contains (t , filters , "provider" , "model property 'provider' should be present" )
2183+ assert .Contains (t , filters , "maturity" , "model property 'maturity' should be present" )
2184+
2185+ // MCP-specific properties MUST NOT be present
2186+ assert .NotContains (t , filters , "version" , "MCP property 'version' must not appear in model filter_options" )
2187+ assert .NotContains (t , filters , "base_name" , "MCP property 'base_name' must not appear in model filter_options" )
2188+ assert .NotContains (t , filters , "deploymentMode" , "MCP property 'deploymentMode' must not appear in model filter_options" )
2189+
2190+ // source_id is shared by both types but excluded by both catalogs' skip lists
2191+ assert .NotContains (t , filters , "source_id" , "source_id should be excluded by the model catalog skip list" )
2192+
2193+ // Reverse direction: verify MCP catalog's GetFilterOptions doesn't leak model properties
2194+ dbMCPCatalog := mcpcatalog .NewDBMCPCatalog (svcs , nil , nil )
2195+ mcpFilterOptions , err := dbMCPCatalog .GetFilterOptions (context .Background ())
2196+ require .NoError (t , err )
2197+ require .NotNil (t , mcpFilterOptions )
2198+ require .NotNil (t , mcpFilterOptions .Filters )
2199+
2200+ mcpFilters := * mcpFilterOptions .Filters
2201+
2202+ // Model-specific properties MUST NOT appear in MCP filter_options
2203+ assert .NotContains (t , mcpFilters , "license" , "model property 'license' must not appear in MCP filter_options" )
2204+ assert .NotContains (t , mcpFilters , "provider" , "model property 'provider' must not appear in MCP filter_options" )
2205+ assert .NotContains (t , mcpFilters , "maturity" , "model property 'maturity' must not appear in MCP filter_options" )
2206+
2207+ // source_id is shared by both types but excluded by both catalogs' skip lists
2208+ assert .NotContains (t , mcpFilters , "source_id" , "source_id should be excluded by the MCP catalog skip list" )
2209+ }
0 commit comments