diff --git a/lib/grpc_reflection/service/builder.ex b/lib/grpc_reflection/service/builder.ex index 86e4189..69bcee2 100644 --- a/lib/grpc_reflection/service/builder.ex +++ b/lib/grpc_reflection/service/builder.ex @@ -14,7 +14,7 @@ defmodule GrpcReflection.Service.Builder do new_state = process_service(service) State.merge(state, new_state) end) - |> State.group_symbols_by_namespace() + |> State.shrink_cycles() {:ok, tree} end 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/cycle.ex b/lib/grpc_reflection/service/cycle.ex new file mode 100644 index 0000000..a8a56ff --- /dev/null +++ b/lib/grpc_reflection/service/cycle.ex @@ -0,0 +1,65 @@ +defmodule GrpcReflection.Service.Cycle do + @moduledoc """ + Find and identify cycles in a state graph + """ + + defmodule DFS do + @moduledoc false + defstruct visited: [], path: [], cycles: [] + end + + def get_cycles(%GrpcReflection.Service.State{files: files}) do + files + |> Map.values() + |> Enum.reject(fn file -> + String.ends_with?(file.name, "Extension.proto") + end) + |> Map.new(fn file -> {file.name, file.dependency} end) + |> find_cycles() + end + + def find_cycles(graph) do + graph + |> Map.keys() + |> Enum.reduce(%DFS{}, fn node, acc -> + %{ + dfs(node, graph, acc) + | path: acc.path + } + end) + |> Map.fetch!(:cycles) + |> Enum.map(&Enum.sort/1) + |> Enum.sort() + |> Enum.uniq() + end + + defp dfs(node, graph, state) do + cond do + Enum.member?(state.path, node) -> + # Node is in current path → cycle found + cycle = [node | Enum.take_while(state.path, &(&1 != node))] + %{state | cycles: [cycle | state.cycles]} + + Enum.member?(state.visited, node) -> + state + + true -> + # Mark as visited and extend path + state = %{ + state + | visited: [node | state.visited], + path: [node | state.path] + } + + # Visit neighbors + graph + |> Map.get(node, []) + |> Enum.reduce(state, fn neighbor, acc -> + %{ + dfs(neighbor, graph, acc) + | path: acc.path + } + end) + end + end +end diff --git a/lib/grpc_reflection/service/state.ex b/lib/grpc_reflection/service/state.ex index d87b33e..16e4a76 100644 --- a/lib/grpc_reflection/service/state.ex +++ b/lib/grpc_reflection/service/state.ex @@ -116,9 +116,66 @@ defmodule GrpcReflection.Service.State do end end - def group_symbols_by_namespace(%__MODULE__{} = state) do - # group symbols by namespace and combine - # IO.inspect(state) + # reduce state size and complexity by combining files into fewer, larger responses + def shrink_cycles(%__MODULE__{} = state) do state + |> GrpcReflection.Service.Cycle.get_cycles() + |> Enum.reduce(state, fn + filenames, state_acc -> + files = Enum.map(filenames, &state.files[&1]) + combined_file = combine_file_descriptors(files) + update_state_with_combined_file(state_acc, combined_file, filenames) + end) + end + + defp update_state_with_combined_file(state, combined_file, combined_filenames) do + # Update files: remove old entries, add new one with updated dependencies + new_files = + state.files + |> Map.drop(combined_filenames) + |> Map.new(fn {filename, descriptor} -> + if Enum.any?(descriptor.dependency, &Enum.member?(combined_filenames, &1)) do + { + filename, + %{ + descriptor + | dependency: (descriptor.dependency -- combined_filenames) ++ [combined_file.name] + } + } + else + {filename, descriptor} + end + end) + |> Map.put(combined_file.name, combined_file) + + # Update symbols: map old symbols to point to the new combined file + new_symbols = + Map.new(state.symbols, fn {symbol, filename} -> + if filename in combined_filenames do + {symbol, combined_file.name} + else + {symbol, filename} + end + end) + + %{state | files: new_files, symbols: new_symbols} + end + + defp combine_file_descriptors(file_descriptors) do + combined_names = Enum.map(file_descriptors, & &1.name) + + Enum.reduce(file_descriptors, %Google.Protobuf.FileDescriptorProto{}, fn descriptor, acc -> + %{ + acc + | syntax: acc.syntax || descriptor.syntax, + package: acc.package || descriptor.package, + name: acc.name || descriptor.name, + 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 -- combined_names)), + extension: Enum.uniq(acc.extension ++ descriptor.extension) + } + end) end end diff --git a/test/integration/v1_reflection_test.exs b/test/integration/v1_reflection_test.exs index ea5d94f..ce2b914 100644 --- a/test/integration/v1_reflection_test.exs +++ b/test/integration/v1_reflection_test.exs @@ -112,26 +112,7 @@ defmodule GrpcReflection.V1ReflectionTest do assert response.dependency == ["google.protobuf.Timestamp.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"} ] = response.message_type end @@ -144,25 +125,7 @@ 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: "Timestamp"} ] = response.message_type end @@ -213,49 +176,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.TestRequest.proto"] - assert response.message_type == [ + assert [ + %Google.Protobuf.FieldDescriptorProto{name: "data"}, + %Google.Protobuf.FieldDescriptorProto{name: "location"} + ] = response.extension + + 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..3623000 100644 --- a/test/integration/v1alpha_reflection_test.exs +++ b/test/integration/v1alpha_reflection_test.exs @@ -115,21 +115,8 @@ defmodule GrpcReflection.V1alphaReflectionTest do %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"} ] } ] = response.message_type @@ -144,25 +131,7 @@ 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: "Timestamp"} ] = response.message_type end @@ -216,7 +185,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.TestRequest.proto"] assert response.extension == [ %Google.Protobuf.FieldDescriptorProto{ diff --git a/test/service/cycle_test.exs b/test/service/cycle_test.exs new file mode 100644 index 0000000..a0b8d2a --- /dev/null +++ b/test/service/cycle_test.exs @@ -0,0 +1,118 @@ +defmodule GrpcReflection.Service.CycleTest do + @moduledoc false + + use ExUnit.Case + + alias GrpcReflection.Service.Cycle + + describe "get_cycles" do + test "should return empty list for empty graph" do + assert Cycle.find_cycles(%{}) == [] + end + + test "should return empty list when no cycles exist" do + graph = %{ + "A" => ["B"], + "B" => ["C"], + "C" => ["D"] + } + + assert Cycle.find_cycles(graph) == [] + end + + test "should detect a cycle in a simple cycle A → B → A" do + graph = %{ + "A" => ["B"], + "B" => ["A"] + } + + assert Cycle.find_cycles(graph) == [ + ["A", "B"] + ] + end + + test "should detect two distinct cycles" do + graph = %{ + "A" => ["B"], + "B" => ["A"], + "C" => ["D"], + "D" => ["C"] + } + + assert Cycle.find_cycles(graph) == [ + ["A", "B"], + ["C", "D"] + ] + end + + test "should detect one cycle in a complex graph" do + graph = %{ + "A" => ["B", "C"], + "B" => ["C"], + "C" => ["D"], + "D" => ["A"] + } + + assert Cycle.find_cycles(graph) == [ + ["A", "B", "C", "D"] + ] + end + + test "should detect cycle with indirect path" do + graph = %{ + "A" => ["B"], + "B" => ["C"], + "C" => ["D"], + "D" => ["A"] + } + + assert Cycle.find_cycles(graph) == [ + ["A", "B", "C", "D"] + ] + end + + test "should return empty list for a single node with no edges" do + graph = %{"A" => []} + assert Cycle.find_cycles(graph) == [] + end + + test "should ignore nodes with no outgoing edges" do + graph = %{ + "A" => [], + "B" => ["C"], + "C" => ["B"] + } + + assert Cycle.find_cycles(graph) == [ + ["B", "C"] + ] + end + + test "should not duplicate one cycle when referenced multiple times" do + graph = %{ + "A" => ["C"], + "B" => ["C"], + "C" => ["D"], + "D" => ["C"] + } + + assert Cycle.find_cycles(graph) == [ + ["C", "D"] + ] + end + + test "should handle nodes with many relations" do + graph = %{ + "A" => ["B", "C", "D", "E"], + "B" => [], + "C" => [], + "D" => [], + "E" => ["A"] + } + + assert Cycle.find_cycles(graph) == [ + ["A", "E"] + ] + end + end +end diff --git a/test/service/state_test.exs b/test/service/state_test.exs index f5ec0d1..b36e102 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 "shrink_cycles" 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.shrink_cycles(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 == "file1.proto" + # referencing file is updated as we expect + assert other_file.dependency == ["file1.proto"] + assert other_file.name == "file3.proto" + end + + test "should combine descriptors", %{state: state} do + file = state.files["file1.proto"] + symbols = Enum.map(file.message_type, & &1.name) + assert symbols == ["Symbol_a", "Symbol_b"] + assert file.service == ["A", "B"] + end + end end