riferrei / srclient

Golang Client for Schema Registry
Apache License 2.0
235 stars 70 forks source link

Expose Http Client #22

Closed jakesylvestre closed 3 years ago

jakesylvestre commented 4 years ago

Certain methods for exposing the registry client (namely service tokens require access to the http client to add auth headers. Would you be open to exposing the httpClient->HttpClient on SchemaRegsitryClient so we can add in interceptors and inject the headers we need. I'm open to other approaches as well and will be happy to submit a pull request however you want me to implement this

riferrei commented 4 years ago

I think exposing the httpClient object is risky because it breaks the encapsulation logic used in the code. Maybe a better approach could be creating an overloaded version of the function CreateSchemaRegistryClient where extra parameters could be provided?

I am open to suggestions along these lines :)

Thanks,

-- @riferrei

jakesylvestre commented 4 years ago

Got it - will set this up now

jakesylvestre commented 4 years ago

@riferrei image

Will clean this up, but probably the cleanest I can do (w/o builder pattern since Golang doesn't support virtual method overriding.

Alternatively, we can add a SetClient method that can be called once the client is created

maarek commented 4 years ago

I was thinking about this last week and had started to add something similar to the following and wasn't quite sure it was the way to go. I ended up backing out of the change and using httpTest in the test cases instead of injecting an httpClient.

type Config func(*SchemaRegistryClient)

func WithHttpClient(httpClient *http.Client) *SchemaRegistryClient {
    return func (client *SchemaRegistryClient) {
        client.httpClient = httpClient
    }
}

func CreateSchemaRegistryClient(schemaRegistryURL string,  options ...Config) *SchemaRegistryClient {
    schemaRegistryClient := ...
   for opt := range options {
       opt(schemaRegistryClient)
   }
   return schemaRegistryClient
}
tomarrell commented 4 years ago

I think the functional option direction would be best here.

I would try steer clear of exposing the http client on the struct to avoid accidental mutation. It also would lead to a bit of a fragmented configuration API imo.

AtakanColak commented 4 years ago

I'd like to assign myself to this work, and I don't plant to expose the http client. Could you give me a list of what kind of changes user might want to do on the http client?

AtakanColak commented 3 years ago

Closed as http client is now exposed with #49