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

Retry duplicates query parameters in v3.0.0-alpha.6 #938

Closed
nf-brentsaner opened this issue Jan 3, 2025 · 4 comments · Fixed by #942
Closed

Retry duplicates query parameters in v3.0.0-alpha.6 #938

nf-brentsaner opened this issue Jan 3, 2025 · 4 comments · Fixed by #942
Assignees
Labels
bug v3 For resty v3

Comments

@nf-brentsaner
Copy link

nf-brentsaner commented Jan 3, 2025

This occurs on v3.

POC:

package main

import (
	"context"
	"errors"
	"log"
	"net/http"

	"resty.dev/v3"
)

const (
	maxRetries int = 5
)

var (
	curRetry int
	client   *resty.Client = resty.New().
		SetBaseURL("http://localhost:8080/").
		SetDebug(true).
		SetRetryCount(maxRetries + 1)
)

func httpHandle(resp http.ResponseWriter, req *http.Request) {

	if curRetry < maxRetries {
		resp.WriteHeader(http.StatusTooManyRequests)
		curRetry++
	}

	// spew.Dump(req)
	_, _ = resp.Write([]byte(req.URL.String() + "\n"))
}

func main() {
	var err error
	var srv *http.Server
	var mux *http.ServeMux
	var req *resty.Request
	var resp *resty.Response

	mux = http.NewServeMux()
	mux.HandleFunc("/", httpHandle)

	srv = &http.Server{
		Addr:    ":8080",
		Handler: mux,
	}

	go func() {
		var hErr error
		if hErr = srv.ListenAndServe(); hErr != nil {
			if !errors.Is(err, http.ErrServerClosed) {
				log.Println(err)
			}
		}
	}()

	req = client.R()
	req.SetQueryParamsFromValues(map[string][]string{
		"foo": []string{
			"baz",
			"bar",
			"bar",
		},
	})
	req.Method = http.MethodGet
	req.URL = ""
	if resp, err = req.Send(); err != nil {
		log.Panicln(err)
	}

	if err = srv.Shutdown(context.Background()); err != nil {
		log.Panicln(err)
	}

	_ = resp
}
$ go run main.go 2>&1 | grep -E '^GET\s+'
GET  /?foo=baz&foo=bar&foo=bar  HTTP/1.1
GET  /?foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar  HTTP/1.1
GET  /?foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar  HTTP/1.1
GET  /?foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar  HTTP/1.1
GET  /?foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar  HTTP/1.1
GET  /?foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar&foo=baz&foo=bar&foo=bar  HTTP/1.1
@nf-brentsaner
Copy link
Author

nf-brentsaner commented Jan 3, 2025

This even still occurs with the following RetryHookFunc:

func retryHook(resp *resty.Response, err error) {

	var changed bool
	var params map[string][]string

	if resp.Request.QueryParams != nil {
		params = resp.Request.QueryParams
		for k, v := range params {
			if len(v) > 1 {
				changed = true
				slices.Sort(params[k])
				params[k] = slices.Compact(params[k])
			}
		}
		if changed {
			// Either of these makes no difference in behavior.
			//resp.Request.QueryParams = params
			resp.Request.SetQueryParamsFromValues(params)
		}
		// resp.Request.SetQueryParams()
	}

}

and also still occurs regardless of if (*resty.Request).Send(), (*resty.Request).Get(""), or (*resty.Request).Execute(resty.MethodGet, "") is used.

@jeevatkm jeevatkm self-assigned this Jan 4, 2025
@jeevatkm
Copy link
Member

jeevatkm commented Jan 4, 2025

@nf-brentsaner Thanks for reaching out.

I have looked at it and tried the above code snippet, which does not help me to replicate the issue. Then I have created a test case to try it on v2 and v3.

func TestRetryQueryParamsGH938(t *testing.T) {
	ts := createGetServer(t)
	defer ts.Close()

	c := New().
		SetBaseURL(ts.URL).
		SetRetryCount(5).
		AddRetryCondition(
			func(r *Response, _ error) bool {
				fmt.Println(r.Request.URL)
				fmt.Println(r.Request.RawRequest.URL.String())
				return true
			},
		)

	_, _ = c.R().
		SetQueryParamsFromValues(map[string][]string{
			"foo": {
				"baz",
				"bar",
				"bar",
			},
		}).
		Get("/set-retrycount-test")
}

I was able to replicate the issue in v3.0.0-alpha.6, but not in v2.16.2. Can you double-check it once?

@nf-brentsaner
Copy link
Author

Oddly... I actually switched to/tried v3 because I was hoping this issue wasn't present in it.

But you're right, confirmed! I now cannot reproduce in v2, though I could have sworn I originally was. I'll edit the original message to indicate it's v3 only.

@jeevatkm jeevatkm added bug v3 For resty v3 and removed analysis labels Jan 4, 2025
@jeevatkm jeevatkm added this to the v3.0.0 Milestone milestone Jan 4, 2025
@jeevatkm
Copy link
Member

jeevatkm commented Jan 4, 2025

@nf-brentsaner Thanks for the confirmation. I will look into the v3 bug.

@jeevatkm jeevatkm changed the title Retry duplicates query parameters Retry duplicates query parameters on v3.0.0-alpha.6 Jan 4, 2025
@jeevatkm jeevatkm changed the title Retry duplicates query parameters on v3.0.0-alpha.6 Retry duplicates query parameters in v3.0.0-alpha.6 Jan 4, 2025
@jeevatkm jeevatkm linked a pull request Jan 5, 2025 that will close this issue
@jeevatkm jeevatkm closed this as completed Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug v3 For resty v3
Development

Successfully merging a pull request may close this issue.

2 participants