[csharp] Helper to get MessageDescriptor from System.Type

[alrz]

See https://github.com/protocolbuffers/protobuf/issues/6960#issuecomment-2011651059 for context.

Currently, implementing the mapping from System.Type is not straightforward as it has to use reflection to check the interface and pull the descriptor. To avoid the interface check the type could be annotated somehow to indicate that we're dealing with a proto message type. But a built-in helper is most preferable so that we don't have to also consider caching on the client code.

[jskeet]

Personally I tend to think an interface check is simpler than fetching a custom attribute, unless more information is needed - but I can see how having an attribute for this could potentially be useful. It'll still require reflection to access the descriptor instance, mind you... but yes, we could have caching for that.

[alrz]

All that is really implementation detail if this was provided built-in in the most efficient manner possible, plus caching. Given a helper like that you can also get the proto name so it's a few birds with a single shot. (unless you think the proto name annotation could be useful on its own, perhaps for documentation purposes and whatnot)

[alrz]

Turns out json transcoding had to implement something like this internally https://github.com/dotnet/aspnetcore/blob/63c8031b6a6af5009b3c5bb4291fcc4c32b06b10/src/Grpc/JsonTranscoding/src/Shared/DescriptorRegistry.cs

[jskeet]

Well, sort of - that never has to work from "just a type" with no other background information - it requires the registry to be populated first. (Whereas I believe you're proposing doing stuff with reflection so that you don't need to pre-register.)

[alrz]

If I'm not mistaken pre-register is required if you want to use Any anyways so it wouldn't feel out of place, especially if it helps with perf. In that case, I imagine this helper could be added to the existing TypeRegistry?

[jskeet]

Right, if you're happy with pre-registering and you're using a TypeRegistry, then adding functionality to TypeRegistry seems reasonable. The context in https://github.com/protocolbuffers/protobuf/issues/6960#issuecomment-2011651059 seemed like a much larger scope. (Admittedly there are other reasons you might want to get a descriptor for a type which have nothing to do with Any, and where TypeRegistry might not be involved at all...)

[alrz]

I initially made that comment as a means to make it easier to implement such helper, if this is going to be provided built-it that discussion is moot, I believe. Note I'm just spitballing here, the request is to have some way to provide this mapping, I am not proposing a specific API shape to achieve that. Perhaps the scenario in gRPC transcoding could be taken into account for that.

[jskeet]

Note I'm just spitballing here, the request is to have some way to provide this mapping

But requests for "a mapping if you've already initialized it in a type registry" and "the mapping must be available without any prior initialization" are very different requests. I really don't want to end up implementing something and then find out it doesn't meet the real requirements.

[alrz]

I think at this point there's at least two options,

// can either cache on-demand or when adding files
// need pre-register
public sealed class TypeRegistry
{
  public DescriptorBase Find(Type type);

  public MessageDescriptor FindMessage(Type type);
  public EnumDescriptor FindEnum(Type type);
  // etc
}

// will cache on-demand
// doesn't need pre-register
public sealed class TypeRegistry
{
  public static DescriptorBase Find(Type type);

  public static MessageDescriptor FindMessage(Type type);
  public static EnumDescriptor FindEnum(Type type);
  // etc
}

Personally, I can work with both (and all I really care about is the FindMessage method), however, I believe DescriptorRegistry from gRPC transcoding as an example "in the wild" should at least be able to use it to ensure this is versatile enough IMO.

[jskeet]

Okay. I'll look again at this when I get time, but just to set expectations, it's unlikely to be soon.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.