diff --git a/internal/schema/service.go b/internal/schema/service.go index 72b2cb67..0e91d508 100644 --- a/internal/schema/service.go +++ b/internal/schema/service.go @@ -713,6 +713,9 @@ func (s *Service) autoPublish(ctx context.Context, resp *pb.ImportSchemaResponse } func (s *Service) createFields(ctx context.Context, versionID string, fields []*pb.SchemaField) ([]domain.SchemaField, error) { + if err := validateNoPrefixOverlap(fields); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "%v", err) + } result := make([]domain.SchemaField, 0, len(fields)) for _, f := range fields { if err := validateFieldConstraints(f); err != nil { diff --git a/internal/schema/validate_constraints.go b/internal/schema/validate_constraints.go index f6a70acc..dcf9f5e8 100644 --- a/internal/schema/validate_constraints.go +++ b/internal/schema/validate_constraints.go @@ -2,10 +2,40 @@ package schema import ( "fmt" + "sort" + "strings" pb "github.com/opendecree/decree/api/centralconfig/v1" ) +// validateNoPrefixOverlap rejects schemas where one field path is a strict +// prefix of another (e.g. "payments" alongside "payments.fee"). Such schemas +// cannot be presented as a tree without ambiguity — the same node would need +// to be both a typed leaf and a parent containing other leaves. Tooling that +// consumes the schema as a tree (UI, code generators, doc generators, future +// CEL `self` binding) would all hit the same conflict. +// +// Sorted-pair adjacency is sufficient: if A is a strict prefix of any path B, +// then A < B lexicographically and no other path C can sort between them +// without also being prefixed by A. +func validateNoPrefixOverlap(fields []*pb.SchemaField) error { + if len(fields) < 2 { + return nil + } + paths := make([]string, len(fields)) + for i, f := range fields { + paths[i] = f.Path + } + sort.Strings(paths) + for i := 1; i < len(paths); i++ { + prev, cur := paths[i-1], paths[i] + if strings.HasPrefix(cur, prev+".") { + return fmt.Errorf("field paths conflict: %q is a strict prefix of %q (a path cannot be both a leaf and a parent)", prev, cur) + } + } + return nil +} + // validateFieldConstraints checks that constraints are applicable to the field type. // Returns an error if a constraint is applied to an incompatible type. func validateFieldConstraints(field *pb.SchemaField) error { diff --git a/internal/schema/validate_constraints_bench_test.go b/internal/schema/validate_constraints_bench_test.go new file mode 100644 index 00000000..174a8693 --- /dev/null +++ b/internal/schema/validate_constraints_bench_test.go @@ -0,0 +1,57 @@ +package schema + +import ( + "fmt" + "testing" + + pb "github.com/opendecree/decree/api/centralconfig/v1" +) + +func makeFields(n int, overlapPair bool) []*pb.SchemaField { + fields := make([]*pb.SchemaField, 0, n) + for i := range n { + fields = append(fields, &pb.SchemaField{ + Path: fmt.Sprintf("group_%04d.field_%04d", i/10, i), + Type: pb.FieldType_FIELD_TYPE_STRING, + }) + } + if overlapPair && n >= 2 { + // Force the last sorted pair to overlap by injecting "x" and "x.y". + fields[0].Path = "x" + fields[1].Path = "x.y" + } + return fields +} + +func BenchmarkValidateNoPrefixOverlap_NoOverlap10(b *testing.B) { + fields := makeFields(10, false) + b.ResetTimer() + for b.Loop() { + _ = validateNoPrefixOverlap(fields) + } +} + +func BenchmarkValidateNoPrefixOverlap_NoOverlap100(b *testing.B) { + fields := makeFields(100, false) + b.ResetTimer() + for b.Loop() { + _ = validateNoPrefixOverlap(fields) + } +} + +func BenchmarkValidateNoPrefixOverlap_NoOverlap1000(b *testing.B) { + fields := makeFields(1000, false) + b.ResetTimer() + for b.Loop() { + _ = validateNoPrefixOverlap(fields) + } +} + +func BenchmarkValidateNoPrefixOverlap_OverlapEarly(b *testing.B) { + // Conflict surfaces in the first adjacent pair after sort. + fields := makeFields(1000, true) + b.ResetTimer() + for b.Loop() { + _ = validateNoPrefixOverlap(fields) + } +} diff --git a/internal/schema/validate_constraints_test.go b/internal/schema/validate_constraints_test.go index 6d00c119..db353863 100644 --- a/internal/schema/validate_constraints_test.go +++ b/internal/schema/validate_constraints_test.go @@ -172,3 +172,86 @@ func TestValidateConstraints_MinLengthGreaterThanMaxLength_Rejected(t *testing.T assert.Error(t, err) assert.Contains(t, err.Error(), "minLength") } + +// --- Prefix-overlap (cross-field) --- + +func mkField(path string) *pb.SchemaField { + return &pb.SchemaField{Path: path, Type: pb.FieldType_FIELD_TYPE_STRING} +} + +func TestValidateNoPrefixOverlap_Empty(t *testing.T) { + require.NoError(t, validateNoPrefixOverlap(nil)) + require.NoError(t, validateNoPrefixOverlap([]*pb.SchemaField{})) +} + +func TestValidateNoPrefixOverlap_Single(t *testing.T) { + require.NoError(t, validateNoPrefixOverlap([]*pb.SchemaField{mkField("payments")})) +} + +func TestValidateNoPrefixOverlap_NoOverlap(t *testing.T) { + require.NoError(t, validateNoPrefixOverlap([]*pb.SchemaField{ + mkField("payments.fee"), + mkField("payments.window"), + mkField("billing.invoice"), + })) +} + +func TestValidateNoPrefixOverlap_Direct(t *testing.T) { + err := validateNoPrefixOverlap([]*pb.SchemaField{ + mkField("payments"), + mkField("payments.fee"), + }) + require.Error(t, err) + assert.Contains(t, err.Error(), `"payments"`) + assert.Contains(t, err.Error(), `"payments.fee"`) +} + +func TestValidateNoPrefixOverlap_DeepGap(t *testing.T) { + // "payments" is a strict prefix of "payments.refunds.window" even though + // the intermediate path "payments.refunds" is not declared. + err := validateNoPrefixOverlap([]*pb.SchemaField{ + mkField("payments"), + mkField("payments.refunds.window"), + }) + require.Error(t, err) +} + +func TestValidateNoPrefixOverlap_TransitiveSiblings(t *testing.T) { + // Sibling siblings are fine; only ancestor-descendant overlap fails. + err := validateNoPrefixOverlap([]*pb.SchemaField{ + mkField("a.b"), + mkField("a.b.c"), + mkField("a.b.d"), + }) + require.Error(t, err) + // First sorted pair (a.b, a.b.c) trips the check. + assert.Contains(t, err.Error(), `"a.b"`) + assert.Contains(t, err.Error(), `"a.b.c"`) +} + +func TestValidateNoPrefixOverlap_SubstringNotPrefix(t *testing.T) { + // "payment" and "payments" share a string prefix but no path-segment + // boundary — must NOT be flagged. + require.NoError(t, validateNoPrefixOverlap([]*pb.SchemaField{ + mkField("payment"), + mkField("payments"), + })) +} + +func TestValidateNoPrefixOverlap_SiblingsWithCommonPrefix(t *testing.T) { + // "payments.fee" and "payments.fees" — neither is an ancestor of the other. + require.NoError(t, validateNoPrefixOverlap([]*pb.SchemaField{ + mkField("payments.fee"), + mkField("payments.fees"), + })) +} + +func TestValidateNoPrefixOverlap_UnsortedInput(t *testing.T) { + // Conflict must be caught regardless of input order. + err := validateNoPrefixOverlap([]*pb.SchemaField{ + mkField("payments.refunds.window"), + mkField("billing"), + mkField("payments"), + }) + require.Error(t, err) +}