-
Notifications
You must be signed in to change notification settings - Fork 266
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
Add GraphQL endpoint #228
Add GraphQL endpoint #228
Conversation
Thanks for the PR! I'm a bit busy this week but I'll try to review it in the next couple of days. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #228 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 44 45 +1
Lines 1930 1994 +64
=========================================
+ Hits 1930 1994 +64
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, look god overall, just a couple of questions/nits.
graphql.go
Outdated
err := s.client.Post("graphql.json", data, &gr) | ||
// internal attempts count towards outer total | ||
attempts += s.client.attempts | ||
s.client.attempts = attempts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get why we want line 84 but why do you need copy the total attempts back to the client?
attempts
isn't even used for anything, it's just incremented and the tests use it to see if it matches retries. I have no context but the original PR and that already had it like this which is odd (unless it was only ever meant to be a helper for the tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, it was only for the tests.
So it is less confusing, I moved attempts to a return value of the method. I'm not sure anyone will have a use for the value, but it allows the tests to run without some hijacking of other variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure anyone will have a use for the value
- I agree so let's not inconvenience everyone just for the sake of the tests. Can you remove attempts as the return value?
If we really care about testing the retries then we could pass out own httpClient and count the number of times a UR is requested. Or even simpler, use httpmock.GetCallCountInfo() to get the number of calls for each URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, GetCallCountInfo
worked a treat
graphql.go
Outdated
err := s.client.Post("graphql.json", data, &gr) | ||
// internal attempts count towards outer total | ||
attempts += s.client.attempts | ||
s.client.attempts = attempts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure anyone will have a use for the value
- I agree so let's not inconvenience everyone just for the sake of the tests. Can you remove attempts as the return value?
If we really care about testing the retries then we could pass out own httpClient and count the number of times a UR is requested. Or even simpler, use httpmock.GetCallCountInfo() to get the number of calls for each URL.
graphql.go
Outdated
|
||
// Query creates a graphql query against the Shopify API | ||
// the "data" portion of the response is unmarshalled into resp | ||
// Returns the number of attempts required to perform the request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth mentioning that Query()
will retry throttled requests but not e..g any other type of 500 error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep the concept of retry more defined, I added this comment to the WithRetry
method
// WithRetry sets the number of times a request will be retried if a rate limit or service unavailable error is returned.
// Rate limiting can be either REST API limits or GraphQL Cost limits
graphql.go
Outdated
|
||
err := s.client.Post("graphql.json", data, &gr) | ||
// internal attempts count towards outer total | ||
attempts += s.client.attempts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the code again and I think this is a problem.
This is not threadsafe when multiple goroutines use the same client and make requests at the same time.
The state attempts
part of the client is shared across requests and can't be relied on for truthfully telling the number of attempts made.
This should be something simple to show in a test (have the mock responder wait a sec for each request and respond with "throttled", then kick off 10 requests, all with the same client, then check attempts
, should be the combined number of requests or so).
I think I'd want to remove attempts
altogether and for instance have an alternate/secondary version of Post that returns the number of attempts.
This might be getting a bit much for this PR, to simplify you could, as first step, replace the attempts+=...
with attempts += 1
andthen we can figure out later if this needs improving.
An alternate option would be to make the retry in doGetHeaders()
smarter and possibly make it aware of GraphQL. But again, that's a bigger effort and I'd be fine with the simplified +=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with the += 1
because I'm trying to ship something that depends on this, but I'll try to loop back and do a more proper management in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
@weirdian2k3 Hello Ian, Thank you for creating this endpoint. However, the example above is not working in my case. I get the error "unacceptable". Maybe you stumbled across a similar error?
|
Same for me, even for simple examples like |
@nickfthedev i think i found it. You should set an explicit version with |
Thanks for pointing this out @johannessteu. This is a bug #314. |
This is a re-introduction of an old PR by another person, trimmed down to just the GraphQL Client and a typo-fix
#118
I'm not sure why the original one got closed, but I've succeessfully used it for making mutations with queries