This repository has been archived by the owner on Oct 19, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 19
Issue: example error parsing problem #280
Open
jschaul
wants to merge
225
commits into
higherkindness:master
Choose a base branch
from
jschaul:example-grpc-client-error
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Issue: example error parsing problem #280
jschaul
wants to merge
225
commits into
higherkindness:master
from
jschaul:example-grpc-client-error
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* 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 <[email protected]>
Co-authored-by: Flavio Corpa <[email protected]>
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] <[email protected]> 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 <[email protected]>
Co-authored-by: Flavio Corpa <[email protected]>
…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] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is twofold:
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:
To run this example:
Output:
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:
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.