Skip to content

Commit 579e15e

Browse files
chambridgeclaude
chambridge
and
claude
authored
feat(catalog): return all sources regardless of enabled field value (kubeflow#1976)
Implements - Sources endpoint should return all sources regardless of the enabled field's value 🤖 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Chris Hambridge <chambrid@redhat.com> Co-authored-by: Claude <noreply@anthropic.com>
1 parent d575328 commit 579e15e

5 files changed

Lines changed: 262 additions & 11 deletions

File tree

‎catalog/internal/catalog/catalog_test.go‎

Lines changed: 89 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,7 @@ func TestLoadCatalogSources(t *testing.T) {
2929
{
3030
name: "test-catalog-sources",
3131
args: args{catalogsPath: "testdata/test-catalog-sources.yaml"},
32-
want: []string{"catalog1"},
33-
wantErr: false,
32+
want: []string{"catalog1", "catalog2"},
3433
},
3534
}
3635
for _, tt := range tests {
@@ -63,6 +62,7 @@ func TestLoadCatalogSources(t *testing.T) {
6362

6463
func TestLoadCatalogSourcesEnabledDisabled(t *testing.T) {
6564
trueValue := true
65+
falseValue := false
6666
type args struct {
6767
catalogsPath string
6868
}
@@ -82,6 +82,12 @@ func TestLoadCatalogSourcesEnabledDisabled(t *testing.T) {
8282
Enabled: &trueValue,
8383
Labels: []string{},
8484
},
85+
"catalog2": {
86+
Id: "catalog2",
87+
Name: "Catalog 2",
88+
Enabled: &falseValue,
89+
Labels: []string{},
90+
},
8591
},
8692
wantErr: false,
8793
},
@@ -470,6 +476,87 @@ func TestLoadCatalogSourcesWithRepositoryErrors(t *testing.T) {
470476
}
471477
}
472478

479+
func TestLoadCatalogSourcesWithNilEnabled(t *testing.T) {
480+
// Test that nil Enabled field is treated as enabled (per OpenAPI spec default: true)
481+
mockModelRepo := &MockCatalogModelRepository{}
482+
mockArtifactRepo := &MockCatalogArtifactRepository{}
483+
mockModelArtifactRepo := &MockCatalogModelArtifactRepository{}
484+
mockMetricsArtifactRepo := &MockCatalogMetricsArtifactRepository{}
485+
486+
services := service.NewServices(
487+
mockModelRepo,
488+
mockArtifactRepo,
489+
mockModelArtifactRepo,
490+
mockMetricsArtifactRepo,
491+
&MockPropertyOptionsRepository{},
492+
)
493+
494+
// Register a test provider
495+
testProviderName := "test-nil-enabled-provider"
496+
RegisterModelProvider(testProviderName, func(ctx context.Context, source *Source, reldir string) (<-chan ModelProviderRecord, error) {
497+
ch := make(chan ModelProviderRecord, 1)
498+
499+
modelName := "test-model-nil-enabled"
500+
model := &dbmodels.CatalogModelImpl{
501+
Attributes: &dbmodels.CatalogModelAttributes{
502+
Name: &modelName,
503+
},
504+
}
505+
506+
ch <- ModelProviderRecord{
507+
Model: model,
508+
Artifacts: []dbmodels.CatalogArtifact{},
509+
}
510+
close(ch)
511+
512+
return ch, nil
513+
})
514+
515+
testConfig := &sourceConfig{
516+
Catalogs: []Source{
517+
{
518+
CatalogSource: apimodels.CatalogSource{
519+
Id: "test-catalog-nil-enabled",
520+
Name: "Test Catalog Nil Enabled",
521+
Enabled: nil, // Nil should be treated as enabled
522+
},
523+
Type: testProviderName,
524+
},
525+
},
526+
}
527+
528+
l := NewLoader(services, []string{})
529+
ctx := context.Background()
530+
531+
// First call updateSources to populate the SourceCollection
532+
err := l.updateSources("test-path", testConfig)
533+
if err != nil {
534+
t.Fatalf("updateSources() error = %v", err)
535+
}
536+
537+
err = l.updateDatabase(ctx)
538+
if err != nil {
539+
t.Fatalf("updateDatabase() error = %v", err)
540+
}
541+
542+
// Wait for processing
543+
time.Sleep(100 * time.Millisecond)
544+
545+
// Verify that the model WAS saved (because nil Enabled is treated as enabled)
546+
if len(mockModelRepo.SavedModels) != 1 {
547+
t.Errorf("Expected 1 model to be saved (nil Enabled should be treated as enabled), got %d", len(mockModelRepo.SavedModels))
548+
}
549+
550+
if len(mockModelRepo.SavedModels) > 0 {
551+
savedModel := mockModelRepo.SavedModels[0]
552+
if savedModel.GetAttributes() == nil || savedModel.GetAttributes().Name == nil {
553+
t.Error("Saved model should have attributes with name")
554+
} else if *savedModel.GetAttributes().Name != "test-model-nil-enabled" {
555+
t.Errorf("Expected model name 'test-model-nil-enabled', got '%s'", *savedModel.GetAttributes().Name)
556+
}
557+
}
558+
}
559+
473560
func TestMockRepositoryBehavior(t *testing.T) {
474561
mockRepo := &MockCatalogModelRepository{}
475562

‎catalog/internal/catalog/loader.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -330,12 +330,18 @@ func (l *Loader) readProviderRecords(ctx context.Context) <-chan ModelProviderRe
330330
ch := make(chan ModelProviderRecord)
331331
var wg sync.WaitGroup
332332

333-
// Get all enabled sources from the merged collection.
333+
// Get all sources from the merged collection.
334334
// This allows sparse overrides to work: a user can enable a disabled source
335335
// with just "id" and "enabled: true", inheriting Type and Properties from the base.
336336
mergedSources := l.Sources.AllSources()
337337

338338
for _, source := range mergedSources {
339+
// Skip disabled sources - only load catalog data from enabled sources
340+
// Per OpenAPI spec, enabled defaults to true, so nil is treated as enabled
341+
if source.Enabled != nil && !*source.Enabled {
342+
continue
343+
}
344+
339345
// Skip sources that have already been loaded
340346
if l.loadedSources[source.Id] {
341347
continue

‎catalog/internal/catalog/sources.go‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -159,21 +159,19 @@ func (sc *SourceCollection) merged() map[string]Source {
159159

160160
// AllSources returns all merged sources including Type and Properties.
161161
// This is used by the loader to get complete source information.
162-
// Only enabled sources are returned.
162+
// All sources are returned regardless of enabled status.
163163
func (sc *SourceCollection) AllSources() map[string]Source {
164164
sc.mu.RLock()
165165
defer sc.mu.RUnlock()
166166

167167
result := map[string]Source{}
168168
for id, source := range sc.merged() {
169-
if source.Enabled != nil && *source.Enabled {
170-
result[id] = source
171-
}
169+
result[id] = source
172170
}
173171
return result
174172
}
175173

176-
// All returns all enabled sources as CatalogSource (for the API).
174+
// All returns all sources as CatalogSource (for the API).
177175
// This excludes internal fields like Type and Properties.
178176
func (sc *SourceCollection) All() map[string]model.CatalogSource {
179177
result := map[string]model.CatalogSource{}

‎catalog/internal/catalog/sources_test.go‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -412,7 +412,7 @@ func TestSourceCollection_MergeOverride(t *testing.T) {
412412
},
413413
},
414414
{
415-
name: "user can disable a source from default - disabled sources not returned",
415+
name: "user can disable a source from default - disabled sources ARE returned",
416416
originOrder: []string{"default.yaml", "user.yaml"},
417417
mergeSequence: []struct {
418418
origin string
@@ -441,8 +441,15 @@ func TestSourceCollection_MergeOverride(t *testing.T) {
441441
},
442442
},
443443
},
444-
// Disabled sources are not returned by All()
445-
expectedSources: map[string]model.CatalogSource{},
444+
// Disabled sources ARE returned by All() - with enabled field showing false
445+
expectedSources: map[string]model.CatalogSource{
446+
"hf": {
447+
Id: "hf",
448+
Name: "Hugging Face",
449+
Enabled: apiutils.Of(false),
450+
Labels: []string{},
451+
},
452+
},
446453
},
447454
{
448455
name: "sparse override: user enables disabled source with just id and enabled",

‎catalog/internal/server/openapi/api_model_catalog_service_service_test.go‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,7 @@ func TestFindSources(t *testing.T) {
354354
expectedSize int32
355355
expectedItems int
356356
checkSorting bool
357+
checkEnabled bool
357358
expectedLabels int
358359
}{
359360
{
@@ -586,6 +587,151 @@ func TestFindSources(t *testing.T) {
586587
},
587588
}
588589

590+
falseValue := false
591+
newTestCases := []struct {
592+
name string
593+
catalogs map[string]catalog.Source
594+
nameFilter string
595+
pageSize string
596+
orderBy model.OrderByField
597+
sortOrder model.SortOrder
598+
nextPageToken string
599+
expectedStatus int
600+
expectedSize int32
601+
expectedItems int
602+
checkSorting bool
603+
checkEnabled bool
604+
expectedLabels int
605+
}{
606+
{
607+
name: "All sources returned regardless of enabled status",
608+
catalogs: map[string]catalog.Source{
609+
"enabled1": {CatalogSource: model.CatalogSource{Id: "enabled1", Name: "Enabled Source 1", Enabled: &trueValue}},
610+
"disabled1": {CatalogSource: model.CatalogSource{Id: "disabled1", Name: "Disabled Source 1", Enabled: &falseValue}},
611+
"enabled2": {CatalogSource: model.CatalogSource{Id: "enabled2", Name: "Enabled Source 2", Enabled: &trueValue}},
612+
"disabled2": {CatalogSource: model.CatalogSource{Id: "disabled2", Name: "Disabled Source 2", Enabled: &falseValue}},
613+
},
614+
nameFilter: "",
615+
pageSize: "10",
616+
orderBy: model.ORDERBYFIELD_ID,
617+
sortOrder: model.SORTORDER_ASC,
618+
expectedStatus: http.StatusOK,
619+
expectedSize: 4,
620+
expectedItems: 4,
621+
checkEnabled: true,
622+
},
623+
{
624+
name: "Enabled field present in response for all sources",
625+
catalogs: map[string]catalog.Source{
626+
"enabled1": {CatalogSource: model.CatalogSource{Id: "enabled1", Name: "Enabled Source 1", Enabled: &trueValue}},
627+
"disabled1": {CatalogSource: model.CatalogSource{Id: "disabled1", Name: "Disabled Source 1", Enabled: &falseValue}},
628+
},
629+
nameFilter: "",
630+
pageSize: "10",
631+
orderBy: model.ORDERBYFIELD_ID,
632+
sortOrder: model.SORTORDER_ASC,
633+
expectedStatus: http.StatusOK,
634+
expectedSize: 2,
635+
expectedItems: 2,
636+
checkEnabled: true,
637+
},
638+
{
639+
name: "Name filtering works across all sources (enabled and disabled)",
640+
catalogs: map[string]catalog.Source{
641+
"enabled1": {CatalogSource: model.CatalogSource{Id: "enabled1", Name: "Test Source A", Enabled: &trueValue}},
642+
"disabled1": {CatalogSource: model.CatalogSource{Id: "disabled1", Name: "Test Source B", Enabled: &falseValue}},
643+
"enabled2": {CatalogSource: model.CatalogSource{Id: "enabled2", Name: "Other Source", Enabled: &trueValue}},
644+
"disabled2": {CatalogSource: model.CatalogSource{Id: "disabled2", Name: "Test Source C", Enabled: &falseValue}},
645+
},
646+
nameFilter: "Test",
647+
pageSize: "10",
648+
orderBy: model.ORDERBYFIELD_ID,
649+
sortOrder: model.SORTORDER_ASC,
650+
expectedStatus: http.StatusOK,
651+
expectedSize: 3, // Should find both enabled and disabled sources with "Test" in name
652+
expectedItems: 3,
653+
},
654+
{
655+
name: "Pagination accounts for all sources including disabled",
656+
catalogs: map[string]catalog.Source{
657+
"enabled1": {CatalogSource: model.CatalogSource{Id: "enabled1", Name: "Source 1", Enabled: &trueValue}},
658+
"disabled1": {CatalogSource: model.CatalogSource{Id: "disabled1", Name: "Source 2", Enabled: &falseValue}},
659+
"enabled2": {CatalogSource: model.CatalogSource{Id: "enabled2", Name: "Source 3", Enabled: &trueValue}},
660+
"disabled2": {CatalogSource: model.CatalogSource{Id: "disabled2", Name: "Source 4", Enabled: &falseValue}},
661+
"enabled3": {CatalogSource: model.CatalogSource{Id: "enabled3", Name: "Source 5", Enabled: &trueValue}},
662+
},
663+
nameFilter: "",
664+
pageSize: "2",
665+
orderBy: model.ORDERBYFIELD_ID,
666+
sortOrder: model.SORTORDER_ASC,
667+
expectedStatus: http.StatusOK,
668+
expectedSize: 2, // Page size, not total
669+
expectedItems: 2, // First page should have 2 items
670+
},
671+
{
672+
name: "Sorting works across all sources (enabled and disabled interleaved)",
673+
catalogs: map[string]catalog.Source{
674+
"source_d": {CatalogSource: model.CatalogSource{Id: "source_d", Name: "D Source", Enabled: &falseValue}},
675+
"source_b": {CatalogSource: model.CatalogSource{Id: "source_b", Name: "B Source", Enabled: &trueValue}},
676+
"source_a": {CatalogSource: model.CatalogSource{Id: "source_a", Name: "A Source", Enabled: &falseValue}},
677+
"source_c": {CatalogSource: model.CatalogSource{Id: "source_c", Name: "C Source", Enabled: &trueValue}},
678+
},
679+
nameFilter: "",
680+
pageSize: "10",
681+
orderBy: model.ORDERBYFIELD_ID,
682+
sortOrder: model.SORTORDER_ASC,
683+
expectedStatus: http.StatusOK,
684+
expectedSize: 4,
685+
expectedItems: 4,
686+
checkSorting: true, // Should be sorted a, b, c, d regardless of enabled status
687+
},
688+
{
689+
name: "Empty catalog returns empty list",
690+
catalogs: map[string]catalog.Source{},
691+
nameFilter: "",
692+
pageSize: "10",
693+
orderBy: model.ORDERBYFIELD_ID,
694+
sortOrder: model.SORTORDER_ASC,
695+
expectedStatus: http.StatusOK,
696+
expectedSize: 0,
697+
expectedItems: 0,
698+
},
699+
{
700+
name: "All sources disabled still returns all sources",
701+
catalogs: map[string]catalog.Source{
702+
"disabled1": {CatalogSource: model.CatalogSource{Id: "disabled1", Name: "Disabled Source 1", Enabled: &falseValue}},
703+
"disabled2": {CatalogSource: model.CatalogSource{Id: "disabled2", Name: "Disabled Source 2", Enabled: &falseValue}},
704+
"disabled3": {CatalogSource: model.CatalogSource{Id: "disabled3", Name: "Disabled Source 3", Enabled: &falseValue}},
705+
},
706+
nameFilter: "",
707+
pageSize: "10",
708+
orderBy: model.ORDERBYFIELD_ID,
709+
sortOrder: model.SORTORDER_ASC,
710+
expectedStatus: http.StatusOK,
711+
expectedSize: 3,
712+
expectedItems: 3,
713+
checkEnabled: true,
714+
},
715+
{
716+
name: "Sorting by NAME works across enabled and disabled sources",
717+
catalogs: map[string]catalog.Source{
718+
"source1": {CatalogSource: model.CatalogSource{Id: "source1", Name: "Zebra Catalog", Enabled: &falseValue}},
719+
"source2": {CatalogSource: model.CatalogSource{Id: "source2", Name: "Alpha Catalog", Enabled: &trueValue}},
720+
"source3": {CatalogSource: model.CatalogSource{Id: "source3", Name: "Beta Catalog", Enabled: &falseValue}},
721+
"source4": {CatalogSource: model.CatalogSource{Id: "source4", Name: "Gamma Catalog", Enabled: &trueValue}},
722+
},
723+
nameFilter: "",
724+
pageSize: "10",
725+
orderBy: model.ORDERBYFIELD_NAME,
726+
sortOrder: model.SORTORDER_ASC,
727+
expectedStatus: http.StatusOK,
728+
expectedSize: 4,
729+
expectedItems: 4,
730+
checkSorting: true,
731+
},
732+
}
733+
testCases = append(testCases, newTestCases...)
734+
589735
// Run test cases
590736
for _, tc := range testCases {
591737
t.Run(tc.name, func(t *testing.T) {
@@ -676,6 +822,13 @@ func TestFindSources(t *testing.T) {
676822
labels = append(labels, item.Labels...)
677823
}
678824
assert.Equal(t, tc.expectedLabels, len(labels))
825+
826+
// Check enabled field if required
827+
if tc.checkEnabled {
828+
for _, item := range sourceList.Items {
829+
assert.NotNil(t, item.Enabled, "Enabled field should be present for source %s", item.Id)
830+
}
831+
}
679832
})
680833
}
681834
}

0 commit comments

Comments
 (0)