Address PR review feedback: validator registry and map type

- Change identifying_properties from Struct to map<string, Value>
  for better TypeScript codegen (per Evan's feedback)
- Implement ValidatorRegistry with AddValidator/RemoveValidator
  pattern for entity-type-specific validation
- Update RepositoryValidator to new interface (no entity type param)
- Add clarifying comments for transaction boundaries and child
  entity reconciliation in add_originating_entity.go
- Update all tests and mocks for new patterns

Co-Authored-By: Claude Opus 4.5 <[email protected]>

Author: JAORMX

docs/docs/ref/proto.mdx

@@ -247,26 +247,32 @@ func (s *Server) RegisterEntity(
 // parseIdentifyingProperties converts proto properties to Properties object
 func parseIdentifyingProperties(req *pb.RegisterEntityRequest) (*properties.Properties, error) {
 	identifyingProps := req.GetIdentifyingProperties()
-	if identifyingProps == nil {
+	if len(identifyingProps) == 0 {
 		return nil, errors.New("identifying_properties is required")
 	}
 
 	// Validate total size to prevent resource exhaustion
-	// Using proto.Size provides a better bound than counting properties,
-	// as it accounts for arbitrarily large values in a Struct
+	// We sum the proto.Size of each value since map itself isn't a proto message
 	const maxProtoSize = 32 * 1024 // 32KB should be plenty for identifying properties
-	if protoSize := proto.Size(identifyingProps); protoSize > maxProtoSize {
+	var totalSize int
+	for _, v := range identifyingProps {
+		totalSize += proto.Size(v)
+	}
+	if totalSize > maxProtoSize {
 		return nil, fmt.Errorf("identifying_properties too large: %d bytes, max %d bytes",
-			protoSize, maxProtoSize)
+			totalSize, maxProtoSize)
 	}
 
-	propsMap := identifyingProps.AsMap()
-
-	// Validate property keys are reasonable length
-	for key := range propsMap {
+	// Convert map[string]*structpb.Value to map[string]any
+	propsMap := make(map[string]any, len(identifyingProps))
+	for key, value := range identifyingProps {
+		// Validate property keys are reasonable length
 		if len(key) > 200 {
 			return nil, fmt.Errorf("property key too long: %d characters", len(key))
 		}
+		if value != nil {
+			propsMap[key] = value.AsInterface()
+		}
 	}
 
 	return properties.NewProperties(propsMap), nil

internal/controlplane/handlers_entity_instances.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"database/sql"
 	"errors"
+	"strings"
 	"testing"
 
 	"github.com/google/uuid"
@@ -27,6 +28,16 @@ import (
 	"github.com/mindersec/minder/pkg/entities/properties"
 )
 
+// toIdentifyingProps converts a map[string]any to map[string]*structpb.Value for tests
+func toIdentifyingProps(m map[string]any) map[string]*structpb.Value {
+	result := make(map[string]*structpb.Value, len(m))
+	for k, v := range m {
+		val, _ := structpb.NewValue(v)
+		result[k] = val
+	}
+	return result
+}
+
 func TestServer_RegisterEntity(t *testing.T) {
 	t.Parallel()
 
@@ -50,13 +61,10 @@ func TestServer_RegisterEntity(t *testing.T) {
 			request: &pb.RegisterEntityRequest{
 				Context:    &pb.ContextV2{},
 				EntityType: pb.Entity_ENTITY_REPOSITORIES,
-				IdentifyingProperties: func() *structpb.Struct {
-					s, _ := structpb.NewStruct(map[string]any{
-						"github/repo_owner": "test-owner",
-						"github/repo_name":  "test-repo",
-					})
-					return s
-				}(),
+				IdentifyingProperties: toIdentifyingProps(map[string]any{
+					"github/repo_owner": "test-owner",
+					"github/repo_name":  "test-repo",
+				}),
 			},
 			setupContext: func(ctx context.Context) context.Context {
 				return engcontext.WithEntityContext(ctx, &engcontext.EntityContext{
@@ -106,12 +114,9 @@ func TestServer_RegisterEntity(t *testing.T) {
 		{
 			name: "fails when entity_type is unspecified",
 			request: &pb.RegisterEntityRequest{
-				Context:    &pb.ContextV2{},
-				EntityType: pb.Entity_ENTITY_UNSPECIFIED,
-				IdentifyingProperties: func() *structpb.Struct {
-					s, _ := structpb.NewStruct(map[string]any{"key": "value"})
-					return s
-				}(),
+				Context:               &pb.ContextV2{},
+				EntityType:            pb.Entity_ENTITY_UNSPECIFIED,
+				IdentifyingProperties: toIdentifyingProps(map[string]any{"key": "value"}),
 			},
 			setupContext: func(ctx context.Context) context.Context {
 				return engcontext.WithEntityContext(ctx, &engcontext.EntityContext{
@@ -145,12 +150,9 @@ func TestServer_RegisterEntity(t *testing.T) {
 		{
 			name: "fails when provider not found",
 			request: &pb.RegisterEntityRequest{
-				Context:    &pb.ContextV2{},
-				EntityType: pb.Entity_ENTITY_REPOSITORIES,
-				IdentifyingProperties: func() *structpb.Struct {
-					s, _ := structpb.NewStruct(map[string]any{"key": "value"})
-					return s
-				}(),
+				Context:               &pb.ContextV2{},
+				EntityType:            pb.Entity_ENTITY_REPOSITORIES,
+				IdentifyingProperties: toIdentifyingProps(map[string]any{"key": "value"}),
 			},
 			setupContext: func(ctx context.Context) context.Context {
 				return engcontext.WithEntityContext(ctx, &engcontext.EntityContext{
@@ -172,13 +174,10 @@ func TestServer_RegisterEntity(t *testing.T) {
 			request: &pb.RegisterEntityRequest{
 				Context:    &pb.ContextV2{},
 				EntityType: pb.Entity_ENTITY_REPOSITORIES,
-				IdentifyingProperties: func() *structpb.Struct {
-					s, _ := structpb.NewStruct(map[string]any{
-						"github/repo_owner": "test-owner",
-						"github/repo_name":  "archived-repo",
-					})
-					return s
-				}(),
+				IdentifyingProperties: toIdentifyingProps(map[string]any{
+					"github/repo_owner": "test-owner",
+					"github/repo_name":  "archived-repo",
+				}),
 			},
 			setupContext: func(ctx context.Context) context.Context {
 				return engcontext.WithEntityContext(ctx, &engcontext.EntityContext{
@@ -209,13 +208,10 @@ func TestServer_RegisterEntity(t *testing.T) {
 			request: &pb.RegisterEntityRequest{
 				Context:    &pb.ContextV2{},
 				EntityType: pb.Entity_ENTITY_REPOSITORIES,
-				IdentifyingProperties: func() *structpb.Struct {
-					s, _ := structpb.NewStruct(map[string]any{
-						"github/repo_owner": "test-owner",
-						"github/repo_name":  "private-repo",
-					})
-					return s
-				}(),
+				IdentifyingProperties: toIdentifyingProps(map[string]any{
+					"github/repo_owner": "test-owner",
+					"github/repo_name":  "private-repo",
+				}),
 			},
 			setupContext: func(ctx context.Context) context.Context {
 				return engcontext.WithEntityContext(ctx, &engcontext.EntityContext{
@@ -243,12 +239,9 @@ func TestServer_RegisterEntity(t *testing.T) {
 		{
 			name: "handles internal errors appropriately",
 			request: &pb.RegisterEntityRequest{
-				Context:    &pb.ContextV2{},
-				EntityType: pb.Entity_ENTITY_REPOSITORIES,
-				IdentifyingProperties: func() *structpb.Struct {
-					s, _ := structpb.NewStruct(map[string]any{"key": "value"})
-					return s
-				}(),
+				Context:               &pb.ContextV2{},
+				EntityType:            pb.Entity_ENTITY_REPOSITORIES,
+				IdentifyingProperties: toIdentifyingProps(map[string]any{"key": "value"}),
 			},
 			setupContext: func(ctx context.Context) context.Context {
 				return engcontext.WithEntityContext(ctx, &engcontext.EntityContext{
@@ -332,14 +325,11 @@ func TestParseIdentifyingProperties(t *testing.T) {
 		{
 			name: "parses valid properties",
 			request: &pb.RegisterEntityRequest{
-				IdentifyingProperties: func() *structpb.Struct {
-					s, _ := structpb.NewStruct(map[string]any{
-						"github/repo_owner": "stacklok",
-						"github/repo_name":  "minder",
-						"upstream_id":       "12345",
-					})
-					return s
-				}(),
+				IdentifyingProperties: toIdentifyingProps(map[string]any{
+					"github/repo_owner": "stacklok",
+					"github/repo_name":  "minder",
+					"upstream_id":       "12345",
+				}),
 			},
 			wantErr: false,
 			validate: func(t *testing.T, props *properties.Properties) {
@@ -365,16 +355,12 @@ func TestParseIdentifyingProperties(t *testing.T) {
 		{
 			name: "rejects properties that are too large",
 			request: &pb.RegisterEntityRequest{
-				IdentifyingProperties: func() *structpb.Struct {
+				IdentifyingProperties: func() map[string]*structpb.Value {
 					// Create a value large enough to exceed 32KB limit
-					largeValue := string(make([]byte, 33*1024))
-					for i := range largeValue {
-						largeValue = string(append([]byte(largeValue[:i]), 'x'))
-					}
-					s, _ := structpb.NewStruct(map[string]any{
+					largeValue := strings.Repeat("x", 33*1024)
+					return toIdentifyingProps(map[string]any{
 						"large_key": largeValue,
 					})
-					return s
 				}(),
 			},
 			wantErr:     true,
@@ -383,15 +369,11 @@ func TestParseIdentifyingProperties(t *testing.T) {
 		{
 			name: "rejects property key that is too long",
 			request: &pb.RegisterEntityRequest{
-				IdentifyingProperties: func() *structpb.Struct {
-					longKey := string(make([]byte, 201))
-					for i := range longKey {
-						longKey = string(append([]byte(longKey[:i]), 'a'))
-					}
-					s, _ := structpb.NewStruct(map[string]any{
+				IdentifyingProperties: func() map[string]*structpb.Value {
+					longKey := strings.Repeat("a", 201)
+					return toIdentifyingProps(map[string]any{
 						longKey: "value",
 					})
-					return s
 				}(),
 			},
 			wantErr:     true,
@@ -400,17 +382,10 @@ func TestParseIdentifyingProperties(t *testing.T) {
 		{
 			name: "handles empty properties map",
 			request: &pb.RegisterEntityRequest{
-				IdentifyingProperties: func() *structpb.Struct {
-					s, _ := structpb.NewStruct(map[string]any{})
-					return s
-				}(),
-			},
-			wantErr: false,
-			validate: func(t *testing.T, props *properties.Properties) {
-				t.Helper()
-				// Empty properties is valid (provider will validate)
-				assert.NotNil(t, props)
+				IdentifyingProperties: toIdentifyingProps(map[string]any{}),
 			},
+			wantErr:     true, // Empty map is now an error (changed behavior)
+			errContains: "identifying_properties is required",
 		},
 	}
 

internal/controlplane/handlers_entity_instances_test.go

@@ -56,18 +56,26 @@ func (a *addOriginatingEntityStrategy) GetEntity(
 	}
 
 	// Get provider from DB
+	// Note: These reads are outside the transaction boundary in EntityCreator.CreateEntity
+	// because they read stable data (parent entity and provider configuration).
+	// The transaction in CreateEntity protects the writes (entity + properties).
+	// If there's a race where parent is deleted between read/write, the FK constraint catches it.
 	provider, err := a.store.GetProviderByID(ctx, parentEwp.Entity.ProviderID)
 	if err != nil {
 		return nil, fmt.Errorf("error getting provider: %w", err)
 	}
 
 	// Use EntityCreator to create child entity
+	// Note: Child entities (artifacts, releases, PRs) don't trigger reconciliation events.
+	// This matches existing behavior and avoids potential loops since this code runs
+	// from a message handler. The parent repository's reconciliation handles the
+	// evaluation of child entities through the entity evaluation graph.
 	childEwp, err := a.entityCreator.CreateEntity(ctx, &provider,
 		parentEwp.Entity.ProjectID, entMsg.Entity.Type, childProps,
 		&entityService.EntityCreationOptions{
 			OriginatingEntityID:        &parentEwp.Entity.ID,
 			RegisterWithProvider:       false, // No webhooks for child entities
-			PublishReconciliationEvent: false, // No reconciliation for child entities
+			PublishReconciliationEvent: false, // Explained above
 		})
 	if err != nil {
 		return nil, fmt.Errorf("error creating entity: %w", err)

internal/entities/handlers/strategies/entity/add_originating_entity.go

@@ -16,6 +16,7 @@ import (
 	"github.com/mindersec/minder/internal/engine/entities"
 	"github.com/mindersec/minder/internal/entities/models"
 	propService "github.com/mindersec/minder/internal/entities/properties/service"
+	"github.com/mindersec/minder/internal/entities/service/validators"
 	"github.com/mindersec/minder/internal/providers/manager"
 	reconcilers "github.com/mindersec/minder/internal/reconcilers/messages"
 	pb "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"
@@ -52,23 +53,12 @@ type EntityCreator interface {
 	) (*models.EntityWithProperties, error)
 }
 
-// EntityValidator validates entity creation based on business rules
-type EntityValidator interface {
-	// Validate returns nil if entity is valid, error otherwise
-	Validate(
-		ctx context.Context,
-		entType pb.Entity,
-		props *properties.Properties,
-		projectID uuid.UUID,
-	) error
-}
-
 type entityCreator struct {
-	store           db.Store
-	propSvc         propService.PropertiesService
-	providerManager manager.ProviderManager
-	eventProducer   interfaces.Publisher
-	validators      []EntityValidator
+	store             db.Store
+	propSvc           propService.PropertiesService
+	providerManager   manager.ProviderManager
+	eventProducer     interfaces.Publisher
+	validatorRegistry validators.ValidatorRegistry
 }
 
 // NewEntityCreator creates a new EntityCreator
@@ -77,14 +67,14 @@ func NewEntityCreator(
 	propSvc propService.PropertiesService,
 	providerManager manager.ProviderManager,
 	eventProducer interfaces.Publisher,
-	validators []EntityValidator,
+	validatorRegistry validators.ValidatorRegistry,
 ) EntityCreator {
 	return &entityCreator{
-		store:           store,
-		propSvc:         propSvc,
-		providerManager: providerManager,
-		eventProducer:   eventProducer,
-		validators:      validators,
+		store:             store,
+		propSvc:           propSvc,
+		providerManager:   providerManager,
+		eventProducer:     eventProducer,
+		validatorRegistry: validatorRegistry,
 	}
 }
 
@@ -122,8 +112,8 @@ func (e *entityCreator) CreateEntity(
 		return nil, fmt.Errorf("error fetching properties: %w", err)
 	}
 
-	// 4. Run validators
-	if err := e.runValidators(ctx, entityType, allProps, projectID); err != nil {
+	// 4. Run validators via registry
+	if err := e.validatorRegistry.Validate(ctx, entityType, allProps, projectID); err != nil {
 		return nil, err
 	}
 
@@ -199,20 +189,6 @@ func (e *entityCreator) CreateEntity(
 	return ewp, nil
 }
 
-func (e *entityCreator) runValidators(
-	ctx context.Context,
-	entityType pb.Entity,
-	allProps *properties.Properties,
-	projectID uuid.UUID,
-) error {
-	for _, validator := range e.validators {
-		if err := validator.Validate(ctx, entityType, allProps, projectID); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 func (e *entityCreator) publishReconciliationEvent(
 	_ context.Context,
 	ewp *models.EntityWithProperties,