-
Notifications
You must be signed in to change notification settings - Fork 710
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
Collecting Inputs for Resty v2.0 and Plan the release #166
Comments
I've used Resty for two small CLI projects that interact with 3rd party REST services. Resty has done everything I wanted it to do and saved me a lot of time. The Resty 1.6 API is adequate for me, but my needs are rather typical so far. The only hurdles I had to overcome were trying to understand how to use Resty's features. Showing a typical usage example with error handling would have helped me better understand the Resty design at a glance. For example, here's how I typically use Resty when interacting with the Quizlet API: // GetSets returns all user sets in Quizlet.
func (c *Config) GetSets() ([]Set, error) {
var sets []Set
var e QuizletError
uri := fmt.Sprintf("%s/users/%s/sets", apiBaseURL, c.Username)
_, err := resty.SetDebug(c.Debug).R().
SetHeader("Accept", "application/json").
SetAuthToken(c.AuthToken).
SetResult(&sets).
SetError(&e).
Get(uri)
if err != nil {
return nil, err
}
if e.Code != 0 {
return nil, e
}
return sets, err
}
// QuizletError represents an error response from the Quizlet API.
type QuizletError struct {
Code int `json:"http_code"`
QError string `json:"error"`
Title string `json:"error_title"`
Description string `json:"error_description"`
ValidationErrors []string `json:"validation_errors"`
}
// Error implements the error interface.
func (e QuizletError) Error() string {
return e.Description
} |
Thank you for your inputs @moorereason. Action Item for v2.0 release:
|
How about having DefaultClient a DefaultTransport with sane defaults for Timeouts? |
Any plans on supporting "net/http2" ? |
@zjjhzx resty already supports |
What are your thoughts on functional options? This can be useful to propagate errors instead of logging(which could be missed) as is the case in |
Another useful feature would be integration with https://github.com/go-playground/form (I prefer this) or https://github.com/google/go-querystring to be able to pass structs for URL encoding. |
@sudo-suhas Thanks for your inputs. I will have a look and get back to you. |
Functional Options Example: |
@sudo-suhas @h7kanna Thanks for your inputs. I have looked into it.
|
I feel error propagation is the most important and that is the reason I prefer functional options. |
This could be solved by using the following interface: type Encoder interface {
Encode(interface{}) (url.Values, error)
} Another thing I wanted to bring up was use of |
|
The |
And once again, thanks for all your great work. I really appreciate it. |
@sudo-suhas Thank you. We had very good discussion. I will think about the design and then we can have a discussion 😄 |
Hi, Not sure if this is feasible, It may be a big API change. Like Thanks for your work on this handy package. |
@h7kanna Thank you, I will have a look on your reference. |
req.SetResult(nil) Above code will cause panic. I was expecting it to undo previous SetResult or just unset the result type and I think it supposed to be. Is this intended? |
@dewitast By default for every request's Result and Error is fresh and applicable to that request only. No reason to supply to |
I recently came across the |
Thanks @sudo-suhas, nice thoughts I will look into it. Yes, suppress and reuse feature should be provided as opt-in. |
Would love to see a built-in mechanism for interacting with NDJSON streams. This can already be done manually, by pulling line-by-line from the raw Response stream and feeding through a JSON decoder, but it would be handy to have it built in. |
@david-l-riley Thanks for your input, I will have a look on NDJSON and get back. |
Can you please support Restful server end feature? |
@topillar Could you please describe in detail? Not sure, just taking a guess. Are you looking for RESTful server side framework? if yes then try out my aah framework |
Yes,I noticed aah just after I sent the request. |
@david-l-riley I have read the NDJSON spec (https://github.com/ndjson/ndjson-spec), it seems each line could be different JSON structure. Not sure how we can generalize this one. Same JSON structure on every line could generalized into resty. Please let me know. |
It's true, in NDJSON basically each line is a complete JSON document that could be a different item, suitable for streaming return values (rather than unmarshaling everything at once) If that's something that's difficult to marshal with the built-in JSON library, that's OK; it can be done reasonably simply just by using a line reader and deserializing each line. |
@david-l-riley Challenge with handling NDJSON response payload is unmarshal data types for different items. One of the approach I could think of handling NDJSON response is to have For example:
Does it make sense? Do you have any other approach? |
Change: pass error returned by Motivation: It is possible for the underlying http.Client to have a round-tripper that does additional HTTP request(s) before issuing an actual user's HTTP request. Example of such round-trippers is |
@pborzenkov Thanks for your inputs. I will think about the design around it. I have added to action items here #167 |
So, coming back to the NDJSON: I've been doing some playing around with NDJSON using the current Resty version. It generally works reasonably well if you attach a Other than that, a way of attaching streaming decoders (of all sorts) to the Body without losing the other useful parsing/handling behaviors of Resty's |
There are some APIs (i. e. https://developers.google.com/drive/api/v3), that return binary content on success and JSON on error. On the one hand, it is not appropriate to read file content into buffer; on the other, error response should be parsed in order to manage retries. Resty client has SetDoNotParseResponse() method, which allows to copy download content from HTTP response directly, but it is very inconvenient to control error response parsing and retries outside the client. I suggest to add something like SetDoParseResponseOnlyOnError() method to Resty. It would be really helpful for such downloads. |
I have noticed that Resty unfortunately has poor support for retries of file uploads. It is possible to pass io.Reader to Request.SetBody(), but if it is a file, it will close after the first unsuccessful attempt. My suggestion is following:
|
@neganovalexey I think suggested method should work in conjunction with method
@neganovalexey I remember resty read and closes the file for multi-part content-type, however I do not remember it closes the Also FYI |
@jeevatkm thank you for your response!
Golang standard HTTP client closes request body if it implements io.Closer. So if an opened file is passed to Resty's Request.SetBody(), and retries are configured, they will fail. It is obviously inefficient to reopen file on each attempt. Moreover, if even io.ReadSeeker does not implement io.Closer (i. e. bytes.Reader), retries will still incorrect, because Resty does not perform Seek() before request. I understand that it is possible to write own methods in order to fix this, but I think it is not convenient and may lead to bugs. |
I have experience using Resty with servers that returns 200 and error in body. Now I handling it like this: type MaybeError interface {
Error() error
}
type SomeResponse struct {
// some fields here
ErrorText *string `json:"error"`
}
func (sr *SomeResponse) Error() error {
if sr.Error != nil {
return errors.New(*sr.ErrorText)
}
return nil
}
func handleMaybeError(c *resty.Client, r *resty.Response) error {
if rerr, ok := r.Result().(MaybeError); ok {
return r.Error()
}
return nil
} and using I know that returning error response with successive code is very bad practice but it's 3rd party service and I can't change it. I suggest to add something like |
@xakep666 Thank you for sharing your experience in 3rd party service. I understand your description I think it may not be a candidate for generalized approach within a Resty. I can quickly think of couple of scenario's why it won't- Scenario 1: Field Name As you have described field name as Scenario 2: Response Body Structure In your example and you're dealing with APIs provide following structure. {
"error" : "error message"
} However, in reality it might have more combinations. [{
"error" : "error message 1"
},
{
"error" : "error message 2"
}] OR {
"message": "some description",
"errors": [{
"error" : "error message 1",
"fieldName": "somefield1"
},
{
"error" : "error message 2",
"fieldName": "somefield2"
}]
} OR ... So its very difficult to standardized. If you ask me, You handling above cases very effectively per APIs provider or vendor. |
@jeevatkm Unfortunately, returning a non-nil error from OnAfterResponse callback combined with configured retries results in unnecessary retries (e.g. Backoff do retries unconditionally if client.execute() returned a non-nil error). This is something we are facing too. One way to fix it, as I previously suggested, is to pass this error into retry condition callback and let it figure out whether or not it is actually a retry-able error. |
@pborzenkov Thank you for your input, I will have a look and get back to you. |
@pborzenkov I have cross checked it. It seems I have already added your suggestion as action items #167 for |
Hi @jeevatkm, I am newbee on Resty, I have use case to track http call. How to integrate resty with zipkin / opentracing / jaeger? |
@incubus8 Resty does not have direct reference to any of the tracing libraries you have mentioned. Since you could implement Make use of Resty request and response middleware for tracing/stats. Also resty does capture the request time by default. |
I'm closing this input thread, gonna start the v2 development. So we could discuss in the respective issues I will be creating. Thank you all for the great inputs and discussion. |
The goal of this thread to collect feedback's from
resty
users andplan v2.0.0
release. Action items get recorded here #167.Share your resty experiences for upcoming
v2.0.0
The text was updated successfully, but these errors were encountered: