Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document why ServerReaderResponse takes a Maybe response #13

Open
crclark opened this issue Apr 30, 2017 · 3 comments
Open

Document why ServerReaderResponse takes a Maybe response #13

crclark opened this issue Apr 30, 2017 · 3 comments
Assignees

Comments

@crclark
Copy link
Collaborator

crclark commented Apr 30, 2017

I just came across this and can't remember why it's optional to return a response message to a client streaming RPC.

The gRPC docs don't mention that the server's response message is optional. It seems to me that either both unary and client streaming responses should be optional, or neither should be optional.

Either way, it should be documented.

@ixmatus
Copy link
Contributor

ixmatus commented Apr 30, 2017

HTTP has a notion of a successful but empty bodied response: the 204 No Content. It wouldn't surprise me if gRPC had a similar response code since it's based on HTTP2. Nothing for an empty bodied but successful response makes a lot of sense to me, though it's a bit funky for the other responses that might expect a non-empty response body.

@intractable
Copy link
Contributor

intractable commented May 1, 2017

Gah, I'm pretty sure there was a reason for this -- I thought that perhaps because of a usage pattern we noticed in some of the C++ client-streaming examples. However, when I went and looked I couldn't reconstruct it. I'm not sure it makes sense either, given that the rpc definition for client-streaming endpoints always specifies a response type. And, as @crclark said, I'm not aware of anything that claims a response message is optional.

I think we should try dropping the Maybe and check interop against the C++ versions as a sanity check. I'll assign this to myself for the time being; will try to look at it soon.

@intractable intractable self-assigned this May 1, 2017
@crclark
Copy link
Collaborator Author

crclark commented May 2, 2017

Looking at the types again, I agree with @ixmatus that the type seems to be accounting for status codes that don't include a response message, and all of our high level types should have Maybe responses. Specifically, we have status codes like UNAUTHENTICATED, but the type of the high level unary RPCs guarantees a response message, which seems wrong prima facie. I'm thinking the high level types need to be redesigned around the assumption that we can receive status codes without a response.

This is kind of related to #3 (I'm going to add some detail to that issue, too).

RichardWarfield pushed a commit to litxio/gRPC-haskell that referenced this issue Apr 25, 2023
The derived `Show` instance for `FieldNumber` is more verbose than
necessary.  This change takes advantage of the fact that `FieldNumber`
implements `Num` so that the `Show` implementation can display just a
numeric literal.  This is still valid code for building a `FieldNumber`.
RichardWarfield pushed a commit to litxio/gRPC-haskell that referenced this issue Apr 25, 2023
* Misc cleanup/refactor pass on test suite descriptions and structure (awakesecurity#13)

* Rename module TestTypes => OldTestTypes

* Rename test.proto and test_import.proto package names to GeneratedTestTypes and GeneratedImportedTestTypes respectively

* +tests/Generated{,Imported}TestTypes.hs and related changes

* Add (:: Int) annotation to enum bound to avoid default type warning in generated modules

* (qcPropDecEncId test) Replace OldTestTypes types with GeneratedTestTypes types

* Remove dependency on quickcheck-text

* Add and support additional fields for `WithNesting` in test.proto

We did this to match prior `NestedMsg` use in `OldTestTypes` as part of the
effort of redirecting the test suite to operate on code generated from .protos
rather than hand-rolled types.

* Use GeneratedTestTypes for remainder of the current types under test (after making small tweaks for type parity)

* Add pretty-show dep so it can be used in the repl

* Fix bug in the `MessageField` instance for `PackedVec Word32`

* Add helpers/instances for pprinting simplified, single-message DotProtos from corresponding Haskell types; comments

* Minor module reorg

* Remove warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants