Allow propagation of errors from Subscriptions channels into Request.… #317
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
…fixing them with Query, Mutation and Subscription.
Prevent panic when "var f *fieldToExec" is nil Added field "name" on pack error
Fix subscribe panic
return it untouched.
graphql.go
Outdated
| @@ -69,6 +69,7 @@ type Schema struct { | |||
| logger log.Logger | |||
| useStringDescriptions bool | |||
| disableIntrospection bool | |||
| prefixRootFunctions bool | |||
There was a problem hiding this comment.
This is a separate feature, allowing you to have the same function name in subscription and query in the GraphQL schema.. yet routing to different functions in Go, because all fields under query would be prefixed Query, those under subscription would be prefixed Subscription, and the same with mutation -> Mutation.
I can try to split the PRs if you would prefer to add only a single feature at a time.
There was a problem hiding this comment.
Yes, please. I would prefer one logical change per PR. Please, remove the prefixes of root functions from this PR.
|
Please, resolve the conflicts. |
| return func(s *Schema) { | ||
| s.prefixRootFunctions = true | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this change needed in order to propagate subscription errors?
|
bump |
# Conflicts: # graphql.go # internal/exec/resolvable/resolvable.go
| @@ -4,6 +4,10 @@ import ( | |||
| "fmt" | |||
| ) | |||
|
|
|||
| type SubscriptionError interface { | |||
There was a problem hiding this comment.
Why do we need this interface?
There was a problem hiding this comment.
It's used below with streaming responses.. so the object you stream back can actually transform the response into a proper error (GraphQL-style).. otherwise, it was impossible to return an object with an error.. you were forced to make the data contain some error field or whatnot.
| @@ -225,8 +225,9 @@ func (p *StructPacker) Pack(value interface{}) (reflect.Value, error) { | |||
| for _, f := range p.fields { | |||
| if value, ok := values[f.field.Name.Name]; ok { | |||
| packed, err := f.fieldPacker.Pack(value) | |||
|
|
|||
There was a problem hiding this comment.
Unnecessary empty line, Please, remove it.
| @@ -229,17 +232,23 @@ func (b *execBuilder) makeObjectExec(typeName string, fields schema.FieldList, p | |||
| Fields := make(map[string]*Field) | |||
| rt := unwrapPtr(resolverType) | |||
| for _, f := range fields { | |||
|
|
|||
There was a problem hiding this comment.
No need for this empty line. Please, remove.
| // TODO: move this check into validation.Validate | ||
| if len(fields) != 1 { | ||
| err = errors.Errorf("%s", "can subscribe to at most one subscription at a time") | ||
| errs = []*errors.QueryError{errors.Errorf("%s", "can subscribe to at most one subscription at a time")} |
There was a problem hiding this comment.
Under what circumstances can we have more than one error?
There was a problem hiding this comment.
It was tweaked to accomodate line 34 up here.. it can hold multiple errors, so it was easier to pass down a list of errors.. instead of assuming there would only be one in there.
|
wow I'm sorry I didn't tend to this earlier.. I'll try to review tomorrow. Unfortunately, there are two changes combined in this PR.. one is for prefixes on root elements (adds Query and Subscription and Mutation, to the top-level function names), the other is the subscriptions propagation of errors. I'll get back to you, and perhaps split both. |
|
I want to review the changes separately. Please, send the error propagation changes only. |
|
ping |
|
hey @pavelnikolov .. is there a way I can reach out to you privately? Perhaps on Twitter, I connected (@bourgetalexndre). |
|
Yes, I'm usually active in gophers.slack.com - same nickname. |
| timeout := r.SubscribeResolverTimeout | ||
| if timeout == 0 { | ||
| timeout = time.Second | ||
| } |
There was a problem hiding this comment.
I like this one and I'd be happy to merge it. Not sure how I've missed the hard-coded time.Second. I'll cherry pick it in a separate PR, though.
added 2 commits
November 10, 2020 15:38
# Conflicts: # internal/exec/resolvable/resolvable.go # internal/exec/subscribe.go
…lver itself panicks The internal exec Subscribe method had code to deal with subscription resolver panicking when being called. But when such handling happen, the error is attached to the request object and it never checked later on. This leads to some zero checks to fail when we try to extract the type from the resolver's channel since this variable was never set. Doing this creates a second panic which is not handled and make the application die. To fix the issue, we now check if there is errors on the request object before continuing with the rest of the check, if there is errors, it's because a panic occurs and we return the response right away.
maoueh
pushed a commit
to streamingfast/graphql-go
that referenced
this pull request
Nov 11, 2020
The previous value was hard-coded to 1 second. This is problematic for resolver that takes more time than this to return a result. When parsing the schema, it's not possible to pass a custom value for the subscription resolver timeout. Extracted from graph-gophers#317
This was referenced Nov 11, 2020
|
@rawagner @pavelnikolov Extracted into #419 |
This was referenced Nov 11, 2020
Closed
|
Almost everything has been split into separated laser-focus PRs. I'll do a last pass tomorrow and close this PR since it has been split into smaller chunks. |
|
Ok I just checked and the other issue that we had a fix in this branch has been fixed already by c80e625, so there is nothing worth left in this integration branch, I'm going to close this PR, split in multiple smaller ones: |
|
split into multiple pull requests |
Umanish
pushed a commit
to tokopedia/graphql-go
that referenced
this pull request
May 15, 2023
* Support for embedded struct type in resolver * fix bug in slice pop * fix bug while finding field * add 'getFieldCount' to resolve ambiguity * Increase extensions test coverage * Remove duplicate unit tests * rename 'getFieldCount' to 'fieldCount' * add test for ambiguous field panic * add unit tests for embedded struct feature * rename TestEmbedded => TestEmbeddedStruct * Fixes graph-gophers#357 * Actually fix graph-gophers#357 * Print context to panic log * Add Example of Custom Errors Adding example and documentation for how to create custom error implementations which include `extensions` within their `error` payload * Clarify errors for mismatching input implementation Producing clearer error messages when field input arguments are implemented by code: * Which does not match the schema e.g. missing field; or * Function missing struct wrapper for field arguments * Allow `schema` to be omitted when using default root op names * Strip Common Indentation from BlockString Descriptions Multi-line descriptions need to have their common indentation level (which results from indentation of that part of the schema, rather than being intentional for the description text) removed to ensure the descriptions use the correct value, are formatted correctly etc This is to meet the condition documented in the GraphQL spec: https://graphql.github.io/graphql-spec/June2018/#sec-String-Value > Since block strings represent freeform text often used in indented > positions, the string value semantics of a block string excludes > uniform indentation and blank initial and trailing lines via > BlockStringValue(). * Syntax highlighting fixed in README * Add walkthrough Fixed small punctuation and added my walkthrough package * Update README.md * Add support for directives in schema parser * Use operationName from query if missing from POST * Fix SIGSEGV when client subs to multiple fields * bugfix: correctly determine fragment usage In previous versions of this code, this validation would exit when it encountered a fragment legitimately used twice. This bugfix skips the recursion but does not stop progress altogether allowing other fragments to be marked as used. * Limit the number of concurrent list nodes processed It uses the current capacity of the limiter as a hint as this is set based on the maxParallelism field on the schema. * Remove need for WaitGroup * More descriptive error when unmarshaling ID/Time This adds a tiny bit more information to the error messages produced when unmarshaling an input value to an ID or Time fails. * Improve README.md Fixes graph-gophers#307 Add short descriptions for different schema options. Move community examples to wiki. Add companies that use this library. * fix graph-gophers#241 Similar to graph-gophers#407, but adds test cases. * Add comment explaining why we limit concurrency * Issue graph-gophers#299: unclear error message in case of multiline string argument * handle case where interface is type-asserted to same interface * Issue graph-gophers#299: unclear error message in case of multiline strings * Issue graph-gophers#299: unclear error message in case of multiline string argument * Update logic to always check for nil pointer returns * Adding variables parameter for query validations. * Fixed `reflect.Value.Type on zero Value` panic when subscription resolver itself panicks The internal exec Subscribe method had code to deal with subscription resolver panicking when being called. But when such handling happen, the error is attached to the request object and it never checked later on. This leads to some zero checks to fail when we try to extract the type from the resolver's channel since this variable was never set. Doing this creates a second panic which is not handled and make the application die. To fix the issue, we now check if there is errors on the request object before continuing with the rest of the check, if there is errors, it's because a panic occurs and we return the response right away. * Added possibility to customize subscription resolver timeout value The previous value was hard-coded to 1 second. This is problematic for resolver that takes more time than this to return a result. When parsing the schema, it's not possible to pass a custom value for the subscription resolver timeout. Extracted from graph-gophers#317 * Allowed Subscription resolver to return `*QueryError` directly Previously, any error returned by the Subscription resolver was immediately wrapped inside its own `*QueryError` value even if the returned error was already a `*QueryError`. Now, when receiving such types, we use it as-is without wrapping again. * Adding/removing empty lines where needed * DisableIntrospection should not skip __typename for usages of GraphQL union types * Add context to validation tracing Context is needed for tracing to access the current span, in order to add tags to it, or create child spans. As presently defined (without a context), this cannot be done: new spans could be created, but they would not be associated with the existing request trace. OpenTracingTracer implements the new interface (it never implemented the old one). Via this 'extension interface', the tracer configured (or the default tracer) will be used as the validation tracer if: * The tracer implements the (optional) interface; and * A validation tracer isn't provided using the deprecated option What this means is that the deprecated option is _preferred_ as an override. This allows users to migrate in a non-breaking, non-behaviour changing way, until such time as they intentionally remove the use of the deprecated option. For those who are currently using the default tracer, and not supplying a validation tracer, validation will be traced immediately with no change required to configuration options. * Add support for nullable types This allows to differentiate between an omitted value and a null value in an input struct. * Fixed duplicated __typename in response (fixes graph-gophers#369) * Create CHANGELOG.md * Update CHANGELOG.md * ignore JetBrains IDEA and vscode meta directories * expose packer.Unmarshaler interface as graphql.Unmarshaler - add tests for graphql.Time as reference implementation * move packer.Unmarshaler interface to decode.Unmarshaler, so the methods are actually visible * add types package Part of graph-gophers#434 and related to graph-gophers#116 this change adds a new package containing all types used by graphql-go in representing the GraphQL specification. The names used in this package should match the specification as closely as possible. In order to have cohesion, all internal packages that use GraphQL types have been changed to use this new package. This change is large but mostly mechanical. I recommend starting by reading through the `types` package to build familiarity. I'll call out places in the code where I made decisions and what the tradeoffs were. * add getter for the types.Schema field This additive function shouldn't break backward compatibility will allow those who want access to the types to get at an AST version of the `types.Schema` * unused fields * rename to match types * remove unused * use a string and not an Ident for a FieldDefinition's name This was an error. When this field was renamed from schema.Field (to avoid ambiguity) its name field changed to match query.Field (to Ident). This caused a cascade of useless changes that will be rolled back in the next commit * fix compile errors introduced by ab449f0 * merge conflict errors * add location fields to type definitions * Fix dir in readme * coerce float64 to int32 in NullInt and vice versa in NullFloat * errors.Errorf preserves original error similar to fmt.Error * removed test dependency on errors.Is * checkErrors ignores the raw error for purposes of determining if the test passed or failed * Update CHANGELOG.md * internal/exec: assign parent type name to __typename fields * Accepting value Json in parameter of request's body in custom Scalar (graph-gophers#467) Accept JSON value in resolver args * Add option for custom panic handler (graph-gophers#468) Add option for custom panic handler * Tests showing query variables are validated correctly (graph-gophers#470) * README nit -- Move '$' out of cut/paste buffer (graph-gophers#473) Move '$' out of cut/paste buffer * internal/exec/resolvable: include struct field name in errors (graph-gophers#477) * internal/exec/resolvable: include struct field name in errors We were only adding method name, which meant that it was taking an empty string if the resolver was a struct field. This was making the error messages hard to parse as the user can't know which field has the error. Added a check to use the correct variable. * improve test * ci: setup SemaphoreCI v2 (graph-gophers#479) Update Semaphore configuration * Support "Interfaces Implementing Interfaces" (graph-gophers#471) Interface implementing interfaces support https://spec.graphql.org/draft/#sec-Interfaces.Interfaces-Implementing-Interfaces * README.md: Fix build status badge I broke this accidentally when removing the legacy SemaphoreCI integration. * fix golangci lint errors in the codebase (graph-gophers#478) Added a base golangci-config to the codebase to get started. Some more changes are pending, and those checks are commented out in the config. * Improve Sempahore CI (graph-gophers#481) Improve Sempahore CI build * Make some more golang-ci improvements (graph-gophers#483) * graphql.Time unmarshal unix nano time (graph-gophers#486) * validation: fix bug in maxDepth fragment spread logic (graph-gophers#492) * Create codeql-analysis.yml * Add OpenTelemetry Support (graph-gophers#493) Add OpenTelemetry tracer implementation * Improve the Getting Started section * Update README.md * Improve the Getting Started section in the README * Create SECURITY.md * Fix the OTEL tracer package name (graph-gophers#495) * Fix parseObjectDef will terminate when object has bad syntax (graph-gophers#491) (graph-gophers#500) Thank you for your contribution * Fix remove checkNilCase test helper function (graph-gophers#504) * Add graphql.Time example (graph-gophers#508) * Apollo Federation Spec: Fetch service capabilities (graph-gophers#507) Add basic support for Apollo Federation Co-authored-by: Alam <sulthan.alam@lemonilo.com> Co-authored-by: pavelnikolov <me@pavelnikolov.net> * Ignore yarn.lock file * add support for repeatable directives (graph-gophers#502) add support for repeatable directives * Fix example/social code (graph-gophers#510) The `Friends` field had higher priority than the `FriendsResolver` method. This is the reason why the field was renamed to a value, that doesn't match the GraphQL resolver. * Fix lint error (graph-gophers#512) * Refactor trace package (graph-gophers#513) Remove dependency for graphql-go on OpenTracing and OpenTelemetry except where those tracers are explicitly configured for use. * Adding in primitive value validation. (graph-gophers#515) * Update README.md * Update README.md * Improve type assertion method argument validation (require zero) (graph-gophers#516) Improve type assertion method argument validation (require zero) It's tempting to include a context argument (or think it's allowed), but not discover that this will fail until a query is executed. Validating the resolver during schema parsing reduces the chance of inadvertant errors here. Signed-off-by: Evan Owen <kainosnoema@gmail.com> * Disallow repeat of non repeatable directives (graph-gophers#525) * Disallow repeat of non repeatable directives * Remove unnecessary scallar * Added changes lost after package update * merging old prs * adding gqlerrors support * adding dev message, error code support * Readded Export query name method functionality after package update * Fix: extension initialisation and updated error method to return extension details * updated QueryError Extensions to not emit if empty --------- Signed-off-by: Evan Owen <kainosnoema@gmail.com> Co-authored-by: Elijah Oyekunle <eloyekunle@gmail.com> Co-authored-by: Pavel Nikolov <me@pavelnikolov.net> Co-authored-by: Pavel Nikolov <pavelnikolov@users.noreply.github.com> Co-authored-by: Dorian Thiessen <Dorian.thiessen@usask.ca> Co-authored-by: Ivan <ivan.petrus@kumparan.com> Co-authored-by: David Ackroyd <dackroyd@fairfaxmedia.com.au> Co-authored-by: pavemaksim <pavemaksim@gmail.com> Co-authored-by: Tony Ghita <ghita71@gmail.com> Co-authored-by: Zaydek <zaydekdotcom@gmail.com> Co-authored-by: Sylvain Cleymans <sylvain@movio.co> Co-authored-by: will@newrelic.com <will@newrelic.com> Co-authored-by: Nicolas Maquet <nicolas@movio.co> Co-authored-by: Sean Sorrell <seansorr@twitch.tv> Co-authored-by: Ryan Slade <ryanslade@gmail.com> Co-authored-by: Thorsten Ball <mrnugget@gmail.com> Co-authored-by: obei <obei.sideg@gmail.com> Co-authored-by: Quinn Slack <quinn@slack.org> Co-authored-by: suntoucha <suntoucha@gmail.com> Co-authored-by: Barry Dutton <dutbarry@justin.tv> Co-authored-by: Sebastian Motavita <Sebastian.Motavita@endava.com> Co-authored-by: Matthieu Vachon <matt@dfuse.io> Co-authored-by: Epsirom <chenhuarongzp@gmail.com> Co-authored-by: David Ackroyd <23301187+dackroyd@users.noreply.github.com> Co-authored-by: Vincent Composieux <vincent@composieux.fr> Co-authored-by: Silvio Ginter <silvio.ginter@esome.com> Co-authored-by: Sam Ko <samuko@twitch.tv> Co-authored-by: jinleileiking <jinleileiking@gmail.com> Co-authored-by: Edward Ma <edward@catch.co> Co-authored-by: Matt Ho <matt.ho@gmail.com> Co-authored-by: Tony Ghita <tony@twitch.tv> Co-authored-by: Gustavo Delfim <gusttavodelfim@gmail.com> Co-authored-by: John Starich <johnstarich@gmail.com> Co-authored-by: Florian Suess <floriansuess96@icloud.com> Co-authored-by: wejafoo <79415032+wejafoo@users.noreply.github.com> Co-authored-by: Agniva De Sarker <agnivade@yahoo.co.in> Co-authored-by: Steve Gray <steve-gray@users.noreply.github.com> Co-authored-by: Connor Vanderhook <14183191+cnnrrss@users.noreply.github.com> Co-authored-by: roaris <61813626+roaris@users.noreply.github.com> Co-authored-by: Sulthan Alam <40392850+aeramu@users.noreply.github.com> Co-authored-by: Alam <sulthan.alam@lemonilo.com> Co-authored-by: speezepearson <speezepearson@users.noreply.github.com> Co-authored-by: Dallas Phillips <dallasphillips24@gmail.com> Co-authored-by: Evan Owen <kainosnoema@gmail.com> Co-authored-by: Igor <9917165+ostrea@users.noreply.github.com> Co-authored-by: Amritansh Kumar <amritansh.kumar@tokopedia.com> Co-authored-by: kumaramritansh <105722986+kumaramritansh@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Please see open issue:
#314