protobuf-net / protobuf-net.Grpc

GRPC bindings for protobuf-net and grpc-dotnet
Other
857 stars 109 forks source link

Validate on service startup/in a test #113

Open ptsoccer opened 4 years ago

ptsoccer commented 4 years ago

I'm writing a Grpc service and am running into a couple of frequently occurring issues that are easy to fix, but only crop up when the associated Grpc service call is made.

  1. 70

  2. Copy pasting proto members and forgetting to update the tag

It would be nice to validate that all the services are working either in a test or at startup (maybe even prevent startup in such a case?). Is such a thing possible in general? And if so does its functionality currently exist?

mgravell commented 4 years ago
  1. Yes, #70 is absolutely on my radar; I just haven't rushed it for multiple reasons; now that the server reflection piece (.grpc.reflection.v1alpha.ServerReflection) is stabilizing, it is pretty high on my list of things to finish up.

  2. Can you clarify what this means? Perhaps with an example? I want to understand the exact problem.

As for validating services; perhaps we could use the server reflection API for this? if your service has this enabled, we could potentially run the reflection code at client and server and highlight any differences? We could probably encapsulate this into some kind of channel.ValidateServiceMetadata<TMyService>() API, which would do all the things?

ptsoccer commented 4 years ago

Just to be clear, I'm not asking any problems to be fixed. I understand the complexity in #70.

For 2.) I'm lazy and attributes are annoying to type out manually, so I write one property with the attribute, then paste the property + attribute definition as many times as I need properties, then do a fixup. Obviously this is error prone and creates this type of issue. It would just be nice to know this earlier in the dev lifecycle.

If something like you suggest were available that would be great and would solve the issue.

mgravell commented 4 years ago

@ptsoccer can you give a specific example of the kind of thing you mean? basically, what I'm trying to get at here is: is this something that I could fix with an analyzer, for example, for things like duplicated field numbers? basically: what scenarios are we talking about here - that would really help

ptsoccer commented 4 years ago
[ProtoContract]
    public class CopyRequest
    {
        [ProtoMember(1)]
        public int ToID { get; set; }

        [ProtoMember(1)]
        public int FromID { get; set; }
    }

I'd say this could be handled by an analyzer. Sometimes I also forget a Service attribute to the Grpc services. Another issue I've had is using the DataMember/DataContract attributes and getting errors because it can't find the System.ServiceModel.Primitives package (easy to just not use those attributes and instead use the Proto* ones). The issue is there's probably just a bunch of one off configuration issues that can all be handled individually, but this will only tell you about the issues you know about and code for.

I will say that I do like the analyzer approach a lot for some of these since now you'll know at compile time rather than start/test/method call time.

christopheblin commented 4 years ago

I need to say that I'd like the same thing here.

We are exposing services both with gRPC and with HTTP, everything is fine and everyone is happy.

The problem we have is that, sometimes, there are only HTTP clients (and no gRPC client). And so, in the impl, we sometimes forget "details" like the fact that you can not have more than one parameter in the method or 2 attributes with the same order number or ... However, at some point, there is someone that wants to use the gRPC endpoint and he takes a really harsh error (this is an easy fix but we must redeploy to all production servers, and we are not a SaaS ...)

So it would definetly be a good thing if we can test that the gRPC definitions are correct upfront.

At the moment, we have tried :

    [Fact]
    public void TestContracts()
    {
        var f = new CodeFirstGrpcClientFactory(new HttpClient());
        f.BuildClient<IMyService>("https", "dummy", 80);            
    }

However, this is working even if we miss the ServiceContract or a DataMember attribute...

Is there a way to be more proactive here ?

mgravell commented 4 years ago

Yes there absolutely is. The problem is: it all takes time and effort, and this isn't my day job..

You're right, we could and should give more output about problems that we see during the reflection phase. I'm not entirely sure that it is a good idea to throw an exception too early, though, as having one method that doesn't work may be acceptable, if all the others do. So throw at call, but with better guidance.

Longer term: I have some very specific plans here including analyzers (so you get a warning in the IDE) and generators (so there is no runtime reflection/emit code), and we also plan to finish the work to allow multiple parameters.

christopheblin commented 4 years ago

As @ptsoccer soccer said, Analyzer will be great, that is for sure!

Regarding the throw at call vs throw at reflection time, I sit on the side of "fail fast" so I definitely prefer reflection time. But I understand that this may not work for everybody and there is some choice to be made here

I'd like to understand if there is a way NOW to "manually" launch the "reflection time" and to throw errors if there is a problem (inside a unit test). Or maybe just calling BuildClient like we did is the best thing for now (until the analyzer) ?

Otherwise, on the top of my mind, another example we recently faced was that the method took asingle parameter that was a Guid (I do not remember the exact error, but it was cryptic). We just use a wrapping DTO (which is always good anyway, especially for long term) but this was not easy to figure out, so definetly better if the error messages can be easier to understand.

Anyway, as I have a chance to tell, thanks for your great work, because I cannot emphasize enough how harder it will be for us to write the proto files first!

mgravell commented 4 years ago

I'd like to understand if there is a way NOW to "manually" launch the "reflection time" and to throw errors if there is a problem (inside a unit test).

right now, no API exists for that; however, I'm totally open to adding one; this could be:

thoughts and ideas welcome