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

implement sql.Scanner and driver.Value for graphql.ID type #40

Closed
wants to merge 2 commits into from

Conversation

nicksrandall
Copy link
Member

This makes graphql.ID type implement sql.Scanner and driver.Valuer. This simplifies the data structures that I'm using so that they can implement the types that graphql likes but can also be marshaled into the DB.

@nicksrandall
Copy link
Member Author

@neelance @bsr203 What do you think of this small change? Do you think it would be useful to others?

@bsr203
Copy link

bsr203 commented Dec 8, 2016

@nicksrandall sorry, I don't use sql backend but could see it may useful for others.

@neelance
Copy link
Collaborator

neelance commented Dec 9, 2016

What about this: Have your own type ID string and give it those methods. Use it in your struct. Then cast to graphql.ID in the resolver of the id field.

@nicksrandall
Copy link
Member Author

Having to always cast to graphql.ID is what I'm trying to avoid. Currently my structs use strings for ID prop and I have to cast to graphql.ID in resolvers. I use strings for the DB. This change would allow me to not have to worry about casting in all the right places.

This also allows me to use the same struct for resolvers as input type because input type requires ID type (to be idiomatic).

I imagine that anybody using this lib with a SQL backend would find this helpful. Am I wrong?

What are your concerns with adding this?

@dmitshur
Copy link

dmitshur commented Dec 9, 2016

I imagine that anybody using this lib with a SQL backend would find this helpful. Am I wrong?

Would everyone who uses this library and a SQL backend want this exact implementation, or would their needs differ slightly?

@nicksrandall
Copy link
Member Author

nicksrandall commented Dec 9, 2016

Anybody using the sql package in the standard lib would need this exact implementation.

EDIT: I can't say that definitively. My guess is that this would cover a large majority of people using the standard sql package.

@neelance
Copy link
Collaborator

neelance commented Dec 9, 2016

I still think that it is beneficial to have a clear separation between resolver API and underlying data structures. The resolvers are a view/projection on the underlying data. To me it feels like your change would mix this up a bit.

On the other hand I see that there currently is a strict 1-to-1 coupling between the GraphQL type ID and the Go type graphql.ID. This is inflexible and contradicts the freedom you have with resolver types.

One option would be to accept any named string type as a GraphQL ID. But that would also allow a pure string, which I dislike.

There might be some good solution around custom resolvers. You could use any type as the result type for ID, but that type would need to provide a method to turn the value into something GraphQL understands. The type would also check at startup if it is compatible with the given GraphQL type. The same method could be used for Time and for custom behavior like #28 and #17. I have to think about it some more.

@nicksrandall
Copy link
Member Author

What I have ended up doing is adding my own custom scalar type ID that is a named string that implements the sql methods that I need.

@neelance neelance mentioned this pull request Dec 18, 2016
@neelance
Copy link
Collaborator

@nicksrandall Please take a look at 2b513d7 and #3 (comment). This should be a proper solution here.

@nicksrandall
Copy link
Member Author

I like it! Thanks @neelance. I close this PR

@nicksrandall nicksrandall deleted the sql branch December 19, 2016 22:34
@binajmen
Copy link

binajmen commented May 13, 2020

@nicksrandall Please take a look at 2b513d7 and #3 (comment). This should be a proper solution here.

@nicksrandall @neelance First of all, thank you for your work!!

Sorry to dig in the past, but could you please explain me how I can apply the proposed solution to my case? I'm using SQL with IDs defined as integers.

As a new gopher, It's not clear where and how I should implement this..

@binajmen
Copy link

binajmen commented May 14, 2020

I ran into the same situation regarding the scalar Time. I solved it at the moment by editing graphql-go/time.go (BUT this should be avoided of course!!):

// Value implements the driver Valuer interface
func (t Time) Value() (driver.Value, error) {
	return t.Time, nil
}

// Scan implements the Scanner interface.
func (t *Time) Scan(src interface{}) error {
	return t.UnmarshalGraphQL(src)
}

This offers me the possibility to use this same struct for GraphQL and GORM:

type Question struct {
	ID       int32  `gorm:"primary_key"`
	Question string `gorm:"not null"`
	...
	CreatedAt graphql.Time
}

However, I'd really like to apply your proposition (using UnmarshalGraphQL I suppose?), but I don't understand how to do so..

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

Successfully merging this pull request may close these issues.

5 participants