go-resty / resty

Simple HTTP and REST client library for Go
MIT License
9.98k stars 706 forks source link

Clone or copy of existing client #773

Closed fabiante closed 7 months ago

fabiante commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

fabiante commented 7 months ago

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 commented 7 months ago

@fabiante @Akaame FYI, https://github.com/go-resty/resty/pull/774#pullrequestreview-1887425340