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

Issue: example error parsing problem #280

Open
wants to merge 225 commits into
base: master
Choose a base branch
from

Conversation

jschaul
Copy link
Contributor

@jschaul jschaul commented Jan 13, 2021

This PR is twofold:

  1. bug report
  2. question about unit testing

1. bug report

As we deviate from the "happy path" (e.g. here when throwing an error), using mu-grpc-client becomes problematic.

sayHello function:

sayHello :: MonadServer m => HelloRequestMessage -> m HelloReplyMessage
sayHello (HelloRequestMessage nm) = do
  case nm of
    -- in some cases we want to throw an error, here when the name sent is 'Bob'
    "Bob" -> throwError $ ServerError NotFound "Bob not there"
    _ -> pure $ HelloReplyMessage ("hi, " <> nm)

To run this example:

stack exec error-parsing

Output:

The result for saying hello to Alice: "GRpcOk \"hi, Alice\""
The result for saying hello to Bob: GRpcErrorString "not enough bytes"

As can be seen, any error thrown will be falsely interpreted by grpc-client as
"GRpcErrorString". This makes any real-life application with grpcclient
impossible (there are not just happy paths, and not all errors are the
same)

The server side implementation appears correct, as I can poke the server
with the grpcurl tool:

grpcurl -d '{"name": "Bobby"}' -format json -plaintext -proto errorparsing.proto localhost:8070 hello.Service.SayHello
{
  "reply": "hi, Bobby"
}

grpcurl -d '{"name": "Bob"}' -format json -plaintext -proto errorparsing.proto localhost:8070 hello.Service.SayHello
ERROR:
  Code: NotFound
  Message: Bob not there

Related issue: haskell-grpc-native/http2-grpc-haskell#6

Why am I filing this PR here?

Providing easy-to run example code along a github issue is hard, therefore I opted to use a PR instead.

More generally, I would have preferred to make this example a unit test (expecting the reply from Bob to be of a certain type which is not GRpcErrorString "not enough bytes"), however this whole project seems to not contain any unit tests at all.

2. Question about unit tests and production-readyness

Do you plan to set up some tasty or hspec (or something else) unit tests more generally? (why are there no unit tests so far? That makes me doubt whether the mu family of libraries can safely be used in any project other than toy projects. Can you comment on the stability of the libraries outside this particular bug?)

In case unit tests are more generally set up, this PR could be converted to be another unit test, which would allow to more easily catch other parsing issues in the mu family of libraries.

Flavio Corpa and others added 30 commits November 18, 2019 09:39
* stylish-haskell 💅🏼

* fix minor typos

* prepare avro QuasiQuoter

* Implement QuasiQuoter! 🎉

* make travis happy :)

* Implement unions and add examples

* fix ByteString encoding + [avroFile|example] 🐛

* Handle TOption case ⌥

* Unwrap first union and contemplate other cases! 🙌🏼

* Start work on flattenDeclarations and revert stylistic changes

* Handle nested schemas! 🕸

* fix travis 🤦🏼‍♂️
* Separate Avro and ProtoBuf things in their own adapter packages
* Separate grpc packages in client and server

Fixes higherkindness#19
* Run stylish-haskell in all files! 💅🏼
* Start work on todolist example ✅

* Fix .proto and implement Definition! 📚

* start work on server

* Implement server! 🚀

* Finish server! 💻

* apply feedback! 🧼

* prevent t to change between calls 🐛

* cleanup
Fixes higherkindness#31
Fixes higherkindness#36

Co-Authored-By: Flavio Corpa <flavio.corpa@47deg.com>
serras and others added 27 commits January 29, 2021 08:35
Co-authored-by: Flavio Corpa <flavio.corpa@47deg.com>
Bumps [kramdown](https://github.com/gettalong/kramdown) from 2.1.0 to 2.3.1.
- [Release notes](https://github.com/gettalong/kramdown/releases)
- [Changelog](https://github.com/gettalong/kramdown/blob/master/doc/news.page)
- [Commits](https://github.com/gettalong/kramdown/commits)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Closes higherkindness#297 

In protobuf, unions themselves are optional, so while parsing if any value fails
to parse, its default must not be considered. When the default is considered and
first value in the union is not specified, the union still gets parsed as the
default of the first value. So, in this instance:

oneof foo {
  int32 foo_int = 1;
  string foo_str = 2;
}

foo_str will never be parsed, as foo_int will have a default, i.e. 0 and so
foo_str "blah" will always wrongly parse as foo_int 0.

Co-authored-by: Akshay Mankar <itsakshaymankar@gmail.com>
Co-authored-by: Flavio Corpa <flavio.corpa@47deg.com>
…rkindness#312)

Changes:
- Don't cancel the handler when server stream ends, this will allow the handler
to finish.
- Use TMChan instead of TMVar for consuming the server stream, this allows the
handler to indicate that it has finished and there will be no more output being
produced.
)

This is a breaking change: The handler for bidirectional streams is returns two
conduits now, instead of one. This enables the client to correctly tackle the
concurrent nature of the client to server stream and the server to client
stream.

Each response in the server-to-client stream is no longer wrapped in GRpcReply,
any error during parsing the stream is thrown in IO.

Other connection related errors are returned in the result value of the conduit
corresponding to the server-to-client Conduit.

Note: The client didn't and still doesn't handle any errors that the server
might indicate using headers or trailers, e.g. grpc-status or the HTTP status
code. This commit also adds TODO comments to handle these.
Bumps [addressable](https://github.com/sporkmonger/addressable) from 2.7.0 to 2.8.0.
- [Release notes](https://github.com/sporkmonger/addressable/releases)
- [Changelog](https://github.com/sporkmonger/addressable/blob/main/CHANGELOG.md)
- [Commits](sporkmonger/addressable@addressable-2.7.0...addressable-2.8.0)

---
updated-dependencies:
- dependency-name: addressable
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request protobuf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet