diff --git a/core/changedetector/proto_change_detector_service.go b/core/changedetector/proto_change_detector_service.go index a8cf0e27..2107b1b2 100644 --- a/core/changedetector/proto_change_detector_service.go +++ b/core/changedetector/proto_change_detector_service.go @@ -80,27 +80,59 @@ func appendImpactedDependents(sce *stencilv1beta1.SchemaChangedEvent, key string } func setDirectlyImpactedSchemasAndFields(currentFds, prevFds *descriptor.FileDescriptorSet, sce *stencilv1beta1.SchemaChangedEvent) { - packageMessageMap := getPackageMessageMap(prevFds) + prevMessageMap := collectAllMessages(prevFds) packageEnumMap := getPackageEnumMap(prevFds) for _, fileDesc := range currentFds.GetFile() { + pkg := fileDesc.GetPackage() for _, newMessageDesc := range fileDesc.GetMessageType() { - messageName := fileDesc.GetPackage() + "." + newMessageDesc.GetName() - oldMessageDesc := getMessageDescriptor(packageMessageMap, fileDesc.GetPackage(), newMessageDesc.GetName()) - if oldMessageDesc == nil { - sce.UpdatedSchemas = append(sce.UpdatedSchemas, messageName) - appendImpactedFields(sce, messageName, GetImpactedMessageFields(oldMessageDesc, newMessageDesc)) - continue - } - if !proto.Equal(oldMessageDesc, newMessageDesc) { - sce.UpdatedSchemas = append(sce.UpdatedSchemas, messageName) - appendImpactedFields(sce, messageName, GetImpactedMessageFields(oldMessageDesc, newMessageDesc)) - } - compareEnumDescInMessageDesc(oldMessageDesc, newMessageDesc, messageName, sce) + compareMessageTree(pkg, newMessageDesc, prevMessageMap, sce) } compareEnumDescriptors(fileDesc, packageEnumMap, sce) } } +// collectAllMessages returns a flat map of fully-qualified name -> descriptor +// for ALL messages (top-level and nested) across all files in the descriptor set. +func collectAllMessages(fds *descriptor.FileDescriptorSet) map[string]*descriptor.DescriptorProto { + result := make(map[string]*descriptor.DescriptorProto) + for _, fileDesc := range fds.GetFile() { + pkg := fileDesc.GetPackage() + for _, msgDesc := range fileDesc.GetMessageType() { + walkMessageTree(pkg, msgDesc, result) + } + } + return result +} + +// walkMessageTree recursively registers a message and all its nested messages +// into the result map under their fully-qualified names (e.g. "test.Outer.Inner"). +func walkMessageTree(prefix string, msg *descriptor.DescriptorProto, result map[string]*descriptor.DescriptorProto) { + fullName := prefix + "." + msg.GetName() + result[fullName] = msg + for _, nested := range msg.GetNestedType() { + walkMessageTree(fullName, nested, result) + } +} + +// compareMessageTree recursively compares a message and all its nested messages, +// recording any changes into the SchemaChangedEvent. +func compareMessageTree(prefix string, newMsg *descriptor.DescriptorProto, prevMessageMap map[string]*descriptor.DescriptorProto, sce *stencilv1beta1.SchemaChangedEvent) { + fullName := prefix + "." + newMsg.GetName() + oldMsg := prevMessageMap[fullName] + if oldMsg == nil { + sce.UpdatedSchemas = append(sce.UpdatedSchemas, fullName) + appendImpactedFields(sce, fullName, GetImpactedMessageFields(oldMsg, newMsg)) + } else if !proto.Equal(oldMsg, newMsg) { + sce.UpdatedSchemas = append(sce.UpdatedSchemas, fullName) + appendImpactedFields(sce, fullName, GetImpactedMessageFields(oldMsg, newMsg)) + compareEnumDescInMessageDesc(oldMsg, newMsg, fullName, sce) + } + // Always recurse into nested messages so each level is independently evaluated. + for _, nested := range newMsg.GetNestedType() { + compareMessageTree(fullName, nested, prevMessageMap, sce) + } +} + func compareEnumDescInMessageDesc(oldMessageDesc, newMessageDesc *descriptorpb.DescriptorProto, messageName string, sce *stencilv1beta1.SchemaChangedEvent) { for _, newEnumDesc := range newMessageDesc.GetEnumType() { enumName := messageName + "." + newEnumDesc.GetName() @@ -147,24 +179,6 @@ func compareEnumDescriptors(fds *descriptorpb.FileDescriptorProto, packageEnumMa } } -/* -packageMessageMap is map having all the messages inside a package -[com.goto.bookinglog][BookingLogMessage]=BookingLogMessageDescriptor -*/ -func getPackageMessageMap(fileDescriptorSet *descriptor.FileDescriptorSet) map[string]map[string]*descriptor.DescriptorProto { - packageMessageMap := make(map[string]map[string]*descriptor.DescriptorProto) - for _, fileDescriptor := range fileDescriptorSet.GetFile() { - pkgName := fileDescriptor.GetPackage() - if _, ok := packageMessageMap[pkgName]; !ok { - packageMessageMap[pkgName] = make(map[string]*descriptor.DescriptorProto) - } - for _, messageDescriptor := range fileDescriptor.GetMessageType() { - packageMessageMap[pkgName][messageDescriptor.GetName()] = messageDescriptor - } - } - return packageMessageMap -} - /* packageEnumMap is map having all the enums inside a package [com.goto.bookinglog][ServiceTypeEnum]=ServiceTypeEnumDescriptor @@ -183,15 +197,6 @@ func getPackageEnumMap(fileDescriptorSet *descriptor.FileDescriptorSet) map[stri return packageEnumMap } -func getMessageDescriptor(packageMessageMap map[string]map[string]*descriptor.DescriptorProto, packageName, messageName string) *descriptor.DescriptorProto { - if packageMap, found := packageMessageMap[packageName]; found { - if descriptor, found := packageMap[messageName]; found { - return descriptor - } - } - return nil -} - func getEnumDescriptor(packageEnumMap map[string]map[string]*descriptor.EnumDescriptorProto, packageName, enumName string) *descriptor.EnumDescriptorProto { if packageMap, found := packageEnumMap[packageName]; found { if descriptor, found := packageMap[enumName]; found { @@ -213,23 +218,33 @@ func findEnumDescriptorFromMessageDescriptor(messageDescriptor *descriptor.Descr func getReverseDependencies(fileDescriptorSet *descriptor.FileDescriptorSet) map[string][]string { reverseDependencies := make(map[string][]string) for _, fileDescriptor := range fileDescriptorSet.GetFile() { + pkg := fileDescriptor.GetPackage() for _, messageDescriptor := range fileDescriptor.GetMessageType() { - messageName := fileDescriptor.GetPackage() + "." + messageDescriptor.GetName() - for _, fieldDescriptor := range messageDescriptor.GetField() { - fieldType := fieldDescriptor.GetTypeName() - /*Check if the field type is a message (nested message or imported message) - Ref:https://cloud.google.com/java/docs/reference/protobuf/latest/com.google.protobuf.DescriptorProtos.FieldDescriptorProto#com_google_protobuf_DescriptorProtos_FieldDescriptorProto_getType__:~:text=The%20type.-,getTypeName(),-public%20String%20getTypeName - */ - if fieldType != "" && fieldType[0] == '.' { - dependentMessage := fieldType[1:] - reverseDependencies[dependentMessage] = append(reverseDependencies[dependentMessage], messageName) - } - } + buildReverseDepsFromMessage(pkg, messageDescriptor, reverseDependencies) } } return reverseDependencies } +// buildReverseDepsFromMessage recursively registers reverse dependencies for a message +// and all its nested messages, so that e.g. "test.Outer.Inner" is a valid lookup key. +func buildReverseDepsFromMessage(prefix string, msg *descriptor.DescriptorProto, reverseDeps map[string][]string) { + msgFullName := prefix + "." + msg.GetName() + for _, fieldDescriptor := range msg.GetField() { + fieldType := fieldDescriptor.GetTypeName() + /*Check if the field type is a message (nested message or imported message) + Ref:https://cloud.google.com/java/docs/reference/protobuf/latest/com.google.protobuf.DescriptorProtos.FieldDescriptorProto#com_google_protobuf_DescriptorProtos_FieldDescriptorProto_getType__:~:text=The%20type.-,getTypeName(),-public%20String%20getTypeName + */ + if fieldType != "" && fieldType[0] == '.' { + dependentMessage := fieldType[1:] + reverseDeps[dependentMessage] = append(reverseDeps[dependentMessage], msgFullName) + } + } + for _, nested := range msg.GetNestedType() { + buildReverseDepsFromMessage(msgFullName, nested, reverseDeps) + } +} + func getDependentImpactedSchemas(reverseDependencies map[string][]string, impactedSchema string, depth int32) []string { visitedMessages := make(map[string]bool) var dependentImpactedSchemas []string diff --git a/core/changedetector/proto_change_detector_service_test.go b/core/changedetector/proto_change_detector_service_test.go index 4cbbe3b7..6c6e8df0 100644 --- a/core/changedetector/proto_change_detector_service_test.go +++ b/core/changedetector/proto_change_detector_service_test.go @@ -250,6 +250,53 @@ func TestIdentifySchemaChange(t *testing.T) { }) } +func TestIdentifySchemaChangeNestedProto(t *testing.T) { + ctx := context.Background() + + t.Run("should detect field change inside nested message and report parent as impacted", func(t *testing.T) { + svc, newRelic := getSvc() + request.Depth = 10 + var called bool + // v1: Outer.Inner has only `value`. v2: Outer.Inner gains `extra_field`. + // Expected: both test.Outer and test.Outer.Inner appear in updated_schemas. + // test.Outer.Inner.updated_fields should contain "extra_field". + // test.Outer.Inner.impacted_schemas should contain test.Outer (it uses Inner). + oldData := getDescriptorData(t, "./testdata/input", true, []string{"schema_with_nested_message_v1.proto"}) + newData := getDescriptorData(t, "./testdata/input", true, []string{"schema_with_nested_message_v2.proto"}) + request.OldData = oldData + request.NewData = newData + newRelic.On("StartGenericSegment", mock.Anything, "Identify Schema Change").Return(func() { called = true }) + actual, err := svc.IdentifySchemaChange(ctx, request) + expected := getSchemaChangeEvent("./testdata/output/sce_nested_message_changed.json") + assert.Nil(t, err) + newRelic.AssertExpectations(t) + assert.True(t, called) + assert.NotNil(t, actual) + assertSchemaChangeEvent(t, expected, actual) + }) + + t.Run("should detect AnotherMessage as impacted when it references Outer.Inner which changed", func(t *testing.T) { + svc, newRelic := getSvc() + request.Depth = -1 + var called bool + // v1: Outer.Inner has only `value`. AnotherMessage uses Outer.Inner directly. + // v2: Outer.Inner gains `extra_field`. + // Expected: test.AnotherMessage appears in impacted_schemas["test.Outer.Inner"]. + oldData := getDescriptorData(t, "./testdata/input", true, []string{"schema_with_nested_cross_ref_v1.proto"}) + newData := getDescriptorData(t, "./testdata/input", true, []string{"schema_with_nested_cross_ref_v2.proto"}) + request.OldData = oldData + request.NewData = newData + newRelic.On("StartGenericSegment", mock.Anything, "Identify Schema Change").Return(func() { called = true }) + actual, err := svc.IdentifySchemaChange(ctx, request) + expected := getSchemaChangeEvent("./testdata/output/sce_nested_cross_ref_changed.json") + assert.Nil(t, err) + newRelic.AssertExpectations(t) + assert.True(t, called) + assert.NotNil(t, actual) + assertSchemaChangeEvent(t, expected, actual) + }) +} + func assertSchemaChangeEvent(t *testing.T, expected, actual *stencilv1beta1.SchemaChangedEvent) { t.Helper() assert.Equal(t, expected.NamespaceName, actual.NamespaceName) diff --git a/core/changedetector/testdata/input/schema_with_nested_cross_ref_v1.proto b/core/changedetector/testdata/input/schema_with_nested_cross_ref_v1.proto new file mode 100644 index 00000000..38f5e2d4 --- /dev/null +++ b/core/changedetector/testdata/input/schema_with_nested_cross_ref_v1.proto @@ -0,0 +1,20 @@ +syntax = "proto3"; + +package test; + +// Inner is defined INSIDE Outer +message Outer { + string id = 1; + + message Inner { + string value = 2; + } + + Inner inner = 3; +} + +// AnotherMessage uses Outer.Inner directly — cross-referencing a nested message +message AnotherMessage { + Outer.Inner inner_ref = 1; +} + diff --git a/core/changedetector/testdata/input/schema_with_nested_cross_ref_v2.proto b/core/changedetector/testdata/input/schema_with_nested_cross_ref_v2.proto new file mode 100644 index 00000000..3e20d6ee --- /dev/null +++ b/core/changedetector/testdata/input/schema_with_nested_cross_ref_v2.proto @@ -0,0 +1,21 @@ +syntax = "proto3"; + +package test; + +// Inner gains a new field `extra_field` +message Outer { + string id = 1; + + message Inner { + string value = 2; + string extra_field = 3; + } + + Inner inner = 3; +} + +// AnotherMessage still uses Outer.Inner — should be impacted by Inner's change +message AnotherMessage { + Outer.Inner inner_ref = 1; +} + diff --git a/core/changedetector/testdata/input/schema_with_nested_message_v1.proto b/core/changedetector/testdata/input/schema_with_nested_message_v1.proto new file mode 100644 index 00000000..acc07d25 --- /dev/null +++ b/core/changedetector/testdata/input/schema_with_nested_message_v1.proto @@ -0,0 +1,14 @@ +syntax = "proto3"; + +package test; + +message Outer { + string id = 1; + + message Inner { + string value = 2; + } + + Inner inner = 3; +} + diff --git a/core/changedetector/testdata/input/schema_with_nested_message_v2.proto b/core/changedetector/testdata/input/schema_with_nested_message_v2.proto new file mode 100644 index 00000000..ab5934b4 --- /dev/null +++ b/core/changedetector/testdata/input/schema_with_nested_message_v2.proto @@ -0,0 +1,15 @@ +syntax = "proto3"; + +package test; + +message Outer { + string id = 1; + + message Inner { + string value = 2; + string extra_field = 3; // new field added to nested message + } + + Inner inner = 3; +} + diff --git a/core/changedetector/testdata/output/sce_nested_cross_ref_changed.json b/core/changedetector/testdata/output/sce_nested_cross_ref_changed.json new file mode 100644 index 00000000..11234b96 --- /dev/null +++ b/core/changedetector/testdata/output/sce_nested_cross_ref_changed.json @@ -0,0 +1,36 @@ +{ + "namespace_name": "testNamespace", + "schema_name": "testSchemaName", + "updated_schemas": [ + "test.Outer", + "test.Outer.Inner" + ], + "updated_fields": { + "test.Outer": {}, + "test.Outer.Inner": { + "field_names": [ + "extra_field" + ] + } + }, + "impacted_schemas": { + "test.Outer": { + "schema_names": [ + "test.Outer" + ] + }, + "test.Outer.Inner": { + "schema_names": [ + "test.Outer.Inner", + "test.Outer", + "test.AnotherMessage" + ] + } + }, + "version": 1, + "metadata": { + "source_url": "https://github.com/some-repo", + "commit_sha": "some-commit-sha" + } +} + diff --git a/core/changedetector/testdata/output/sce_nested_message_changed.json b/core/changedetector/testdata/output/sce_nested_message_changed.json new file mode 100644 index 00000000..719a5603 --- /dev/null +++ b/core/changedetector/testdata/output/sce_nested_message_changed.json @@ -0,0 +1,35 @@ +{ + "namespace_name": "testNamespace", + "schema_name": "testSchemaName", + "updated_schemas": [ + "test.Outer", + "test.Outer.Inner" + ], + "updated_fields": { + "test.Outer": {}, + "test.Outer.Inner": { + "field_names": [ + "extra_field" + ] + } + }, + "impacted_schemas": { + "test.Outer": { + "schema_names": [ + "test.Outer" + ] + }, + "test.Outer.Inner": { + "schema_names": [ + "test.Outer.Inner", + "test.Outer" + ] + } + }, + "version": 1, + "metadata": { + "source_url": "https://github.com/some-repo", + "commit_sha": "some-commit-sha" + } +} + diff --git a/ui/embed.go b/ui/embed.go index 540dc893..5065dec7 100644 --- a/ui/embed.go +++ b/ui/embed.go @@ -4,5 +4,5 @@ import "embed" // Assets embed build folder // -//go:embed build/* +//go:embed all:build var Assets embed.FS