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

Accept endpoint as Uri #185

Merged
merged 4 commits into from
Dec 13, 2022
Merged

Accept endpoint as Uri #185

merged 4 commits into from
Dec 13, 2022

Conversation

Pliner
Copy link
Member

@Pliner Pliner commented Dec 13, 2022

Fixes #131, #166

I made the following changes:

  1. HostUrl, PortNumber and Username were removed. The first two were replaced by Endpoint property. The third one seems useless at all, so I just removed it.
  2. Endpoint is used to receive a base absolute path to broker.
  3. Querystring parameters were escaped.

@@ -20,8 +20,8 @@ public GetRatesCriteria(int age, int increment)
{
return new Dictionary<string, string>
{
{nameof(MsgRatesAge), MsgRatesAge.ToString()},
{nameof(MsgRatesIncr), MsgRatesIncr.ToString()}
{ "msg_rates_age", MsgRatesAge.ToString() },
Copy link
Member Author

Choose a reason for hiding this comment

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

To remove code which converts query parameters to snake case.

httpClient = new HttpClient(httpHandler) { Timeout = timeout ?? defaultTimeout };
var httpHandler = new HttpClientHandler { Credentials = new NetworkCredential(username, password) };
configureHttpClientHandler?.Invoke(httpHandler);
httpClient = new HttpClient(httpHandler) { Timeout = timeout ?? DefaultTimeout, BaseAddress = endpoint };
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider to switch to IHttpClientFactory to get all rich configuration features on caller side. So you do not bother with Action<HttpClientHandler> when manually building HttpClient. Of course this will be one more dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am fine with that if there is a simple way to wire up IHttpClientFactory in console app without DI.

Copy link
Contributor

@sungam3r sungam3r Dec 13, 2022

Choose a reason for hiding this comment

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

You may use some DI stuff inside like for CreateBus in ENQ. Consider something like that:

public class ManagementClient
 {
   public ManagementClient(...., Action<IHttpClientBuilder> configure)
   {
     var services = new ServiceCollection();
     var builder = services.AddHttpClient(...);
     configure(builder); // so caller may use all existing available rich API to configure http pipeline
     provider = services.BuildServiceProvider(); // store into field, dispose field in Dispose() method
     httpClient = provider.GetRequiredService<IHttpClientFactory>().GetClient(nameof(ManagementClient));
    }
}

It another ctor accepting Action<IHttpClientBuilder> configure instead of IHttpClientFactory.

@Pliner Pliner merged commit e62805d into master Dec 13, 2022
@Pliner Pliner deleted the accept-uri-as-endpoint branch December 13, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Url with path prefix throws an regex error
2 participants