diff --git a/core/controller/getchangedtargets.go b/core/controller/getchangedtargets.go index d0de1ee3..146cc91d 100644 --- a/core/controller/getchangedtargets.go +++ b/core/controller/getchangedtargets.go @@ -245,20 +245,37 @@ func (c *controller) compareTargetGraphs(ctx context.Context, firstGraph, second } } - if sourceFileRuleTypeID != -1 { - // Iterate over the changed targets and check if any of them are source files. - // If so, check if any of their dependencies are changed source files. - // If so, mark the target as a direct change. - for name, ct := range changedByName { - if ct.GetChangeType() == pb.CHANGE_TYPE_DIRECT { - // Already marked as direct, skip - continue - } - newT := secondByName[name] - // Ifdependency of the target is a changed source file - if hasDepInChangedSourceFileTargets(newT.GetDirectDependencies(), secondMetadata, changedSourceFileTargets) { - ct.ChangeType = pb.CHANGE_TYPE_DIRECT - } + // Iterate over the changed targets and check if any of them are DIRECT changes. + for name, ct := range changedByName { + if ct.GetChangeType() == pb.CHANGE_TYPE_DIRECT || ct.GetChangeType() == pb.CHANGE_TYPE_NEW { + // Already marked as direct or new + continue + } + newT := secondByName[name] + oldT := firstByName[name] + + // Check if any dependency is a changed source file + if hasDepInChangedSourceFileTargets(newT.GetDirectDependencies(), secondMetadata, changedSourceFileTargets) { + ct.ChangeType = pb.CHANGE_TYPE_DIRECT + continue + } + + // Check if direct dependencies changed + depsChanged, err := dependenciesChanged(oldT, firstMetadata, newT, secondMetadata) + if err != nil { + return nil, fmt.Errorf("failed to check dependencies changed: %w", err) + } + if depsChanged { + ct.ChangeType = pb.CHANGE_TYPE_DIRECT + continue + } + // Check if attributes changed + attrsChanged, err := attributesChanged(oldT, firstMetadata, newT, secondMetadata) + if err != nil { + return nil, fmt.Errorf("failed to check attributes changed: %w", err) + } + if attrsChanged { + ct.ChangeType = pb.CHANGE_TYPE_DIRECT } } @@ -366,6 +383,118 @@ func hasDepInChangedSourceFileTargets(depIds []int32, meta *pb.Metadata, changed return false } +// dependenciesChanged checks if the set of direct dependencies changed between old and new targets. +func dependenciesChanged(oldTarget *pb.OptimizedTarget, oldMeta *pb.Metadata, newTarget *pb.OptimizedTarget, newMeta *pb.Metadata) (bool, error) { + if oldMeta == nil || newMeta == nil { + return false, nil + } + + oldDepIDs := oldTarget.GetDirectDependencies() + newDepIDs := newTarget.GetDirectDependencies() + + // Early exit: if lengths differ, dependencies changed + if len(oldDepIDs) != len(newDepIDs) { + return true, nil + } + // Early exit: if both are empty, no change + if len(oldDepIDs) == 0 { + return false, nil + } + + // validate target names are equivalent. + if err := validateTargetNames(oldTarget, newTarget, oldMeta, newMeta); err != nil { + return false, fmt.Errorf("target names are different") + } + + // Cache metadata mappings to avoid repeated map lookups + oldTargetIDMapping := oldMeta.GetTargetIdMapping() + newTargetIDMapping := newMeta.GetTargetIdMapping() + // Build set of new dependency names (only one set needed) + newDepSet := make(map[string]struct{}, len(newDepIDs)) + for _, depID := range newDepIDs { + if name := newTargetIDMapping[depID]; name != "" { + newDepSet[name] = struct{}{} + } + } + + // Check if all old deps exist in new deps + for _, depID := range oldDepIDs { + if name := oldTargetIDMapping[depID]; name != "" { + if _, exists := newDepSet[name]; !exists { + return true, nil + } + } + } + + return false, nil +} + +// attributesChanged checks if the attributes changed between old and new targets. +func attributesChanged(oldTarget *pb.OptimizedTarget, oldMeta *pb.Metadata, newTarget *pb.OptimizedTarget, newMeta *pb.Metadata) (bool, error) { + if oldMeta == nil || newMeta == nil { + return false, nil + } + // validate target names are equivalent. + if err := validateTargetNames(oldTarget, newTarget, oldMeta, newMeta); err != nil { + return false, err + } + + oldAttrIDs := oldTarget.GetAttributes() + newAttrIDs := newTarget.GetAttributes() + + // Early exit: if lengths differ, attributes changed + if len(oldAttrIDs) != len(newAttrIDs) { + return true, nil + } + + // Early exit: if both are empty, no change + if len(oldAttrIDs) == 0 { + return false, nil + } + + // Cache metadata mappings to avoid repeated map lookups + oldAttrNameMapping := oldMeta.GetAttributeNameMapping() + oldAttrValMapping := oldMeta.GetAttributeStringValueMapping() + newAttrNameMapping := newMeta.GetAttributeNameMapping() + newAttrValMapping := newMeta.GetAttributeStringValueMapping() + + // Build map of new attributes (only one map needed) + newAttrMap := make(map[string]string, len(newAttrIDs)) + for attrNameID, attrValID := range newAttrIDs { + if attrName := newAttrNameMapping[attrNameID]; attrName != "" { + newAttrMap[attrName] = newAttrValMapping[attrValID] + } + } + + // Check if all old attributes match + for attrNameID, attrValID := range oldAttrIDs { + if attrName := oldAttrNameMapping[attrNameID]; attrName != "" { + oldVal := oldAttrValMapping[attrValID] + newVal, exists := newAttrMap[attrName] + if !exists || newVal != oldVal { + return true, nil + } + } + } + return false, nil +} + +// validateTargetNames checks if the target names are the same between old and new targets, and exists in both metadata maps. +func validateTargetNames(oldTarget, newTarget *pb.OptimizedTarget, oldMeta, newMeta *pb.Metadata) error { + oldTargetName, ok := oldMeta.GetTargetIdMapping()[oldTarget.GetId()] + if !ok { + return fmt.Errorf("old target id %d not found in metadata", oldTarget.GetId()) + } + newTargetName, ok := newMeta.GetTargetIdMapping()[newTarget.GetId()] + if !ok { + return fmt.Errorf("new target id %d not found in metadata", newTarget.GetId()) + } + if oldTargetName != newTargetName { + return fmt.Errorf("target names are different %s != %s", oldTargetName, newTargetName) + } + return nil +} + // transposeOptimizedTarget remaps a target into the canonical ID space using name-based mappers. func transposeOptimizedTarget( src *pb.OptimizedTarget, diff --git a/core/controller/getchangedtargets_test.go b/core/controller/getchangedtargets_test.go index a78af5ed..2d7a658e 100644 --- a/core/controller/getchangedtargets_test.go +++ b/core/controller/getchangedtargets_test.go @@ -398,3 +398,320 @@ func TestCompareTargetGraphs_IndirectWhenNoSourceDep(t *testing.T) { require.Len(t, cs.GetChangedTargets(), 1) require.Equal(t, pb.CHANGE_TYPE_INDIRECT, cs.GetChangedTargets()[0].GetChangeType()) } + +func TestCompareTargetGraphs_DirectWhenDependenciesChanged(t *testing.T) { + c := &controller{logger: zaptest.NewLogger(t)} + + // Old: T (id 1, rule) with deps on A + first := []*pb.GetTargetGraphResponse{ + { + Item: &pb.GetTargetGraphResponse_Targets{ + Targets: &pb.OptimizedTargets{ + Targets: []*pb.OptimizedTarget{ + {Id: 1, Hash: "h1", RuleType: 200, DirectDependencies: []int32{10}}, + {Id: 10, Hash: "h1", RuleType: 200}, // Dependency A + }, + }, + }, + }, + { + Item: &pb.GetTargetGraphResponse_Metadata{ + Metadata: &pb.Metadata{ + TargetIdMapping: map[int32]string{ + 1: "//app:T", + 10: "//app:A", + }, + RuleTypeMapping: map[int32]string{ + 100: "source file", + 200: "rule", + }, + }, + }, + }, + } + // New: T now depends on B instead of A (hash changed due to dep change) + second := []*pb.GetTargetGraphResponse{ + { + Item: &pb.GetTargetGraphResponse_Targets{ + Targets: &pb.OptimizedTargets{ + Targets: []*pb.OptimizedTarget{ + {Id: 2, Hash: "h2", RuleType: 201, DirectDependencies: []int32{20}}, + {Id: 20, Hash: "h1", RuleType: 201}, // Dependency B + }, + }, + }, + }, + { + Item: &pb.GetTargetGraphResponse_Metadata{ + Metadata: &pb.Metadata{ + TargetIdMapping: map[int32]string{ + 2: "//app:T", + 20: "//app:B", + }, + RuleTypeMapping: map[int32]string{ + 101: "source file", + 201: "rule", + }, + }, + }, + }, + } + res, err := c.compareTargetGraphs(context.Background(), first, second, nil) + require.NoError(t, err) + cs := res[0].GetChangedTargets() + require.NotNil(t, cs) + + // Find target T in the changed targets + var targetT *pb.ChangedTarget + for _, ct := range cs.GetChangedTargets() { + name := res[1].GetMetadata().GetTargetIdMapping()[ct.GetNewTarget().GetId()] + if name == "//app:T" { + targetT = ct + break + } + } + require.NotNil(t, targetT) + require.Equal(t, pb.CHANGE_TYPE_DIRECT, targetT.GetChangeType(), "Target with changed dependencies should be marked as DIRECT") +} + +func TestCompareTargetGraphs_DirectWhenAttributesChanged(t *testing.T) { + c := &controller{logger: zaptest.NewLogger(t)} + + // Old: T with attribute "key1" -> "value1" + first := []*pb.GetTargetGraphResponse{ + { + Item: &pb.GetTargetGraphResponse_Targets{ + Targets: &pb.OptimizedTargets{ + Targets: []*pb.OptimizedTarget{ + { + Id: 1, + Hash: "h1", + RuleType: 200, + Attributes: map[int32]int32{1: 10}, // attr name 1 -> attr value 10 + }, + }, + }, + }, + }, + { + Item: &pb.GetTargetGraphResponse_Metadata{ + Metadata: &pb.Metadata{ + TargetIdMapping: map[int32]string{1: "//app:T"}, + RuleTypeMapping: map[int32]string{ + 100: "source file", + 200: "rule", + }, + AttributeNameMapping: map[int32]string{1: "key1"}, + AttributeStringValueMapping: map[int32]string{10: "value1"}, + }, + }, + }, + } + // New: T with attribute "key1" -> "value2" (changed value) + second := []*pb.GetTargetGraphResponse{ + { + Item: &pb.GetTargetGraphResponse_Targets{ + Targets: &pb.OptimizedTargets{ + Targets: []*pb.OptimizedTarget{ + { + Id: 2, + Hash: "h2", + RuleType: 201, + Attributes: map[int32]int32{2: 20}, // attr name 2 -> attr value 20 + }, + }, + }, + }, + }, + { + Item: &pb.GetTargetGraphResponse_Metadata{ + Metadata: &pb.Metadata{ + TargetIdMapping: map[int32]string{2: "//app:T"}, + RuleTypeMapping: map[int32]string{ + 101: "source file", + 201: "rule", + }, + AttributeNameMapping: map[int32]string{2: "key1"}, + AttributeStringValueMapping: map[int32]string{20: "value2"}, + }, + }, + }, + } + res, err := c.compareTargetGraphs(context.Background(), first, second, nil) + require.NoError(t, err) + cs := res[0].GetChangedTargets() + require.NotNil(t, cs) + require.Len(t, cs.GetChangedTargets(), 1) + require.Equal(t, pb.CHANGE_TYPE_DIRECT, cs.GetChangedTargets()[0].GetChangeType(), "Target with changed attributes should be marked as DIRECT") +} + +func TestCompareTargetGraphs_DirectWhenNewAttributeAdded(t *testing.T) { + c := &controller{logger: zaptest.NewLogger(t)} + + // Old: T with one attribute + first := []*pb.GetTargetGraphResponse{ + { + Item: &pb.GetTargetGraphResponse_Targets{ + Targets: &pb.OptimizedTargets{ + Targets: []*pb.OptimizedTarget{ + { + Id: 1, + Hash: "h1", + RuleType: 200, + Attributes: map[int32]int32{1: 10}, + }, + }, + }, + }, + }, + { + Item: &pb.GetTargetGraphResponse_Metadata{ + Metadata: &pb.Metadata{ + TargetIdMapping: map[int32]string{1: "//app:T"}, + RuleTypeMapping: map[int32]string{ + 100: "source file", + 200: "rule", + }, + AttributeNameMapping: map[int32]string{1: "key1"}, + AttributeStringValueMapping: map[int32]string{10: "value1"}, + }, + }, + }, + } + // New: T with two attributes (added key2) + second := []*pb.GetTargetGraphResponse{ + { + Item: &pb.GetTargetGraphResponse_Targets{ + Targets: &pb.OptimizedTargets{ + Targets: []*pb.OptimizedTarget{ + { + Id: 2, + Hash: "h2", + RuleType: 201, + Attributes: map[int32]int32{ + 2: 20, // key1 -> value1 + 3: 30, // key2 -> value2 (NEW) + }, + }, + }, + }, + }, + }, + { + Item: &pb.GetTargetGraphResponse_Metadata{ + Metadata: &pb.Metadata{ + TargetIdMapping: map[int32]string{2: "//app:T"}, + RuleTypeMapping: map[int32]string{ + 101: "source file", + 201: "rule", + }, + AttributeNameMapping: map[int32]string{ + 2: "key1", + 3: "key2", + }, + AttributeStringValueMapping: map[int32]string{ + 20: "value1", + 30: "value2", + }, + }, + }, + }, + } + res, err := c.compareTargetGraphs(context.Background(), first, second, nil) + require.NoError(t, err) + cs := res[0].GetChangedTargets() + require.NotNil(t, cs) + require.Len(t, cs.GetChangedTargets(), 1) + require.Equal(t, pb.CHANGE_TYPE_DIRECT, cs.GetChangedTargets()[0].GetChangeType(), "Target with new attribute added should be marked as DIRECT") +} + +func TestCompareTargetGraphs_IndirectWhenOnlyHashChanged(t *testing.T) { + c := &controller{logger: zaptest.NewLogger(t)} + + // Old: T with deps and attributes + first := []*pb.GetTargetGraphResponse{ + { + Item: &pb.GetTargetGraphResponse_Targets{ + Targets: &pb.OptimizedTargets{ + Targets: []*pb.OptimizedTarget{ + { + Id: 1, + Hash: "h1", + RuleType: 200, + DirectDependencies: []int32{10}, + Attributes: map[int32]int32{1: 10}, + }, + {Id: 10, Hash: "h1", RuleType: 200}, + }, + }, + }, + }, + { + Item: &pb.GetTargetGraphResponse_Metadata{ + Metadata: &pb.Metadata{ + TargetIdMapping: map[int32]string{ + 1: "//app:T", + 10: "//app:A", + }, + RuleTypeMapping: map[int32]string{ + 100: "source file", + 200: "rule", + }, + AttributeNameMapping: map[int32]string{1: "key1"}, + AttributeStringValueMapping: map[int32]string{10: "value1"}, + }, + }, + }, + } + // New: T with same deps and attributes, but hash changed (e.g., due to transitive dep change) + second := []*pb.GetTargetGraphResponse{ + { + Item: &pb.GetTargetGraphResponse_Targets{ + Targets: &pb.OptimizedTargets{ + Targets: []*pb.OptimizedTarget{ + { + Id: 2, + Hash: "h2", // Changed + RuleType: 201, + DirectDependencies: []int32{20}, // Same dep (//app:A) + Attributes: map[int32]int32{2: 20}, // Same attribute + }, + {Id: 20, Hash: "h2", RuleType: 201}, // Dep A hash changed + }, + }, + }, + }, + { + Item: &pb.GetTargetGraphResponse_Metadata{ + Metadata: &pb.Metadata{ + TargetIdMapping: map[int32]string{ + 2: "//app:T", + 20: "//app:A", + }, + RuleTypeMapping: map[int32]string{ + 101: "source file", + 201: "rule", + }, + AttributeNameMapping: map[int32]string{2: "key1"}, + AttributeStringValueMapping: map[int32]string{20: "value1"}, + }, + }, + }, + } + res, err := c.compareTargetGraphs(context.Background(), first, second, nil) + require.NoError(t, err) + cs := res[0].GetChangedTargets() + require.NotNil(t, cs) + + // Find target T + var targetT *pb.ChangedTarget + for _, ct := range cs.GetChangedTargets() { + name := res[1].GetMetadata().GetTargetIdMapping()[ct.GetNewTarget().GetId()] + if name == "//app:T" { + targetT = ct + break + } + } + require.NotNil(t, targetT) + require.Equal(t, pb.CHANGE_TYPE_INDIRECT, targetT.GetChangeType(), "Target with only hash change (not deps/attrs) should be marked as INDIRECT") +}