fix: Make Go SDK parse response by result type #1021
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@jkaster The TS SDK exhibits the antipattern and returns string for text/json/image content types. https://github.com/looker-open-source/sdk-codegen/blob/main/packages/sdk/src/4.0/methods.ts#L8253 |
Not quite. See the existing tests that verify the image payloads for various SDK methods. The |
|
For more strictly typed languages, you may need to indicate either byte array or string as the return type. For kotlin and swift, I hacked together a solution we've never cleaned up that implements binary payloads via the "streaming" parallel functions for the SDKs. |
This comment has been minimized.
This comment has been minimized.
| return fmt.Errorf("response error. status=%s. error parsing error body", res.Status) | ||
| } | ||
|
|
||
| return fmt.Errorf("response error. status=%s. error=%s", res.Status, string(b)) |
There was a problem hiding this comment.
I am wondering if this could break any code that relies on the string.
There was a problem hiding this comment.
Yeah this could, though consumers reallllllly shouldn't be matching on strings. I'll be implementing better error context in the future so consumers can match on the status code and/or specific errors. #795 (comment)
The possibility of breakage is not great, but while we try to fully support the Go SDK, consider it in beta and we will provide best effort to not break downstream dependencies.
|
This is a major improvement. Can you add a test for content_thumbnail that verifies the PNG and SVG payload? See the TS tests for PNG bytestream verification code. |
This comment has been minimized.
This comment has been minimized.
|
@jkaster Added png/svg integration test. Made use of golang standard library for checking mime type https://pkg.go.dev/net/http#DetectContentType |
This comment has been minimized.
This comment has been minimized.
Currently go sdk model generation doesn't support union type for responses like
RunQuery. So we cannot parse according to Content Type Header. For now, parse
based off model type.
We need to update sdk model generation to use interface{}
for union types, and then use Content Type Header to parse response accordingly.
This will break downstream dependencies.
- Added additional integration tests around queries looks and
dashboards.
- Parse response based of type defined in go sdk models, the result type
- Added unit tests
- Return error body in addition to error status code.
…ing with new 22.4
Go Tests 7 files 7 suites 5m 17s ⏱️ Results for commit 7e07cfa. |
|
@panoskoug Thanks for going out of your way for the approval! |
This comment has been minimized.
This comment has been minimized.
Changes:
string,struct,interface{}.dashboards. This fills out the basic integration test requirements.
Ideally we want the content type header to be the source of truth when parsing the response. However, currently, go sdk model generation doesn't support union type for results, so we cannot parse according to Content Type Header. For now, parse based off result type.
Take for example RunQuery. It returns 4 content type headers
text,application/json,image/png, andimage/jpeg. Result types should bestring,map[string]interface{},byte[]. Right now result type is onlystringhere. Passing in a string type to the decoder means we cannot parse tomap[string]interface{}orbyte[].In this case, it is an antipattern to force the Go SDK caller to cast the
stringresult to what they need, but it at least works.For the future:
We need to update sdk model generation to use interface{} (any) for union types, and then use Content Type Header to parse response accordingly. This will break downstream dependencies. Issue tracking this.