diff --git a/lib/grpc_reflection/service/builder/util.ex b/lib/grpc_reflection/service/builder/util.ex index 89f0740..7236571 100644 --- a/lib/grpc_reflection/service/builder/util.ex +++ b/lib/grpc_reflection/service/builder/util.ex @@ -163,6 +163,9 @@ defmodule GrpcReflection.Service.Builder.Util do [] end + # in gRPC, the leading "." signifies it is a FQDN + # we trim it and assume everything is a FQDN + # it works so far, but there may be corner cases def trim_symbol("." <> symbol), do: symbol def trim_symbol(symbol), do: symbol end diff --git a/lib/grpc_reflection/service/state.ex b/lib/grpc_reflection/service/state.ex index d87b33e..e27f16c 100644 --- a/lib/grpc_reflection/service/state.ex +++ b/lib/grpc_reflection/service/state.ex @@ -117,8 +117,72 @@ defmodule GrpcReflection.Service.State do end def group_symbols_by_namespace(%__MODULE__{} = state) do - # group symbols by namespace and combine - # IO.inspect(state) - state + state.symbols + |> Map.keys() + |> Enum.group_by(&GrpcReflection.Service.Builder.Util.get_package(&1)) + |> Enum.reduce(state, fn {package, symbols}, state_acc -> + # each symbol in symbols is in the same package + # we will combine their files into a single file, and update them to + # reference this new file + + # Step 1: Collect descriptors to be combined + symbol_files = Enum.map(symbols, &state_acc.symbols[&1]) + files_to_combine = state_acc.files |> Map.take(symbol_files) |> Map.values() + + # Step 2: Combine the descriptors + combined_file = + Enum.reduce( + files_to_combine, + %Google.Protobuf.FileDescriptorProto{ + package: package, + name: package <> ".proto" + }, + fn descriptor, acc -> + %{ + acc + | syntax: descriptor.syntax, + message_type: Enum.uniq(acc.message_type ++ descriptor.message_type), + service: Enum.uniq(acc.service ++ descriptor.service), + enum_type: Enum.uniq(acc.enum_type ++ descriptor.enum_type), + dependency: Enum.uniq(acc.dependency ++ descriptor.dependency), + extension: Enum.uniq(acc.extension ++ descriptor.extension) + } + end + ) + + # Step 3: remove internal dependency refs + cleaned_file = + %{combined_file | dependency: combined_file.dependency -- symbol_files} + + # Step 4: rework state around combined descriptor + # removing and re-adding symbols pointing to combined file + # removing combined file descriptors + # editing existing descriptors for relevant dependency entries + # add combined file descriptor + %{ + state_acc + | symbols: + state_acc.symbols + |> Map.drop(symbols) + |> Map.merge(Map.new(symbols, &{&1, cleaned_file.name})), + files: + state_acc.files + |> Map.drop(symbol_files) + |> Map.new(fn {filename, descriptor} -> + if Enum.any?(descriptor.dependency, &Enum.member?(symbol_files, &1)) do + { + filename, + %{ + descriptor + | dependency: (descriptor.dependency -- symbol_files) ++ [cleaned_file.name] + } + } + else + {filename, descriptor} + end + end) + |> Map.put(cleaned_file.name, cleaned_file) + } + end) end end diff --git a/test/integration/v1_reflection_test.exs b/test/integration/v1_reflection_test.exs index ea5d94f..df2dd48 100644 --- a/test/integration/v1_reflection_test.exs +++ b/test/integration/v1_reflection_test.exs @@ -74,7 +74,7 @@ defmodule GrpcReflection.V1ReflectionTest do test "describing a nested type returns the root type", ctx do message = {:file_containing_symbol, "testserviceV3.TestRequest.Payload"} assert {:ok, response} = run_request(message, ctx) - assert response.name == "testserviceV3.TestRequest.proto" + assert response.name == "testserviceV3.proto" end test "type with leading period still resolves", ctx do @@ -90,11 +90,8 @@ defmodule GrpcReflection.V1ReflectionTest do assert {:ok, response} = run_request(message, ctx) assert_response(response) - # we pretend all modules are in different files, dependencies are listed - assert response.dependency == [ - "helloworld.HelloRequest.proto", - "helloworld.HelloReply.proto" - ] + # we pretend each namespace is in a single file, dependencies are listed + assert response.dependency == ["google.protobuf.proto"] end test "reject filename that doesn't match a reflection module", ctx do @@ -104,39 +101,21 @@ defmodule GrpcReflection.V1ReflectionTest do end test "get replytype by filename", ctx do - filename = "helloworld.HelloReply.proto" + filename = "helloworld.proto" message = {:file_by_filename, filename} assert {:ok, response} = run_request(message, ctx) assert response.name == filename assert response.package == "helloworld" - assert response.dependency == ["google.protobuf.Timestamp.proto"] + assert response.dependency == ["google.protobuf.proto"] assert [ - %Google.Protobuf.DescriptorProto{ - name: "HelloReply", - field: [ - %Google.Protobuf.FieldDescriptorProto{ - name: "message", - number: 1, - label: :LABEL_OPTIONAL, - type: :TYPE_STRING, - json_name: "message" - }, - %Google.Protobuf.FieldDescriptorProto{ - name: "today", - number: 2, - label: :LABEL_OPTIONAL, - type: :TYPE_MESSAGE, - type_name: ".google.protobuf.Timestamp", - json_name: "today" - } - ] - } + %Google.Protobuf.DescriptorProto{name: "HelloReply"}, + %Google.Protobuf.DescriptorProto{name: "HelloRequest"} ] = response.message_type end test "get external by filename", ctx do - filename = "google.protobuf.Timestamp.proto" + filename = "google.protobuf.proto" message = {:file_by_filename, filename} assert {:ok, response} = run_request(message, ctx) assert response.name == filename @@ -144,53 +123,30 @@ defmodule GrpcReflection.V1ReflectionTest do assert response.dependency == [] assert [ - %Google.Protobuf.DescriptorProto{ - field: [ - %Google.Protobuf.FieldDescriptorProto{ - json_name: "seconds", - label: :LABEL_OPTIONAL, - name: "seconds", - number: 1, - type: :TYPE_INT64 - }, - %Google.Protobuf.FieldDescriptorProto{ - json_name: "nanos", - label: :LABEL_OPTIONAL, - name: "nanos", - number: 2, - type: :TYPE_INT32 - } - ], - name: "Timestamp" - } + %Google.Protobuf.DescriptorProto{name: "Any"}, + %Google.Protobuf.DescriptorProto{name: "StringValue"}, + %Google.Protobuf.DescriptorProto{name: "Timestamp"} ] = response.message_type end test "ensures file descriptor dependencies are unique", ctx do - filename = "testserviceV3.TestReply.proto" + filename = "testserviceV3.proto" message = {:file_by_filename, filename} assert {:ok, response} = run_request(message, ctx) assert response.name == filename assert response.package == "testserviceV3" - assert response.dependency == [ - "google.protobuf.Timestamp.proto", - "google.protobuf.StringValue.proto" - ] + assert response.dependency == ["google.protobuf.proto"] end test "ensure exclusion of nested types in file descriptor dependencies", ctx do - filename = "testserviceV3.TestRequest.proto" + filename = "testserviceV3.proto" message = {:file_by_filename, filename} assert {:ok, response} = run_request(message, ctx) assert response.name == filename assert response.package == "testserviceV3" - assert response.dependency == [ - "testserviceV3.Enum.proto", - "google.protobuf.Any.proto", - "google.protobuf.StringValue.proto" - ] + assert response.dependency == ["google.protobuf.proto"] end end @@ -213,49 +169,18 @@ defmodule GrpcReflection.V1ReflectionTest do assert {:ok, response} = run_request(message, ctx) assert response.name == extendee <> "Extension.proto" assert response.package == "testserviceV2" - assert response.dependency == [extendee <> ".proto"] - - assert response.extension == [ - %Google.Protobuf.FieldDescriptorProto{ - name: "data", - extendee: extendee, - number: 10, - label: :LABEL_OPTIONAL, - type: :TYPE_STRING, - type_name: nil - }, - %Google.Protobuf.FieldDescriptorProto{ - name: "location", - extendee: extendee, - number: 11, - label: :LABEL_OPTIONAL, - type: :TYPE_MESSAGE, - type_name: "testserviceV2.Location" - } - ] + assert response.dependency == ["testserviceV2.proto"] + + assert [ + %Google.Protobuf.FieldDescriptorProto{name: "data"}, + %Google.Protobuf.FieldDescriptorProto{name: "location"} + ] = response.extension - assert response.message_type == [ + assert [ %Google.Protobuf.DescriptorProto{ - name: "Location", - field: [ - %Google.Protobuf.FieldDescriptorProto{ - name: "latitude", - number: 1, - label: :LABEL_OPTIONAL, - type: :TYPE_DOUBLE, - json_name: "latitude" - }, - %Google.Protobuf.FieldDescriptorProto{ - name: "longitude", - extendee: nil, - number: 2, - label: :LABEL_OPTIONAL, - type: :TYPE_DOUBLE, - json_name: "longitude" - } - ] + name: "Location" } - ] + ] = response.message_type end end end diff --git a/test/integration/v1alpha_reflection_test.exs b/test/integration/v1alpha_reflection_test.exs index 0bd5092..761e1e2 100644 --- a/test/integration/v1alpha_reflection_test.exs +++ b/test/integration/v1alpha_reflection_test.exs @@ -74,7 +74,7 @@ defmodule GrpcReflection.V1alphaReflectionTest do test "describing a nested type returns the root type", ctx do message = {:file_containing_symbol, "testserviceV3.TestRequest.Payload"} assert {:ok, response} = run_request(message, ctx) - assert response.name == "testserviceV3.TestRequest.proto" + assert response.name == "testserviceV3.proto" end test "type with leading period still resolves", ctx do @@ -91,10 +91,7 @@ defmodule GrpcReflection.V1alphaReflectionTest do assert_response(response) # we pretend all modules are in different files, dependencies are listed - assert response.dependency == [ - "helloworld.HelloRequest.proto", - "helloworld.HelloReply.proto" - ] + assert response.dependency == ["google.protobuf.proto"] end test "reject filename that doesn't match a reflection module", ctx do @@ -104,39 +101,32 @@ defmodule GrpcReflection.V1alphaReflectionTest do end test "get replytype by filename", ctx do - filename = "helloworld.HelloReply.proto" + filename = "helloworld.proto" message = {:file_by_filename, filename} assert {:ok, response} = run_request(message, ctx) assert response.name == filename assert response.package == "helloworld" - assert response.dependency == ["google.protobuf.Timestamp.proto"] + assert response.dependency == ["google.protobuf.proto"] assert [ %Google.Protobuf.DescriptorProto{ name: "HelloReply", field: [ - %Google.Protobuf.FieldDescriptorProto{ - name: "message", - number: 1, - label: :LABEL_OPTIONAL, - type: :TYPE_STRING, - json_name: "message" - }, - %Google.Protobuf.FieldDescriptorProto{ - name: "today", - number: 2, - label: :LABEL_OPTIONAL, - type: :TYPE_MESSAGE, - type_name: ".google.protobuf.Timestamp", - json_name: "today" - } + %Google.Protobuf.FieldDescriptorProto{name: "message"}, + %Google.Protobuf.FieldDescriptorProto{name: "today"} + ] + }, + %Google.Protobuf.DescriptorProto{ + name: "HelloRequest", + field: [ + %Google.Protobuf.FieldDescriptorProto{name: "name"} ] } ] = response.message_type end test "get external by filename", ctx do - filename = "google.protobuf.Timestamp.proto" + filename = "google.protobuf.proto" message = {:file_by_filename, filename} assert {:ok, response} = run_request(message, ctx) assert response.name == filename @@ -144,53 +134,30 @@ defmodule GrpcReflection.V1alphaReflectionTest do assert response.dependency == [] assert [ - %Google.Protobuf.DescriptorProto{ - field: [ - %Google.Protobuf.FieldDescriptorProto{ - json_name: "seconds", - label: :LABEL_OPTIONAL, - name: "seconds", - number: 1, - type: :TYPE_INT64 - }, - %Google.Protobuf.FieldDescriptorProto{ - json_name: "nanos", - label: :LABEL_OPTIONAL, - name: "nanos", - number: 2, - type: :TYPE_INT32 - } - ], - name: "Timestamp" - } + %Google.Protobuf.DescriptorProto{name: "Any"}, + %Google.Protobuf.DescriptorProto{name: "StringValue"}, + %Google.Protobuf.DescriptorProto{name: "Timestamp"} ] = response.message_type end test "ensures file descriptor dependencies are unique", ctx do - filename = "testserviceV3.TestReply.proto" + filename = "testserviceV3.proto" message = {:file_by_filename, filename} assert {:ok, response} = run_request(message, ctx) assert response.name == filename assert response.package == "testserviceV3" - assert response.dependency == [ - "google.protobuf.Timestamp.proto", - "google.protobuf.StringValue.proto" - ] + assert response.dependency == ["google.protobuf.proto"] end test "ensure exclusion of nested types in file descriptor dependencies", ctx do - filename = "testserviceV3.TestRequest.proto" + filename = "testserviceV3.proto" message = {:file_by_filename, filename} assert {:ok, response} = run_request(message, ctx) assert response.name == filename assert response.package == "testserviceV3" - assert response.dependency == [ - "testserviceV3.Enum.proto", - "google.protobuf.Any.proto", - "google.protobuf.StringValue.proto" - ] + assert response.dependency == ["google.protobuf.proto"] end end @@ -216,7 +183,7 @@ defmodule GrpcReflection.V1alphaReflectionTest do assert {:ok, response} = run_request(message, ctx) assert response.name == extendee <> "Extension.proto" assert response.package == "testserviceV2" - assert response.dependency == [extendee <> ".proto"] + assert response.dependency == ["testserviceV2.proto"] assert response.extension == [ %Google.Protobuf.FieldDescriptorProto{ diff --git a/test/service/builder_test.exs b/test/service/builder_test.exs index 40e8129..af400a7 100644 --- a/test/service/builder_test.exs +++ b/test/service/builder_test.exs @@ -11,15 +11,8 @@ defmodule GrpcReflection.Service.BuilderTest do assert %State{services: [TestserviceV3.TestService.Service]} = tree assert Map.keys(tree.files) == [ - "google.protobuf.Any.proto", - "google.protobuf.StringValue.proto", - "google.protobuf.Timestamp.proto", - "testserviceV3.Enum.proto", - "testserviceV3.TestReply.proto", - "testserviceV3.TestRequest.Payload.proto", - "testserviceV3.TestRequest.Token.proto", - "testserviceV3.TestRequest.proto", - "testserviceV3.TestService.proto" + "google.protobuf.proto", + "testserviceV3.proto" ] assert Map.keys(tree.symbols) == [ @@ -47,13 +40,9 @@ defmodule GrpcReflection.Service.BuilderTest do assert %State{services: [TestserviceV2.TestService.Service]} = tree assert Map.keys(tree.files) == [ - "google.protobuf.Any.proto", - "google.protobuf.Timestamp.proto", - "testserviceV2.Enum.proto", - "testserviceV2.TestReply.proto", - "testserviceV2.TestRequest.proto", + "google.protobuf.proto", "testserviceV2.TestRequestExtension.proto", - "testserviceV2.TestService.proto" + "testserviceV2.proto" ] assert Map.keys(tree.symbols) == [ @@ -109,13 +98,9 @@ defmodule GrpcReflection.Service.BuilderTest do names = Enum.map(Map.values(tree.files), & &1.name) assert names == [ - "google.protobuf.Any.proto", - "google.protobuf.Timestamp.proto", - "testserviceV2.Enum.proto", - "testserviceV2.TestReply.proto", - "testserviceV2.TestRequest.proto", + "google.protobuf.proto", "testserviceV2.TestRequestExtension.proto", - "testserviceV2.TestService.proto" + "testserviceV2.proto" ] end diff --git a/test/service/state_test.exs b/test/service/state_test.exs index f5ec0d1..0336f32 100644 --- a/test/service/state_test.exs +++ b/test/service/state_test.exs @@ -55,4 +55,67 @@ defmodule GrpcReflection.Service.StateTest do fn -> State.merge(state1, state2) end end end + + describe "group_symbols_by_namespace" do + setup do + state_with_recursion = %State{ + services: ["Service1", "Service2"], + files: %{ + "file1.proto" => %Google.Protobuf.FileDescriptorProto{ + name: "file1.proto", + package: ".common.path", + dependency: ["file2.proto"], + service: ["A"], + message_type: [%Google.Protobuf.DescriptorProto{name: "Symbol_a"}], + syntax: "proto2" + }, + "file2.proto" => %Google.Protobuf.FileDescriptorProto{ + name: "file2.proto", + package: ".common.path", + dependency: ["file1.proto"], + service: ["B"], + message_type: [%Google.Protobuf.DescriptorProto{name: "Symbol_b"}], + syntax: "proto2" + }, + "file3.proto" => %Google.Protobuf.FileDescriptorProto{ + name: "file3.proto", + package: ".other.path", + dependency: ["file1.proto", "file2.proto"], + service: ["C"], + message_type: [%Google.Protobuf.DescriptorProto{name: "Symbol_c"}], + syntax: "proto2" + } + }, + symbols: %{ + "common.path.Symbol_a" => "file1.proto", + "common.path.Symbol_b" => "file2.proto" + } + } + + %{ + state: State.group_symbols_by_namespace(state_with_recursion) + } + end + + test "should maintain all symbols", %{state: state} do + assert Map.keys(state.symbols) == ["common.path.Symbol_a", "common.path.Symbol_b"] + end + + test "should reduce and update files", %{state: state} do + assert [combined_file, other_file] = Map.values(state.files) + # combined file is present as we expect + assert combined_file.dependency == [] + assert combined_file.name == "common.path.proto" + # referencing file is updated as we expect + assert other_file.dependency == ["common.path.proto"] + assert other_file.name == "file3.proto" + end + + test "should combine descriptors", %{state: state} do + file = state.files["common.path.proto"] + symbols = Enum.map(file.message_type, & &1.name) + assert symbols == ["Symbol_a", "Symbol_b"] + assert file.service == ["A", "B"] + end + end end