sdk-codegen
sdk-codegen copied to clipboard
looker-open-source
[Go SDK] Request fails when the string fields of request body json contains percent sign
Hi developers, Thank you for developing great SDKs.
I found that a request using Go SDK will be failed when the string field of the request body json contains % (percent sign).
(I did not check other languages, but it will be happened)
For example,
{
"some_field": "100%"
}
And I found following discussions in the looker forum. https://community.looker.com/looker-api-77/percent-sign-in-the-json-in-api-request-is-rejected-15410
I tried adding Accept and Content-Type headers explicitly as follows (at here) and such requests succeeded.
// create new request
req, err := http.NewRequest(method, u, bytes.NewBufferString(bodyString))
if err != nil {
return err
}
// add headers explicitly
req.Header.Add("Accept", `application/json`)
req.Header.Add("Content-Type", `application/json`)
// set query params
setQuery(req.URL, reqPars)
I would like to create PR for this problem if it is OK.
Hey @hirosassa again! Thanks for bringing this up. Let me look into this a little further and get back to you.
My gut feeling is only Content-Type header is needed for only JSON request bodies.
Also when you have a chance, please update your Go SDK to latest. We've refactored the Go SDK a bit, and added some tracking for internal purposes.
@jeremytchang Hey! Thanks for your comment! I tried latest provider in the PR below and it is reproduced (the request is failed) without headers: https://github.com/hirosassa/terraform-provider-looker/pull/27
My gut feeling is only Content-Type header is needed for only JSON request bodies.
Yeah, I agree with this. We have to switch by type of request body.
Hey all! Thanks @hirosassa for highlighting this issue, I’ve been having a similar problem regarding setting request headers. When querying the User method, I am getting back HTML if the user doesn’t exist. For example,
response error. status=404 Not Found. error=<!DOCTYPE html>
<html>
<head>
<title>Looker Not Found (404)</title>
…
…
</html>
When I set the Accept header to application/json, the response is as expected. For error cases, it seems like the Looker API defaults to application/json for most requests but there are cases where it needs an Accept header. I’ve tried setting Content-Type as application/json instead but that doesn’t do the trick unfortunately.
There are some SDK methods that have multiple result_formats not just application/json , for example the RunQuery method. We could inspect the API spec and if more than one result_format is supported, we either add an additional argument to pass the header or we set the header as part of the request struct. If a header is not provided then we can default to application/json.
I can make a PR to resolve this issue with this approach? What do you think?
@jessicasomaiya Thanks for your report! FYI, the following commit is the sample of implementation to solve this issue. I'm looking forward to your PR!
https://github.com/hirosassa/sdk-codegen/commit/6ef4a4149ee4f4eff2dae52489156f7c714876c5
Hey all. After starting the implementation of the above https://github.com/looker-open-source/sdk-codegen/issues/1075#issuecomment-1243838576, it is a major change to add a new headerOptions arg to each method and is not backwards compatible with older SDK versions - so I'd rather support your change @hirosassa to add the application/json header as a default for every request.
It could be beneficial to support header options as part of a larger SDK change, or move towards a variadic With option style to support adding further options and replace the ApiSettings argument as it is not being used. We could also add a context arg to each method.
@jeremytchang what do you think about setting the Accept header to default to application/json? This is especially useful for non-2xx responses so the json can be parsed.
I think @jessicasomaiya's implementation. @jeremytchang What do you think?
We need the possibilty to add custom request header per request for tracing purposes. The PR created by @jessicasomaiya looks promising to me. Maybe it needs some finalization, e.g. alignment of indentation pattern. The most important thing is to handle the breaking change of the SDK. I think we have two options:
- Add "...WithHeaders(...)" functions and change existing functions to call them with nil
- Create a new major version of the SDK.
BTW: The *ApiSettings parameter is indeed confusing and could be removed in my eyes but this is out of scope I guess.
@hirosassa @jeremytchang is there a chance to push this matter forward somehow?
(I know this is becoming a bit OT, however, since this solution solves the original issue, I continued here with discussion.)
Thanks everyone for their input and discussion. It's great seeing active users of Looker's Go SDK! 🎉 And I'd love to learn more about your use cases!
Currently, this thread has grown in scope past the content-type header issue. I have a fix for the content-type header here: https://github.com/looker-open-source/sdk-codegen/pull/1283.
For those who don't know, this SDK was created to enable Go users and strives to follow the our typescript sdk. In that regard, to set custom headers we'll follow precedent here: https://github.com/looker-open-source/sdk-codegen/blob/main/packages/sdk-rtl/src/transport.ts#L342 and add headers to options in a separate change. This will allow you to set custom headers per request, including Accept headers. The Accept headers change that was posted in this thread is a good first attempt, but it's a bigger sdk generation change that we'd prefer to avoid for now.
As a follow up, a PR for custom headers in Go SDK. https://github.com/looker-open-source/sdk-codegen/pull/1288 @jessicasomaiya @JanReimerD