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

Clone or copy of existing client #773

Closed
fabiante opened this issue Feb 2, 2024 · 6 comments · Fixed by #774
Closed

Clone or copy of existing client #773

fabiante opened this issue Feb 2, 2024 · 6 comments · Fixed by #774

Comments

@fabiante
Copy link
Contributor

fabiante commented Feb 2, 2024

I have a use case where my application should have a global HTTP client with sane defaults for tracing, logging and error handling and then some submodules which must apply highly specific settings for only their use case.

For that I would like to copy / clone an existing client which thus inherits all of the clients settings. In other libraries / languages this is also sometimes possible by having a child-parent relationship between clients.

What would be the best way to approach this with resty? Is there a function or common solutions for this use case?

Edit: If this is not a present feature but of interest to people, let's discuss how that could be implemented.

@fabiante
Copy link
Contributor Author

fabiante commented Feb 2, 2024

Potentially I have fallen victim to a blind spot in using pointers: I tend to always use pointers when dealing with struct types. Thus, I also always pass around *restry.Client values.

Wouldn't simply passing resty.Client arround already create a copy of the client those submodules can now freely modify be what I need? I will test this.

@fabiante
Copy link
Contributor Author

fabiante commented Feb 2, 2024

Yeah, so this is pretty simple:

func copyRestyClient(c *resty.Client) *resty.Client {
	// dereference the pointer and copy the value
	cc := *c
	return &cc
}

If you fancy, some tests show that this is valid:

func Test_copyRestyClient(t *testing.T) {
	t.Run("returns valid copy", func(t *testing.T) {
		original := resty.New()
		copied := copyRestyClient(original)

		require.NotSame(t, original, copied)

		t.Run("copy modifications do not affect original", func(t *testing.T) {
			copied.JSONUnmarshal = func(data []byte, v interface{}) error {
				return errors.New("this is some error")
			}

			require.NotSame(t, original.JSONUnmarshal, copied.JSONUnmarshal)
		})
	})
}

What I'd like to know from a maintainer is if I am overlooking any specifics or potential bugs, if I were to go for this approach.

If not and this is a valid, bug-free solution, is there interest in adding a comfort function to resty which does exactly this, something like func (client *Client) Clone() *Client ?

@Akaame
Copy link

Akaame commented Feb 5, 2024

This is problematic because doing so will result in

assignment copies lock value to FOOBAR: github.com/go-resty/resty/v2.Client contains sync.RWMutex

Execute

go vet ./...

on the piece of code you are running.

Having a clone function would make sense, like resty.NewFromRestyClient etc.

@Akaame
Copy link

Akaame commented Feb 5, 2024

Pinging @jeevatkm to get his opinion on if there is a recommended way of doing this.

@fabiante
Copy link
Contributor Author

I will push a PR which adresses this issue. I am totally open to not merging it since the implementation is pretty crude and I don't know what the usual maintainers prefer :)

@jeevatkm
Copy link
Member

@fabiante @Akaame FYI, #774 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants