Skip to content
This repository was archived by the owner on Sep 9, 2026. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions internal/api/osac/private/v1/field_definition_type.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions internal/api/osac/public/v1/field_definition_type.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

185 changes: 185 additions & 0 deletions internal/servers/catalog_item_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"errors"
"fmt"
"slices"
"sort"
"strconv"
"strings"
"time"
Expand All @@ -28,11 +29,13 @@ import (
grpcstatus "google.golang.org/grpc/status"
"google.golang.org/protobuf/encoding/protojson"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/types/known/structpb"

privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1"
"github.com/osac-project/fulfillment-service/internal/database/dao"
"github.com/osac-project/fulfillment-service/internal/maputil"
"github.com/osac-project/fulfillment-service/internal/utils"
)

// catalogItem is implemented by both ClusterCatalogItem and ComputeInstanceCatalogItem.
Expand Down Expand Up @@ -64,6 +67,188 @@ func validateFieldDefinitions(fieldDefinitions []*privatev1.FieldDefinition) err
return nil
}

// validateFieldDefinitionPaths checks that each field definition path corresponds to a valid field
// in the given spec message descriptor. Paths starting with "template_parameters" are skipped
// (validated separately by validateFieldDefinitionTemplateParams).
func validateFieldDefinitionPaths(
fieldDefinitions []*privatev1.FieldDefinition,
specDescriptor protoreflect.MessageDescriptor,
) error {
for _, fd := range fieldDefinitions {
path := fd.GetPath()
if path == "" {
continue
}
segments := strings.Split(path, ".")
if segments[0] == "template_parameters" {
if len(segments) == 1 {
return grpcstatus.Errorf(grpccodes.InvalidArgument,
"invalid field_definition path 'template_parameters': "+
"must specify a parameter name (e.g., 'template_parameters.param_name')")
}
continue
Comment on lines +70 to +89

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Path validation stops at first error while template param validation collects all

validateFieldDefinitionPaths returns on the first invalid path, while validateFieldDefinitionTemplateParams collects all invalid parameters and reports them together. Consider collecting all invalid paths too so the caller can fix everything in one round trip. Minor UX inconsistency.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: "template_parameters" is hardcoded in 5 places across this file (lines 77, 80, 120, 189, 231+). Consider extracting a const like templateParametersPrefix = "template_parameters." - there is no existing constant in the codebase, but this PR adds enough new uses to justify one.

}
if err := validatePathAgainstDescriptor(segments, specDescriptor); err != nil {
return grpcstatus.Errorf(grpccodes.InvalidArgument,
"invalid field_definition path '%s': %v", path, err)
}
}
return nil
}

func validatePathAgainstDescriptor(segments []string, md protoreflect.MessageDescriptor) error {
fields := md.Fields()
fd := fields.ByName(protoreflect.Name(segments[0]))
if fd == nil {
return fmt.Errorf("field '%s' does not exist; valid fields: %s",
segments[0], listFieldNames(fields))
}

remaining := segments[1:]
if len(remaining) == 0 {
return nil
}

if fd.IsMap() {
// Map field: next segment is a key (any value), then continue with value descriptor.
remaining = remaining[1:]
if len(remaining) == 0 {
return nil
}
valueDesc := fd.MapValue()
if valueDesc.Kind() != protoreflect.MessageKind {
return fmt.Errorf("field '%s' is a map with scalar values; path cannot continue beyond the key",
segments[0])
}
return validatePathAgainstDescriptor(remaining, valueDesc.Message())
}

if fd.Kind() == protoreflect.MessageKind {
return validatePathAgainstDescriptor(remaining, fd.Message())
}

return fmt.Errorf("field '%s' is a scalar; path cannot continue with '%s'",
segments[0], remaining[0])
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

func listFieldNames(fields protoreflect.FieldDescriptors) string {
names := make([]string, 0, fields.Len())
for i := range fields.Len() {
names = append(names, string(fields.Get(i).Name()))
}
sort.Strings(names)
return strings.Join(names, ", ")
}

// validateFieldDefinitionTemplateParams checks that template_parameters.* paths in field definitions
// correspond to parameters declared in the referenced template.
func validateFieldDefinitionTemplateParams(
fieldDefinitions []*privatev1.FieldDefinition,
template utils.Template,
) error {
var paramPaths []string
for _, fd := range fieldDefinitions {
path := fd.GetPath()
if strings.HasPrefix(path, "template_parameters.") {
paramPaths = append(paramPaths, path)
}
}
if len(paramPaths) == 0 {
return nil
}

validNames := make(map[string]bool)
for _, p := range template.GetParameters() {
validNames[p.GetName()] = true
}

var invalid []string
for _, path := range paramPaths {
paramName := strings.TrimPrefix(path, "template_parameters.")
if !validNames[paramName] {
invalid = append(invalid, paramName)
}
}

if len(invalid) > 0 {
sort.Strings(invalid)
validList := make([]string, 0, len(validNames))
for name := range validNames {
validList = append(validList, name)
}
sort.Strings(validList)
return grpcstatus.Errorf(grpccodes.InvalidArgument,
"field_definition references unknown template parameter(s) %s; "+
"valid parameters for template '%s': %s",
strings.Join(invalid, ", "),
template.GetId(),
strings.Join(validList, ", "))
}
return nil
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// templateResolver looks up a template by ID and returns it wrapped as a utils.Template.
type templateResolver func(ctx context.Context, id string) (utils.Template, error)

// validateCatalogItemFieldDefinitionPaths validates that field definition paths match the spec
// proto schema and that template_parameters paths reference valid template parameters.
func validateCatalogItemFieldDefinitionPaths(
ctx context.Context,
item catalogItem,
specDescriptor protoreflect.MessageDescriptor,
resolve templateResolver,
) error {
fieldDefs := item.GetFieldDefinitions()
if len(fieldDefs) == 0 {
return nil
}

if err := validateFieldDefinitionPaths(fieldDefs, specDescriptor); err != nil {
return err
}

return validateTemplateParams(ctx, item.GetTemplate(), fieldDefs, resolve)
}

func validateTemplateParams(
ctx context.Context, templateRef string,
fieldDefs []*privatev1.FieldDefinition,
resolve templateResolver,
) error {
hasTemplateParamPaths := false
for _, fd := range fieldDefs {
if strings.HasPrefix(fd.GetPath(), "template_parameters.") {
hasTemplateParamPaths = true
break
}
}
if !hasTemplateParamPaths {
return nil
}

if templateRef == "" {
return grpcstatus.Errorf(grpccodes.InvalidArgument,
"field_definitions reference template_parameters but no template is set")
}

template, err := resolve(ctx, templateRef)
if err != nil {
return err
}

return validateFieldDefinitionTemplateParams(fieldDefs, template)
}

func templateLookupError(id string, err error) error {
var notFoundErr *dao.ErrNotFound
if errors.As(err, &notFoundErr) {
return grpcstatus.Errorf(grpccodes.InvalidArgument,
"template '%s' not found", id)
}
return grpcstatus.Errorf(grpccodes.Internal,
"failed to retrieve template '%s'", id)
}

// applyFieldDefinitions validates and applies field definitions from a catalog item against a resource spec.
// Rejects any spec field not listed in field_definitions (except system fields catalog_item and template).
// For non-editable fields: rejects user-provided values; applies the catalog item default.
Expand Down
Loading
Loading