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

Query and Mutation methods ignore query content when it's wrapped in an interface #117

Open
JonasDoe opened this issue Dec 5, 2023 · 3 comments

Comments

@JonasDoe
Copy link

JonasDoe commented Dec 5, 2023

The following code won't work

var query any

// imagine a switch-case pattern here which dynamically decides what the query actually is
query = struct {
	Hero struct {
		Name  graphql.String
		Droid struct {
			PrimaryFunction graphql.String
		} `graphql:"... on Droid"`
		Human struct {
			Height graphql.Float
		} `graphql:"... on Human"`
	} `graphql:"hero(episode: \"JEDI\")"`
}{}

err := a.client.Query(context.Background(), &query, nil)

The request body will look like {"query":""}. I set a query string in the graphql tag, it would appear here - but the requested fields etc. would still be missing.
The problem is probably that the type of query is now interface{} | struct {...} (or interface{} | HeroQuery with an explicit type declaration), and only type interface{} is regarded.

Edit: I had a look into the code. Seems like you only process reflect.Ptr, reflect.Type and reflect.Struct in the function writeQuery. The kind here is (pointer to) interface{}. Reflection is such a pain in Go. :/

@JonasDoe JonasDoe changed the title Mutation and Query structs are parsed unreliable Mutation and Query structs are parsed unreliable/depend on how they are declared Dec 5, 2023
@JonasDoe JonasDoe changed the title Mutation and Query structs are parsed unreliable/depend on how they are declared Mutation and Query structs are parsed unreliably/depend on how they are declared Dec 5, 2023
@dmitshur
Copy link
Member

dmitshur commented Dec 5, 2023

Thanks for reporting this. I'll take a look.

@dmitshur dmitshur self-assigned this Dec 5, 2023
@dmitshur
Copy link
Member

dmitshur commented Dec 6, 2023

The documentation for Query currently includes:

q should be a pointer to struct that corresponds to the GraphQL schema.

To make the snippet you've shown work as is, that would need to change to something like this:

q should be a pointer to struct that corresponds to the GraphQL schema,
or a pointer to an interface holding a struct that corresponds to the GraphQL schema.

That seems to make the API more complex.

But maybe it's possible to make this use-case work without needing to do that, like so:

 var query any

 // imagine a switch-case pattern here which dynamically decides what the query actually is
-query = struct {
+query = &struct {
 	Hero struct {
 		Name  graphql.String
 		Droid struct {
 			PrimaryFunction graphql.String
 		} `graphql:"... on Droid"`
 		Human struct {
 			Height graphql.Float
 		} `graphql:"... on Human"`
 	} `graphql:"hero(episode: \"JEDI\")"`
 }{}
 
-err := a.client.Query(context.Background(), &query, nil)
+err := a.client.Query(context.Background(), query, nil)

The difference is that the query (type any, i.e., empty interface) is being passed in directly to Query, but the value its being assigned is a pointer to a struct. Do you think that could work for you?

TestClient_Query_Issue117

For reference, here's a test I used to check that the above should work:

func TestClient_Query_Issue117(t *testing.T) {
	mux := http.NewServeMux()
	mux.HandleFunc("/graphql", func(w http.ResponseWriter, req *http.Request) {
		body := mustRead(req.Body)
		if got, want := body, `{"query":"{user{name}}"}`+"\n"; got != want {
			t.Errorf("got body: %v, want %v", got, want)
		}
		w.Header().Set("Content-Type", "application/json")
		mustWrite(w, `{"data": {"user": {"name": "Gopher"}}}`)
	})
	client := graphql.NewClient("/graphql", &http.Client{Transport: localRoundTripper{handler: mux}})

	var q any = &struct {
		User struct {
			Name string
		}
	}{}
	err := client.Query(context.Background(), q, map[string]any{})
	if err != nil {
		t.Fatal(err)
	}
	if got, want := reflect.ValueOf(q).Elem().FieldByName("User").FieldByName("Name").Interface().(string), "Gopher"; got != want {
		t.Errorf("got q.User.Name: %q, want: %q", got, want)
	}
}

@dmitshur dmitshur removed their assignment Dec 6, 2023
@dmitshur dmitshur changed the title Mutation and Query structs are parsed unreliably/depend on how they are declared Query and Mutation methods ignore query content when it's wrapped in an interface Dec 6, 2023
@JonasDoe
Copy link
Author

JonasDoe commented Dec 6, 2023

Hey, thanks for the fast response! That would surely work for me.
Regarding the underlying questions - whether the API would become more complex -, I'ld slightly disagree, though. The documentation might become longer b/c the list of supported cases increases, but the API itself wouldn't change a bit and just becomes more robust. It's not like it would introduce more requirements - quite the opposite, it becomes more lenient about the passed type, and produces (for my taste) less suprising behavior. But it's your call ofc., personally I'm fine with working with pointers! :)

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

No branches or pull requests

2 participants