Refine Logic for Handling Nested Types

[zhihuizhang17]

When handling nested types, there's no need to generate new symbol-kvs or file-kvs in the state for these nested types, as all their information is already stored in the root parent symbol's descriptor. Therefore:

• For a nested type's key in state["symbols"] and state["files"], set the value to the parent's descriptor instead.
• Nested type dependencies are removed from the parent type.

By the way, this PR fixes issue #34.

[zhihuizhang17]

doesn't that mean that we cannot describe .namespace.ParentType.NestedType?

Yes, I think there's no need to do so for all nested types. For a describe request like {:file_containing_symbol, ".namespace.ParentType.NestedType"}, returning the descriptor file for ".namespace.ParentType" is sufficient. This solution aligns with Go's gRPC reflection and it works with grpcurl. If you have concerns about this in Elixir, I can test it further with one of my online services later.

[mjheilmann]

doesn't that mean that we cannot describe .namespace.ParentType.NestedType?

Yes, I think there's no need to do so for all nested types. For a describe request like {:file_containing_symbol, ".namespace.ParentType.NestedType"}, returning the descriptor file for ".namespace.ParentType" is sufficient. This solution aligns with Go's gRPC reflection and it works with grpcurl. If you have concerns about this in Elixir, I can test it further with one of my online services later.

I agree that returning the payload from the parent type for the nested type is appropriate. I mean that if we call describe on the nested symbol we will be unable to do so because we are not inserting the nested symbols into the State structure for later lookup in your changes. You have code to try to do a dynamic lookup when searching the symbol like we do in the build stage, but I think keeping that only in the build stage is better.

[zhihuizhang17]

doesn't that mean that we cannot describe .namespace.ParentType.NestedType?

Yes, I think there's no need to do so for all nested types. For a describe request like {:file_containing_symbol, ".namespace.ParentType.NestedType"}, returning the descriptor file for ".namespace.ParentType" is sufficient. This solution aligns with Go's gRPC reflection and it works with grpcurl. If you have concerns about this in Elixir, I can test it further with one of my online services later.

I agree that returning the payload from the parent type for the nested type is appropriate. I mean that if we call describe on the nested symbol we will be unable to do so because we are not inserting the nested symbols into the State structure for later lookup in your changes. You have code to try to do a dynamic lookup when searching the symbol like we do in the build stage, but I think keeping that only in the build stage is better.

I understand your point. Let me rephrase my explanation:

It would be redundant to set a new %Google.Protobuf.DescriptorProto key-value pair for nested types in the generated Elixir code because the %Google.Protobuf.DescriptorProto struct for nested types is already contained within the parent's %Google.Protobuf.DescriptorProto struct.

Additionally, IMO, the protoc-gen-elixir plugin's approach of generating a descriptor/0 function for all types, including nested types, is unnecessary and results in larger file sizes. This is indeed different from the approach taken in Go, where a single fileDescriptor0 variable is generated for all types defined in the same .proto file.

I think there is an area for potential improvement in the protoc-gen-elixir plugin. It would be more efficient to generate the descriptor/0 function only for top-level message types and enums, and rely on the nested message and enum definitions within the parent's %Google.Protobuf.DescriptorProto struct. This would not only reduce the generated code size but also align more closely with the approach taken by other language implementations of Protocol Buffers.

[mjheilmann]

@zhihuizhang17 Lets pause and rethink what is going on here

I think your core idea is correct, which means that I think there are 2 things and only 2 things that need to happen:

1. We don't list nested types as dependencies to be resolved later
2. We still want to be able to reflect nested types, we just return the parent payload instead of generating a payload for the nested type

Those 2 things, plus some tests, are all that is needed to correct the nested type problem.

It would be redundant to set a new %Google.Protobuf.DescriptorProto key-value pair for nested types in the generated Elixir code because the %Google.Protobuf.DescriptorProto struct for nested types is already contained within the parent's %Google.Protobuf.DescriptorProto struct.

You've said this and things like this repeatedly in this PR. You are missing the point of the State lookup object. State already contains redundant information to streamline reflection calls. State is comprised of several 1-1 lookup maps that align with the grpc reflection methods: get_by_symbol and get_by_filename. Reference again here. There are a few other structures that assisted in building, such as references that could be removed and only present in a temporary structure used only in build but that is a different issue.

The state struct has the following traits that should be maintained:

1. Map.keys(state.symbols) lists every symbol that is in the given reflection tree
2. Map.keys(state.files) lists every pseudo proto file that we allow to be referenced in this reflection tree
3. The state object that is output from build is complete and static and fully resolves any reflection request -- or rejects as the case may be.

Optimizing denormalized payloads from State is out of the scope of correcting the nested types resolution problem. I am open to this as a separate action, and have put some thought to this myself, but it needs to be done carefully as an independent PR.

Now as far as the dynamic lookup you added to State.lookup_symbol, this is problematic as it is a potential attack vector. We perform the dynamic lookup in build because we have to due to the dynamic constraints of how the modules are associated and the load_extensions grpc problem. Code.ensure_loaded can and will load and compile additional modules from disk, and this can have side effects on the runtime. We are forced to do it during build, we should not do it anywhere else.

[zhihuizhang17]

@zhihuizhang17 Lets pause and rethink what is going on here

I think your core idea is correct, which means that I think there are 2 things and only 2 things that need to happen:

1. We don't list nested types as dependencies to be resolved later
2. We still want to be able to reflect nested types, we just return the parent payload instead of generating a payload for the nested type

Those 2 things, plus some tests, are all that is needed to correct the nested type problem.

It would be redundant to set a new %Google.Protobuf.DescriptorProto key-value pair for nested types in the generated Elixir code because the %Google.Protobuf.DescriptorProto struct for nested types is already contained within the parent's %Google.Protobuf.DescriptorProto struct.

You've said this and things like this repeatedly in this PR. You are missing the point of the State lookup object. State already contains redundant information to streamline reflection calls. State is comprised of several 1-1 lookup maps that align with the grpc reflection methods: get_by_symbol and get_by_filename. Reference again here. There are a few other structures that assisted in building, such as references that could be removed and only present in a temporary structure used only in build but that is a different issue.

The state struct has the following traits that should be maintained:

1. Map.keys(state.symbols) lists every symbol that is in the given reflection tree
2. Map.keys(state.files) lists every pseudo proto file that we allow to be referenced in this reflection tree
3. The state object that is output from build is complete and static and fully resolves any reflection request -- or rejects as the case may be.

Optimizing denormalized payloads from State is out of the scope of correcting the nested types resolution problem. I am open to this as a separate action, and have put some thought to this myself, but it needs to be done carefully as an independent PR.

Now as far as the dynamic lookup you added to State.lookup_symbol, this is problematic as it is a potential attack vector. We perform the dynamic lookup in build because we have to due to the dynamic constraints of how the modules are associated and the load_extensions grpc problem. Code.ensure_loaded can and will load and compile additional modules from disk, and this can have side effects on the runtime. We are forced to do it during build, we should not do it anywhere else.

Thanks for the detailed explanation!!!

We are forced to do it during build, we should not do it anywhere else.

You convinced me. I will implement it within build scope by adding a new nested map to the state struct, like this: %GrpcReflection.Service.State{parent_symbols: %{k: v}}. An alternative ways is that, for all nested types' key, the value is set to the parent's descriptor. What do you think about these two solutions?

You are missing the point of the State lookup object. State already contains redundant information to streamline reflection calls. State is comprised of several 1-1 lookup maps that align with the grpc reflection methods: get_by_symbol and get_by_filename

I think I understand your point. The only usage of state["symbols"] I found is for the request {:file_containing_symbol, "<symbol_name>"} for now. So, do you still want to store nested types' key-value pairs in state["symbols"] even after I add a new map to store parent symbols' map? I'm open to that.

Optimizing denormalized payloads from State is out of the scope of correcting the nested types resolution problem.

The main goal of this PR is to handle nested types more efficiently:

1. Avoid storing key-value pairs for nested types to optimize space.
2. Return the parent type's payload when requesting nested types.

[mjheilmann]

@zhihuizhang17 I do not understand why you need to modify the state object to store the nested types, nested symbols are not treated differently from normal symbols during reflection calls, we just need to handle them during the build stage. After they are discovered during build and the correct payload is identified for them, they are indistinguishable from regular symbols as far as State and reflection calls are concerned. Am I missing something here?

Avoid storing key-value pairs for nested types to optimize space.

For several reasons do not worry about this, just stick to the scope of resolving nested types correctly.

1. memory footprint optimization is outside of the scope of the issue, and if handled should be done in a different PR
2. memory footprint has not been identified as an issue, so optimization may be premature and introduce unnecessary complexity
3. This is a simple agent holding a map in memory. If you store the same payload struct under multiple keys, elixir will already reuse the same underlying immutable structure. We would need to generate a new payload or make a copy of the one we have to increase memory usage. Just reusing the parent payload instead of generating new ones for nested types (current behavior) is already going to be a memory savings.

[zhihuizhang17]

I do not understand why you need to modify the state object to store the nested types

Is that a typo or I misunderstand? My PR is actually aimed at avoiding the storage of nested types instead of storing the nested types.

Just reusing the parent payload instead of generating new ones for nested types (current behavior) is already going to be a memory savings.

The current behavior generates a new descriptor (instead of reusing the parent type's descriptor) for nested types. This behavior caused the package name issue in your online service (it generated a wrong package name for all the nested types). I have proposed two solutions in above comments.

1. Don't store key-value pairs for nested types and return the parent's descriptor when a nested type's descriptor is requested.
2. For all nested types' keys, set the value to the parent's descriptor in state["symbols"]

What do you think about these two solutions?

[mjheilmann]

Your proposal of option 2 is closest to what I've been asking for.

[zhihuizhang17]

Your proposal of option 2 is closest to what I've been asking for.

Okay, I will address it in this PR this way.

[mjheilmann]

Thanks for the work @zhihuizhang17