Skip to content

Commit fd4de10

Browse files
grokspawngrokspawn
committed
more review comments
Signed-off-by: grokspawn <jordan@nimblewidget.com>
1 parent 50e7a6e commit fd4de10

6 files changed

Lines changed: 74 additions & 147 deletions

File tree

‎internal/catalogd/graphql/discovery_test.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,9 +103,6 @@ func TestAnalyzeJSONObject_FieldTypes(t *testing.T) {
103103
t.Errorf("Field %s array status: expected %v, got %v", graphqlField, shouldBeArray, fieldInfo.IsArray)
104104
}
105105

106-
if len(fieldInfo.SampleValues) == 0 {
107-
t.Errorf("No sample values recorded for field %s", graphqlField)
108-
}
109106
}
110107

111108
testField("name", false)

‎internal/catalogd/graphql/graphql.go‎

Lines changed: 26 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,13 @@ type FieldInfo struct {
3131
GraphQLType graphql.Type
3232
JSONType reflect.Kind
3333
IsArray bool
34-
SampleValues []interface{}
3534
NestedFields map[string]*FieldInfo // For array-of-objects, stores object structure
3635
}
3736

3837
// SchemaInfo holds discovered schema information
3938
type SchemaInfo struct {
4039
Fields map[string]*FieldInfo
4140
TotalObjects int
42-
SampleObject map[string]interface{}
4341
}
4442

4543
// CatalogSchema holds the complete discovered schema
@@ -334,8 +332,9 @@ func determineFieldType(value interface{}) (reflect.Kind, bool) {
334332
return reflect.TypeOf(firstElem).Kind(), true
335333
}
336334

335+
// maxSampleElements limits how many slice elements are examined when inferring
336+
// GraphQL types from catalog data, bounding work on large arrays.
337337
const maxSampleElements = 10
338-
const maxSampleValues = 10
339338

340339
// analyzeFieldValue analyzes a field value and returns type info, sample value, and nested fields.
341340
// For slices, it examines up to maxSampleElements to detect heterogeneous element types
@@ -427,7 +426,6 @@ func analyzeNestedObject(obj map[string]interface{}) map[string]*FieldInfo {
427426
GraphQLType: jsonTypeToGraphQL(jsonType, isArray, sampleValue),
428427
JSONType: jsonType,
429428
IsArray: isArray,
430-
SampleValues: []interface{}{value},
431429
}
432430
}
433431

@@ -437,15 +435,7 @@ func analyzeNestedObject(obj map[string]interface{}) map[string]*FieldInfo {
437435
// mergeNestedFields merges discovered nested fields into existing ones
438436
func mergeNestedFields(existing, new map[string]*FieldInfo) {
439437
for fieldName, newInfo := range new {
440-
if existingInfo, ok := existing[fieldName]; ok {
441-
// Merge sample values
442-
for _, sample := range newInfo.SampleValues {
443-
if len(existingInfo.SampleValues) >= maxSampleValues {
444-
break
445-
}
446-
existingInfo.SampleValues = appendUnique(existingInfo.SampleValues, sample)
447-
}
448-
} else {
438+
if _, ok := existing[fieldName]; !ok {
449439
existing[fieldName] = newInfo
450440
}
451441
}
@@ -472,7 +462,6 @@ func analyzeJSONObject(obj map[string]interface{}, info *SchemaInfo) {
472462
GraphQLType: jsonTypeToGraphQL(jsonType, isArray, sampleValue),
473463
JSONType: jsonType,
474464
IsArray: isArray,
475-
SampleValues: []interface{}{sampleValue},
476465
NestedFields: nestedFields,
477466
}
478467
continue
@@ -484,11 +473,6 @@ func analyzeJSONObject(obj map[string]interface{}, info *SchemaInfo) {
484473
"graphqlField", fieldName, "existingKey", existing.OriginalName, "newKey", key)
485474
}
486475

487-
// Update existing field
488-
if len(existing.SampleValues) < maxSampleValues {
489-
existing.SampleValues = appendUnique(existing.SampleValues, sampleValue)
490-
}
491-
492476
// Merge nested fields if discovered
493477
if nestedFields == nil {
494478
continue
@@ -501,110 +485,48 @@ func analyzeJSONObject(obj map[string]interface{}, info *SchemaInfo) {
501485
}
502486
}
503487

504-
// appendUnique adds a value to slice if not already present, using JSON string as key for uniqueness
505-
func appendUnique(slice []interface{}, value interface{}) []interface{} {
506-
seen := make(map[string]struct{}, len(slice))
507-
508-
for _, existing := range slice {
509-
key, err := json.Marshal(existing)
510-
if err != nil {
511-
continue // skip values that can't be marshaled
512-
}
513-
seen[string(key)] = struct{}{}
488+
// processMetaIntoSchema parses one meta blob and updates the catalog schema.
489+
// TotalObjects is incremented only on successful parse so pagination counts
490+
// reflect objects that can actually be returned by the ObjectLoader.
491+
func processMetaIntoSchema(catalogSchema *CatalogSchema, meta *declcfg.Meta) {
492+
if meta.Schema == "" {
493+
return
514494
}
515-
516-
valueKey, err := json.Marshal(value)
517-
if err != nil {
518-
return slice // skip value if it can't be marshaled
495+
if catalogSchema.Schemas[meta.Schema] == nil {
496+
catalogSchema.Schemas[meta.Schema] = &SchemaInfo{
497+
Fields: make(map[string]*FieldInfo),
498+
}
519499
}
500+
info := catalogSchema.Schemas[meta.Schema]
520501

521-
if _, exists := seen[string(valueKey)]; exists {
522-
return slice
502+
var obj map[string]interface{}
503+
if err := json.Unmarshal(meta.Blob, &obj); err != nil {
504+
klog.V(4).InfoS("skipping malformed meta blob during schema discovery",
505+
"schema", meta.Schema, "name", meta.Name, "error", err)
506+
return
523507
}
524508

525-
return append(slice, value)
509+
info.TotalObjects++
510+
analyzeJSONObject(obj, info)
526511
}
527512

528-
// DiscoverSchemaFromMetas analyzes Meta objects to discover schema structure
513+
// DiscoverSchemaFromMetas analyzes Meta objects to discover schema structure.
529514
func DiscoverSchemaFromMetas(metas []*declcfg.Meta) (*CatalogSchema, error) {
530-
catalogSchema := &CatalogSchema{
531-
Schemas: make(map[string]*SchemaInfo),
532-
}
533-
534-
// Process each meta object
515+
catalogSchema := &CatalogSchema{Schemas: make(map[string]*SchemaInfo)}
535516
for _, meta := range metas {
536-
if meta.Schema == "" {
537-
continue
538-
}
539-
540-
// Ensure schema info exists
541-
if catalogSchema.Schemas[meta.Schema] == nil {
542-
catalogSchema.Schemas[meta.Schema] = &SchemaInfo{
543-
Fields: make(map[string]*FieldInfo),
544-
TotalObjects: 0,
545-
}
546-
}
547-
548-
info := catalogSchema.Schemas[meta.Schema]
549-
info.TotalObjects++
550-
551-
// Parse the JSON blob
552-
var obj map[string]interface{}
553-
if err := json.Unmarshal(meta.Blob, &obj); err != nil {
554-
klog.V(4).InfoS("skipping malformed meta blob during schema discovery",
555-
"schema", meta.Schema, "name", meta.Name, "error", err)
556-
continue
557-
}
558-
559-
// Store a sample object for reference
560-
if info.SampleObject == nil {
561-
info.SampleObject = obj
562-
}
563-
564-
// Analyze general fields (including nested structures)
565-
analyzeJSONObject(obj, info)
517+
processMetaIntoSchema(catalogSchema, meta)
566518
}
567-
568519
return catalogSchema, nil
569520
}
570521

571522
// DiscoverSchemaFromChannel performs streaming schema discovery, processing
572523
// one meta at a time through a channel. Each meta's blob is parsed, analyzed,
573524
// and then goes out of scope — avoiding accumulation of all blobs in memory.
574525
func DiscoverSchemaFromChannel(metasChan <-chan *declcfg.Meta) (*CatalogSchema, error) {
575-
catalogSchema := &CatalogSchema{
576-
Schemas: make(map[string]*SchemaInfo),
577-
}
578-
526+
catalogSchema := &CatalogSchema{Schemas: make(map[string]*SchemaInfo)}
579527
for meta := range metasChan {
580-
if meta.Schema == "" {
581-
continue
582-
}
583-
584-
if catalogSchema.Schemas[meta.Schema] == nil {
585-
catalogSchema.Schemas[meta.Schema] = &SchemaInfo{
586-
Fields: make(map[string]*FieldInfo),
587-
TotalObjects: 0,
588-
}
589-
}
590-
591-
info := catalogSchema.Schemas[meta.Schema]
592-
info.TotalObjects++
593-
594-
var obj map[string]interface{}
595-
if err := json.Unmarshal(meta.Blob, &obj); err != nil {
596-
klog.V(4).InfoS("skipping malformed meta blob during schema discovery",
597-
"schema", meta.Schema, "name", meta.Name, "error", err)
598-
continue
599-
}
600-
601-
if info.SampleObject == nil {
602-
info.SampleObject = obj
603-
}
604-
605-
analyzeJSONObject(obj, info)
528+
processMetaIntoSchema(catalogSchema, meta)
606529
}
607-
608530
return catalogSchema, nil
609531
}
610532

‎internal/catalogd/graphql/graphql_test.go‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -236,17 +236,12 @@ func TestAnalyzeJSONObject(t *testing.T) {
236236

237237
for origField, expectedType := range expectedFields {
238238
graphqlField := remapFieldName(origField)
239-
fieldInfo, exists := info.Fields[graphqlField]
239+
_, exists := info.Fields[graphqlField]
240240
if !exists {
241241
t.Errorf("Field %s (mapped to %s) not discovered", origField, graphqlField)
242242
continue
243243
}
244244

245-
// Type checking would require GraphQL types, so we just check that it was analyzed
246-
if len(fieldInfo.SampleValues) == 0 {
247-
t.Errorf("No sample values recorded for field %s", graphqlField)
248-
}
249-
250245
_ = expectedType // We can't easily test GraphQL types without the library
251246
}
252247
}
@@ -711,11 +706,8 @@ func TestDiscoverSchemaFromChannel_SkipsMalformedBlob(t *testing.T) {
711706
if info == nil {
712707
t.Fatal("expected schema entry for 'test'")
713708
}
714-
if info.TotalObjects != 1 {
715-
t.Errorf("expected TotalObjects=1 (counted before parse), got %d", info.TotalObjects)
716-
}
717-
if info.SampleObject != nil {
718-
t.Error("expected nil SampleObject for malformed blob")
709+
if info.TotalObjects != 0 {
710+
t.Errorf("expected TotalObjects=0 for malformed blob (not counted until after parse), got %d", info.TotalObjects)
719711
}
720712
}
721713

‎internal/catalogd/graphql/validation.go‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ import (
88
)
99

1010
const (
11-
MaxQueryDepth = 10
12-
MaxQueryAliases = 50
13-
MaxQueryFields = 500
11+
maxQueryDepth = 10
12+
maxQueryAliases = 50
13+
maxQueryFields = 500
1414
)
1515

1616
type queryComplexity struct {
@@ -30,7 +30,6 @@ func ValidateQueryComplexity(query string) error {
3030

3131
c := &queryComplexity{
3232
fragments: make(map[string]*ast.FragmentDefinition),
33-
visited: make(map[string]bool),
3433
}
3534
for _, def := range doc.Definitions {
3635
if frag, ok := def.(*ast.FragmentDefinition); ok {
@@ -39,6 +38,8 @@ func ValidateQueryComplexity(query string) error {
3938
}
4039
for _, def := range doc.Definitions {
4140
if op, ok := def.(*ast.OperationDefinition); ok {
41+
// Reset per operation so shared fragments are counted for each operation.
42+
c.visited = make(map[string]bool)
4243
if err := c.walkSelectionSet(op.SelectionSet, 1); err != nil {
4344
return err
4445
}
@@ -51,21 +52,21 @@ func (c *queryComplexity) walkSelectionSet(ss *ast.SelectionSet, depth int) erro
5152
if ss == nil {
5253
return nil
5354
}
54-
if depth > MaxQueryDepth {
55-
return fmt.Errorf("query exceeds maximum depth of %d", MaxQueryDepth)
55+
if depth > maxQueryDepth {
56+
return fmt.Errorf("query exceeds maximum depth of %d", maxQueryDepth)
5657
}
5758

5859
for _, sel := range ss.Selections {
5960
switch s := sel.(type) {
6061
case *ast.Field:
6162
c.fields++
62-
if c.fields > MaxQueryFields {
63-
return fmt.Errorf("query exceeds maximum field count of %d", MaxQueryFields)
63+
if c.fields > maxQueryFields {
64+
return fmt.Errorf("query exceeds maximum field count of %d", maxQueryFields)
6465
}
6566
if s.Alias != nil && s.Alias.Value != "" {
6667
c.aliases++
67-
if c.aliases > MaxQueryAliases {
68-
return fmt.Errorf("query exceeds maximum alias count of %d", MaxQueryAliases)
68+
if c.aliases > maxQueryAliases {
69+
return fmt.Errorf("query exceeds maximum alias count of %d", maxQueryAliases)
6970
}
7071
}
7172
if err := c.walkSelectionSet(s.SelectionSet, depth+1); err != nil {

‎internal/catalogd/graphql/validation_test.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,14 @@ func TestValidateQueryComplexity_ParseError(t *testing.T) {
2424
}
2525

2626
func TestValidateQueryComplexity_ExceedsDepth(t *testing.T) {
27-
// Build a query that exceeds MaxQueryDepth (10)
27+
// Build a query that exceeds maxQueryDepth (10)
2828
var b strings.Builder
2929
b.WriteString("{ ")
30-
for i := 0; i <= MaxQueryDepth+1; i++ {
30+
for i := 0; i <= maxQueryDepth+1; i++ {
3131
b.WriteString(fmt.Sprintf("f%d { ", i))
3232
}
3333
b.WriteString("leaf")
34-
for i := 0; i <= MaxQueryDepth+1; i++ {
34+
for i := 0; i <= maxQueryDepth+1; i++ {
3535
b.WriteString(" }")
3636
}
3737
b.WriteString(" }")
@@ -46,14 +46,14 @@ func TestValidateQueryComplexity_ExceedsDepth(t *testing.T) {
4646
}
4747

4848
func TestValidateQueryComplexity_WithinDepthLimit(t *testing.T) {
49-
// Build a query at exactly MaxQueryDepth (should pass)
49+
// Build a query at exactly maxQueryDepth (should pass)
5050
var b strings.Builder
5151
b.WriteString("{ ")
52-
for i := 1; i < MaxQueryDepth; i++ {
52+
for i := 1; i < maxQueryDepth; i++ {
5353
b.WriteString(fmt.Sprintf("f%d { ", i))
5454
}
5555
b.WriteString("leaf")
56-
for i := 1; i < MaxQueryDepth; i++ {
56+
for i := 1; i < maxQueryDepth; i++ {
5757
b.WriteString(" }")
5858
}
5959
b.WriteString(" }")
@@ -67,7 +67,7 @@ func TestValidateQueryComplexity_WithinDepthLimit(t *testing.T) {
6767
func TestValidateQueryComplexity_ExceedsAliases(t *testing.T) {
6868
var b strings.Builder
6969
b.WriteString("{ ")
70-
for i := 0; i <= MaxQueryAliases; i++ {
70+
for i := 0; i <= maxQueryAliases; i++ {
7171
b.WriteString(fmt.Sprintf("a%d: name ", i))
7272
}
7373
b.WriteString("}")
@@ -84,7 +84,7 @@ func TestValidateQueryComplexity_ExceedsAliases(t *testing.T) {
8484
func TestValidateQueryComplexity_ExceedsFieldCount(t *testing.T) {
8585
var b strings.Builder
8686
b.WriteString("{ ")
87-
for i := 0; i <= MaxQueryFields; i++ {
87+
for i := 0; i <= maxQueryFields; i++ {
8888
b.WriteString(fmt.Sprintf("f%d ", i))
8989
}
9090
b.WriteString("}")

0 commit comments

Comments
 (0)