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

How to return an error only at one element of an array? #414

Open
marcosgmeli opened this issue Oct 15, 2020 · 5 comments
Open

How to return an error only at one element of an array? #414

marcosgmeli opened this issue Oct 15, 2020 · 5 comments

Comments

@marcosgmeli
Copy link

Hi all, you have made a great work on the repo, thanks for it.

I am new at GraphQL but for what I understand from the specs if an element of an array can't be returned it should create an error, but unless the wrapped element is a nonNull, it should not null the entire list, what makes sense thinking on a best effort approach, and is a similar result to when a NonNull field of an element fails.

I think that is similar to be able to treat every element of an array as a field.

We have tried several ways but can't find the one to achieve this behavior, is it posible? the specs really support it?

Thanks in advance.

@pavelnikolov
Copy link
Member

I'm not sure I follow. Could you, please, give an example?

@sGy1980de
Copy link
Contributor

sGy1980de commented Oct 6, 2021

I'll jump in here because I'm currently facing the exact same problem.
Let's say you get a list of ids returned by some aggregation, but this list is not secured via foreign key constraints for some reason. When you now want to resolve this list of ids into their corresponding entities, but it fails for one of these, there is little you can currently do to report this issue in the response. So either you just drop the failing item silently or you report an error in the resolver, but then all the items being resolved successfully are lost as well.

type entityRepository struct{}

func (e *entityRepository) Lookup(id int) (*Entity, error) {
	// imagine some db lookup here
	return nil, nil
}

type Entity struct {
	Id int
	Name string
	Deleted bool
}

type resolver struct {}

func (r resolver) Entities() (list []Entity, err error) {
	// imagine this to be the raw list of ids coming from some aggregation, but not secured via fk constraint 
	var ids = []int{1337, 4711, 90210}

	er := new(entityRepository)
	for _, id := range ids {
		entity, err := er.Lookup(id)
		if err != nil {
			// what to do here???
			// option 1: we could just raise the error, but then all the items resolved successfully will be lost
			// option 2: we could just drop/log the issue, but then the user will never know
			continue
		}
		list = append(list, *entity)
	}
	return
}

I personally would wish for some sort of soft error here. So it will be reported in the error field in the response, but the successful cases are still present in the response.

@pavelnikolov
Copy link
Member

@marcosgmeli, @sGy1980de
I verified and this is a bug indeed. Can be reproduced by:

package main

import (
        "log"
        "net/http"

        graphql "github.com/graph-gophers/graphql-go"
        "github.com/graph-gophers/graphql-go/relay"
)

type query struct{}

func (*query) Words() ([]*string, error) {
	word1 := "word1"
	word3 := "word3"
	word4 := "word4"
	res := []*string{&word1, nil, &word3, &word4, nil}
	return res, fmt.Errorf("something went wrong with indices 1 and 4")
}

func main() {
	s := `
		type Query {
			words: [String]!
		}
	`
	schema := graphql.MustParseSchema(s, &query{})
	http.Handle("/query", &relay.Handler{Schema: schema})
	log.Fatal(http.ListenAndServe(":8080", nil))
}

and then running:

curl -XPOST -d '{"query": "{ words }"}' localhost:8080/query

we get back:

{
  "errors": [
    {
      "message": "something went wrong with indices 1 and 4",
      "path": [
        "words"
      ]
    }
  ],
  "data": null
}

Where I would expect to receive:

{
  "errors": [
    {
      "message": "something went wrong with indices 1 and 4",
      "path": [
        "words"
      ]
    }
  ],
  "data": {
    "words": ["word1", null, "word3", "word4", null]
  }
}

@pavelnikolov
Copy link
Member

Actually, after more carefully reading the spec, the library works perfectly fine where in the above example if we don't return the error, the result would be:

{
  "data": {
    "words": [
      "word1",
      null,
      "word3",
      "word4",
      null
    ]
  }
}

which corresponds to option 2) in @sGy1980de 's comment. This exactly what the spec recommends that we should do:

Screenshot 2023-03-02 at 17 37 58

I am closing this issue as the library already supports the result coercion recommended in the spec.

@pavelnikolov
Copy link
Member

The only thing that I am not sure about is that some fields return an Error: <error_here> and others have (With logged error). I wonder what does this mean? Maybe it's worth checking the reference implementation of the graphql-js library.

@pavelnikolov pavelnikolov reopened this Mar 2, 2023
ggilmore added a commit to sourcegraph/sourcegraph-public-snapshot that referenced this issue Jun 17, 2024
…errors that occur when fetching permission syncs (#63302)

Closes
https://linear.app/sourcegraph/issue/SRC-421/customer-reported-an-issue-with-user-permissions-stats

This PR ensures that any non not-found errors that occur when fetching
permission sync jobs are logged and added as error events in the trace.
Before, these errors were silently swallowed which makes things
exceptionally hard to debug.

This PR only changes the monitoring around this query - **it doesn't
change the underlying behavior.**

**Note**: I spent a while reading up on how to communicate partial
errors in our graphql API, but it seems like there isn't a clear
consensus.
- graph-gophers/graphql-go#414
- https://spec.graphql.org/October2021/#sel-HAHlBNJDPEBAAADFBU-_D

According to the above, it seems like ideally we should be returning
`null` in the graphQL for permission sync jobs that have an error.
However, when I tried to do this I begain seeing nil panics all over the
place. It's hard for me to know if the underlying library supports this
or if we are just holding it wrong. 😥

## Test plan

Unit tests

## Changelog

- Our graphqlAPI now logs and traces any non-not-found errors that occur
when fetching permission sync jobs (as opposed to being silently
swallowed).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants