protobuf-net / protobuf-net.Grpc

GRPC bindings for protobuf-net and grpc-dotnet
Other
868 stars 114 forks source link

(analyzer) Difference between ServiceAttribute and ServiceContractAttribute #268

Open leotohill opened 1 year ago

leotohill commented 1 year ago

From Issue #66 I understood from that ServiceAttribute would be identical to ServiceContractAttribute. However, when using ServiceAttribute, a method that returns byte[] fails to compile, error "PBN2003 The return value of a gRPC method must currently be ..."

When using ServiceContractAttribute there is no error (and in fact it seems to run just fine!)

On v3.1.17

mgravell commented 1 year ago

Right; so that analyzer needs updating to consider both :)

The intent of the analyzer is to push people to use message types; as an accident of implementation, it turns out that the code does technically work for a few (not many) non-message types in the signature, i.e. byte[] and possibly string - but this was not the intended usage.

When I get all the available time, the plan is to allow more versatile signatures with codegen to fill in the spaces by generating types at build time to shim over the inbuilt gRPC limitations (outside of my control)

menaheme commented 1 year ago

Pardon the semi-hijack, does the aforementioned codegen cover allowing multi paraneter method signatures (and maybe primitive types , structs)?

On Wed, Jan 18, 2023, 3:48 PM Marc Gravell @.***> wrote:

Right; so that analyzer needs updating to consider both :)

The intent of the analyzer is to push people to use message types; as an accident of implementation, it turns out that the code does technically work for a few (not many) non-message types in the signature, i.e. byte[] and possibly string - but this was not the intended usage.

When I get all the available time, the plan is to allow more versatile signatures with codegen to fill in the spaces by generating types at build time to shim over the inbuilt gRPC limitations (outside of my control)

— Reply to this email directly, view it on GitHub https://github.com/protobuf-net/protobuf-net.Grpc/issues/268#issuecomment-1387102440, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLOIHZ6EDTDVFVGISBVNYTWS7YCTANCNFSM6AAAAAAT6N6D3U . You are receiving this because you are subscribed to this thread.Message ID: @.***>

mgravell commented 1 year ago

exactly that, yes; this is purely hypothetical (i.e. not coded), but IMO we need to make this an explicit feature to avoid a: problems with existing use of byte[] / string (see above), or b: problems when you start with 1 parameter and then add another (we don't want to smash the API shape), so I'm thinking:

[SomeNewAttribute]
int SomeMethod(DateTime x, SomeStruct y);

which would be shimmed via a simulated message type for input and output that just makes everything work (gRPC requires exactly one message type per message and has a T : class constraint - both need attention)

I know there's a pre-existing PR that does something similar, but my view is that it significantly complicates the codebase, and we should address it a different way; also, AOT

menaheme commented 1 year ago

I would like to help, but im not up to par with your codimg standard. Maybe with some guidance ?

On Wed, Jan 18, 2023, 4:28 PM Marc Gravell @.***> wrote:

exactly that, yes; this is purely hypothetical (i.e. not coded), but IMO we need to make this an explicit feature to avoid a: problems with existing use of byte[] / string (see above), or b: problems when you start with 1 parameter and then add another (we don't want to smash the API shape), so I'm thinking:

[SomeNewAttribute] int SomeMethod(DateTime x, SomeStruct y);

which would be shimmed via a simulated message type for input and output that just makes everything work (gRPC requires exactly one message type per message and has a T : class constraint - both need attention)

I know there's a pre-existing PR that does something similar, but my view is that it significantly complicates the codebase, and we should address it a different way; also, AOT

— Reply to this email directly, view it on GitHub https://github.com/protobuf-net/protobuf-net.Grpc/issues/268#issuecomment-1387163846, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLOIH3EEJFFEC32O4HJAH3WS742FANCNFSM6AAAAAAT6N6D3U . You are receiving this because you commented.Message ID: @.***>

leotohill commented 1 year ago

It's a happy accident that a method that returns string[] will accommodate null entries in the array, and a unhappy truth that when done property with message types, we can't have null entries. More code.

mgravell commented 1 year ago

A string[] will not normally support nulls. However, for both string and message types, the SupportsNull feature has been re-added to V3, so: nulls can indeed be transmitted when using an outer message (i.e. not just a naked array as a service return type) although it currently requires tweaks to the RuntimeTypeModel configuration in code. A change to do that via attributes is also in progress.