How should pointers to standard types be handled when no `Resolve` is defined?

bbuck commented 8 years ago

I know that if I had a struct like this:

type User struct {
        Username *string `json:"username"`
        Age      *int    `json:"age"`
}

Then I could simply define a type with a field called "username" and a type of graphql.String and then another field "age" with a type of graphql.Int and the fields would "auto-resolve". However, I've been defining GraphQL types for AWS API return values and AWS uses a lot of pointers in their structs. For example their IDs/Names are an 'aws.String' which is basically *string so the pointer address is returned in the GraphQL response. Obviously this is not what is desired, but it's odd to have this all over the place:

// ...
Resolve: func(params graphql.ResolveParams) (interface{}, error) {
        source := params.Source.(*User)

        return *source.Username, nil
}

// ...
Resolve: func(params graphql.ResolveParams) (interface{}, error) {
        source := params.Source.(*User)

        return *source.Age, nil
}

Do you think the standard Scalar types should support pointers as well? Or should there be a wrapper type like Type: graphql.NewPointer(...)?

bbuck commented 8 years ago

I think it'd be best to make it a declaration on the type of some sort, like graphql.NewPointer(graphql.Int) which we could then do something like:

refValue := reflect.ValueOf(field)
fieldValue := refValue.Elem()
return coerceInt(fieldValue)

(Granted, tons of a additional testing)

If this seems like a good approach I'll get started implementing it.

augustoroman commented 8 years ago

The current API already has graphql.NotNull(...type...), so adding a pointer seems redundant. If you have a pointer and return it directly, it should either be returned as null (for a nil pointer) or the value (for a non-nil pointer). graphql.NotNull should enforce that the pointer is non-nil.

bbuck commented 8 years ago

The current behavior for non-nil pointers is to serialize their memory address. Not the value like I'm talking about which would make it a non-redundant feature.

On Sunday, January 24, 2016, Augusto Roman notifications@github.com wrote:

The current API already has graphql.NotNull(...type...), so adding a pointer seems redundant. If you have a pointer and return it directly, it should either be returned as null (for a nil pointer) or the value (for a non-nil pointer). graphql.NotNull should enforce that the pointer is non-nil.

— Reply to this email directly or view it on GitHub https://github.com/graphql-go/graphql/issues/101#issuecomment-174355593.

augustoroman commented 8 years ago

Sorry -- I understand that it's not working correctly right now, however I don't think adding a "pointer" type to the API is the right solution. Rather, the standard Scalar types should automatically dereference non-nil pointers without you having to specify anything in the API, IMO.

bbuck commented 8 years ago

This is what I was looking for. I figure the same thing but wanted to seek some additional opinions from the community before I work on a PR

On Sunday, January 24, 2016, Augusto Roman notifications@github.com wrote:

Sorry -- I understand that it's not working correctly right now, however I don't think adding a "pointer" type to the API is the right solution. Rather, the standard Scalar types should automatically dereference non-nil pointers without you having to specify anything in the API, IMO.

— Reply to this email directly or view it on GitHub https://github.com/graphql-go/graphql/issues/101#issuecomment-174396718.

sogko commented 8 years ago

@augustoroman @bbuck I think that would probably be a good enhancement to the library. But we might want to consider if this needed to handled by the library or let the user of the library to implement a custom Scalar Type. There are already an API to do that.

Discussions/ PRs would be greatly welcomed 👍🏻

bbuck commented 8 years ago

I still stick with my suggested implementation concept of having it be a wrapper type like List for values. Otherwise we end up with a significant amount of pollution in the already large coerce funcs. Pointers to basic types don't seem to be all that common (in code I write, and in libraries I use). But I encountered an abnormally high number of *string values in the AWS GO SDK which is what caused my frustration. I propose the graphql.NewPointer(graphql.Int) because then using reflect to unwrap the pointer doesn't have to care about type and can immediately forward to the Scalar for the type desired.

I'll spin up a PR as soon as I can for it.

bbuck commented 8 years ago

@sogko

After completing a PR to fix this issue, I think allowing custom scalar types would be quite simple if we grouped the logic for evaluation into the type itself. Right now the logic is built into executor.go but if we created an interface (bear with names, I'm just getting the idea across)

type Executer interface {
        Execute(*ExecutionContext, Type, []*ast.Field, ResolveInfo, interface{}) interface{}
}

Then users can easily supply their own Scalar type by just defining it and using like any other Type value. Of course, you'd want to group all the methods into a Type or some other named interface (I would guess) to prevent assignment of values that don't fit any part of the requirements. For types like Int coercion can simple be their Execute then the logic in Executor becomes much simpler: return returnType.Execute(...) or something else relatively similar.

I can look into working up a PR to make types more dynamic.

sogko commented 8 years ago

Updating this issue with comments that I added to PR #114 ....

Currently I have reservations about introducing a new type to graphql-go that goes beyond the current published spec. GraphQL is still very much in its early infancy and things does get change pretty quickly (we barely have enough time to achieve parity with graphql-js at the moment lol).

I think currently #101 fits the use case for a user-defined Scalar type. This means that the user can create a new Scalar type named Pointer through existing API graphql.NewScalar(), similar how built-in types like Float, String and Int are defined in the library. (Refer to: https://github.com/graphql-go/graphql/blob/master/scalars.go)

If anyone need a complete example of how to implement a custom scalar type using graphql.NewScalar(), I can spend some time writing that a gist for it.

Thanks again!

Edit: I'm not sure if you have already tried the graphql.NewScalar() approach. If you have already, maybe you could share issues and obstacles that you had faced when trying to implement the Pointer type.

... Cheers! 👍🏻

bbuck commented 8 years ago

@sogko My initial concern with the NewScalar() is the same now as it was before. Go is not JavaScript so sticking to only matching the JS implementation is doomed to introduce oddities (52-bit numbers ring a bell?). JavaScript doesn't have pointers nor an easy way to simulate yet Go does. Given that Go has pointers it's likely that projects will begin sharing the same NewScalar Pointer definition as it's required in their projects with the Go implementation that has the concept of a Pointer.

Because Go is different, and has this unique aspect to the language (and a way to handle it) I think it makes perfect sense to a Pointer type to exist within the Go port of GraphQL. Just like I'd expect any JavaScript oddities to be handled by the JS implementation but not be present in the Go implementation. Yea, it's off spec too, but again. The spec is language agnostic and we're ultimately weighing whether we support a native language feature in our native language implementation and I think in that case it makes sense.

sogko commented 8 years ago

Hi @bbuck

Thanks for bringing up those points for discussion, I do think that your concerns are valid 😃

I agree with you that since this library is written on Go, it should take advantage of the language and its capabilities fully 👍🏻

I failed to elaborate previously why I had reservations about including Pointers type as part of the built-in library.

Consider this: Consider this: while it is true that graphql-go enables you to write a GraphQL server in Go, GraphQL in itself still "communicates" to its clients in JSON, for both input and output.

1: Let's talk in JSON, everybody =) Firstly, this means that you might have clients written by other developers in other languages, be it JS on a browser, or a C# client application querying for data or even an iOS app written in ObjC or Swift that mutates data on your GraphQL. As long as the clients speaks the same JSON language, all is goooood 😃. To cater for clients written in different language, running on different platforms, naturally GraphQL had to compromise and to adopt the "lowest-denominator" approach. This is reflected in how the GraphQL was conceptualised.

(Side note: The 52-bit truncation for Int type is a lax implementation; strictly, Int should be of 32-bit value. This implementation was ported from graphql-js for parity but should be updated)

2: Clients ideally should not worry about platform-specifics of the GraphQL server Second, with graphql-go, we also want to be able to say that, if a developer has written a GraphQL client in Ruby, for example, that "talks" to a GraphQL server that was written in Python, he/she can easily use the same client to talk to another GraphQL server that was written with graphql-go (of course, given that both GraphQL server has the same schema, just that the schema and its implementation are written in different languages).

As such, we have to be mindful about introducing Go-specific features into the built-in Type System.

Keeping those points above in mind, it's natural to standby the position that platform specific features/quirks should be handled in user-defined custom types. This is concurred in how the spec was written.

3: Pointer type Talking specifically on having a Pointer type, naturally clients should not worry itself if a field-value is a pointer or reference to the actual value. The client intuitively would be more interested in the actual value that the pointer. For that, I suggest that implemented GraphQL endpoint should be the one dealing with resolving pointers into its actual value. What this means is that instead of creating a custom type for Pointer or adding Pointer to the type system, two things can be done:

GraphQL server implementor takes care of resolving pointers by implementing the Resolve() func
graphql-go handles this automatically in its default resolve function i.e. DefaultResolveFn. This helps with the need to write custom Resolve() func.

I am not suggesting that we can't make full use of Go's concurrency model to implement the internals of graphql-go, and to guide us when we design the library APIs for usability. But conforming to the specs in areas such as Query Language, Type System, Serialisation and Response Formats etc, would help in the long run for both GraphQL and Go communities. In addition to that, it helps to encourage adoption of the library and source contribution, which in turn ideally would result in a better library for all.

In my personal humble opinion: It is too early to fork off the published GraphQL spec at this time, since every thing is still new and things are bound to change. I can just imagine the chaos in maintenance lol 🔥 😅

I hope that I managed to add useful and valuable points into this discussion, I would encourage discussion from others as well!

Cheers guys! 🍻😃

Edit: I hope that I don't come across as a specs zealot lol, I'm trying to contribute to this project by expressing my opinions regarding what I believe is best for it in the long run, given the information at hand at this moment, as every one of you do. Healthy discussion about issues like this is good,

Useful references:

Re: how to handle Int bigger than 32-bits:

Numeric integer values larger than 32‐bit should either use String or a custom‐defined Scalar type, as not all platforms and transports support encoding integer numbers larger than 32‐bit. [http://facebook.github.io/graphql/#sec-Int]

Re: The ability to write custom Scalar types, for e.g. Time, Int64, and in this case Pointers

GraphQL provides a number of built‐in scalars, but type systems can add additional scalars with semantic meaning. For example, a GraphQL system could define a scalar called Time which, while serialized as a string, promises to conform to ISO‐8601. When querying a field of type Time, you can then rely on the ability to parse the result with an ISO‐8601 parser and use a client‐specific primitive for time. Another example of a potentially useful custom scalar is Url, which serializes as a string, but is guaranteed by the server to be a valid URL. http://facebook.github.io/graphql/#sec-Scalars

andrewscarani commented 7 years ago

@sogko @bbuck Any thoughts on changing how graphql.String maps to struct implementations in future versions of this library?

This makes more sense to me from a Go perspective (and seems to align more with how other libraries work): graphql.String -> *string graphql.NotNull(graphql.String) -> string

graphql-go / graphql

How should pointers to standard types be handled when no `Resolve` is defined? #101