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

Make PathParams public #476

Merged
merged 1 commit into from
Oct 31, 2021
Merged

Conversation

lavoiesl
Copy link
Contributor

This PR makes the PathParams property public, same as QueryParam, FormData, Header, etc.

I don't really see the value in having it private and it prevents me from adding a middleware to capture all the existing parameters.

Example:

func InjectContextMiddleware(service string) resty.RequestMiddleware {
	return func(c *resty.Client, req *resty.Request) error {
		ctx := req.Context()
		ctx = logger.WithFields(ctx, map[string]interface{}{
			"http_service":         service,
			"http_service_host":    c.HostURL,
			"http_request_method":  req.Method,
			"http_request_uri":     req.URL, // URLs should have fairly low cardinality. Users should use PathParams and QueryParams for variable parts
			"http_request_attempt": req.Attempt,
			"http_request_retry":   req.Attempt > 1,
		})

		for k, v := range req.QueryParam {
			if len(v) > 1 {
				ctx = logger.WithField(ctx, "http_request_query_"+k, v)
			} else {
				// most common scenario, only one value; inline the value instead of serializing an array
				ctx = logger.WithField(ctx, "http_request_query_"+k, v[0])
			}
		}

		// Currently not possible
		for k, v := range req.PathParams {
			ctx = logger.WithField(ctx, "http_request_path_"+k, v)
		}

		req.SetContext(ctx)
		return nil
	}
}

Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lavoiesl Thanks for the PR and improvements to Resty.
Currently, Travis CI build is not helping me to validate this PR, do you mind executing tests locally and attach it to the PR?
I will include this PR in v2.7.0.
Of course, I will work on #479 to sort out for the long term.

@lavoiesl
Copy link
Contributor Author

Sure thing 👍

$ go version
go version go1.11.13 darwin/amd64
$ go test ./...
ok  	github.com/go-resty/resty/v2	170.098s

Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lavoiesl Thanks for the test execution info.

@jeevatkm jeevatkm merged commit d837dfc into go-resty:master Oct 31, 2021
@jeevatkm jeevatkm added this to the v2.7.0 Milestone milestone Oct 31, 2021
hurracrane pushed a commit to Shopify/resty that referenced this pull request May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants