V1: Fix lint errors and wire up to CI #605
Conversation
|
@jhump if you'd rather I split these up into smaller PRs, that's cool too. Just figured since this was mostly inconsequential chore work, I'd combine them into one. |
jhump
changed the title
| // A unary endpoint that the server should not implement and should instead | ||
| // return an unimplemented error when invoked. | ||
| rpc Unimplemented(UnimplementedRequest) returns (UnimplementedResponse); |
There was a problem hiding this comment.
I don't think this is sufficient. It is perhaps useful, but it only captures the case where the server handler does provide the method: it's in the proto and thus in the generated server interfaces. So the only reason it will be unimplemented is because the server framework is supplying a default implementation.
We will also eventually need to test completely unimplemented services and methods -- service and method names not known to the server at all. This can't be done right now using a simple switch in the reference client, so we can punt on it.
In the client implementation (not this PR), we probably want a TODO in the switch default that we'll eventually want to support other dispatch mechanisms -- that will allow us to do all kinds of things, like simulate exotic or intentionally incorrect client behavior, to verify how the server reacts.
Note that only the reference will need to be able to craft requests for such unknown services and methods, since that is testing the server behavior. Other clients-under-test would not need to worry about it.
There was a problem hiding this comment.
Makes sense. Should I just remove this here? I feel like it's useful to test the scenario you mention: server knows about a service, but has not yet implemented it. But, I can certainly delete it if you don't think it provides that much value.
There was a problem hiding this comment.
The old protos defined a unary and a server-streaming RPC that should not be implemented, and a separate service that should not be implemented, also with unary and server-streaming RPCs. It never seemed like a good solution to me.
I tend to think that it might be sufficient to:
- have the reference client call unknown RPCs to verify they respond with either HTTP 404 or a gRPC / Connect unimplemented error
- have the reference server respond with HTTP 404 and gRPC / Connect unimplemented error responses for certain test cases (based on test name in a request header)
This doesn't verify that servers reply with an in-protocol unimplemented error, but IMO this behavior could be considered an application-level or framework-level feature, not strictly a requirement by the protocol. On the upside, it doesn't introduce RPCs for the testee to reason about.
WDYT?
There was a problem hiding this comment.
Yep, that was basically along the lines I was thinking for how to expand on this. Only the reference clients/servers need to handle these edge cases (i.e. a client-under-test will never need to be able to send a request for an unimplemented service or method).
| // This field is only relevant in the first message in the stream | ||
| // and should be ignored in subsequent messages. | ||
| // Note, this is only applicable to bidi endpoints. | ||
| bool full_duplex = 16; |
There was a problem hiding this comment.
Hmmm. I thought this change had already been made.
There was a problem hiding this comment.
That was in server_compat.proto. Changing it here to match.
jhump
changed the title
jhump
changed the title
This performs some minor cleanup to the repo. Namely:
make lintUnimplementedendpoint toConformanceServiceso that unimplemented RPCs can be testedtest,lint, andcheckgenerate. Note, there are no tests right now, but some could be added in the future.buf.gen.yamlto use the remote plugin forprotoc-gen-gorather than rely on a locally installed plugin. Note this also updates to use latest version (1.31.0)