ShopifySharp icon indicating copy to clipboard operation
ShopifySharp copied to clipboard

How to manually set proxy details

Open ericdunn opened this issue 5 years ago • 10 comments

Hey guys,

I just updated to the latest version of ShopifySharp, and based on a previous commit, you are now using HttpClient instead or Flurl. I don't mind removing that dependency, but this did break my app in the sense that I was setting proxy details using Flurl's FlurlHttp.GlobalSettings , so it worked across the app. Not the best, but it worked.

I was wondering, now that we're using HttpClient, how would one manually set proxy details? I can't rely on what's configured on the system and have to manually set it.

Any ideas?

Thanks! Eric.

ericdunn avatar Nov 29 '18 20:11 ericdunn

Without creating a full pull request, something like adding an HttpMessageHanlder to ShopifyService's constructor may do the trick :

        private static IRequestExecutionPolicy _GlobalExecutionPolicy = new DefaultRequestExecutionPolicy();

        private static JsonSerializer _Serializer = new JsonSerializer { DateParseHandling = DateParseHandling.DateTimeOffset };

        private static HttpClient _Client { get; set; }

        private IRequestExecutionPolicy _ExecutionPolicy;

        protected Uri _ShopUri { get; set; }

        protected string _AccessToken { get; set; }

        /// <summary>
        /// Creates a new instance of <see cref="ShopifyService" />.
        /// </summary>
        /// <param name="myShopifyUrl">The shop's *.myshopify.com URL.</param>
        /// <param name="shopAccessToken">An API access token for the shop.</param>
        protected ShopifyService(string myShopifyUrl, string shopAccessToken, HttpMessageHandler handler = null)
        {
            _ShopUri = BuildShopUri(myShopifyUrl, false);
            _AccessToken = shopAccessToken;
            _Client = handler == null ? new HttpClient() : new HttpClient(handler);

            // If there's a global execution policy it should be set as this instance's policy.
            // User can override it with instance-specific execution policy.
            _ExecutionPolicy = _GlobalExecutionPolicy ?? new DefaultRequestExecutionPolicy();
        }

Implementing it as null by default wouldn't break anybody, but would allow a bit more flexibility.

ericdunn avatar Nov 29 '18 20:11 ericdunn

It's a bad practice to instantiate a new HttpClient for each request. HttpClient must be long lived. We could potentially make the static HttpClient setter public. Or somehow use the recently added HttpClientFactory designed to provide more flexibilty.

https://docs.microsoft.com/en-us/dotnet/standard/microservices-architecture/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-2.1

clement911 avatar Nov 29 '18 22:11 clement911

You're right about multiple HttpClients lol. I think the clientfactory route is probably better, since you then remain in control of the HttpClient. Either or for me works really!

ericdunn avatar Nov 30 '18 14:11 ericdunn

Only problem there I guess would be the need to up the min support to netstandard2.1, and drop support for anything lower, or figure out a clean way to use preprocessor directives. It can be done, and is probably the right thing to do (using the factory vs handling the client yourself), but it's not a quick fix. I'll see about creating a pull request and see what that looks like.

ericdunn avatar Nov 30 '18 14:11 ericdunn

That's likely a little aggressive to move everyone to my beloved netcore2.1.

Could this be handled with a global setting that just creates a new httpClient right when you set it?

Like how the execution policy is handled, but for all?

dkershner6 avatar Nov 30 '18 14:11 dkershner6

I agree with you there. For now, I just override ExecuteRequestAsync and set things up myself. Not ideal, but at least it unblocked me. I'll try to think of a good solution over the weekend.

Personally I like the mimicking the execution policy.

You could have something that would return either an HttpClient from there, or an HttpClientHandler (although this approach would probably mean instanciating a new HttpClient everytime to make sure to use the provided handler). Then there could be a default that just uses the status quo.

Something to think about for sure. Thanks all!

ericdunn avatar Nov 30 '18 16:11 ericdunn

Interesting use case! I also like the execution policy suggestion. What about making the underlying HttpClient virtual, so you can extend each service that you use and then set your own HttpClient in the constructor?

Edit: woops nevermind, forgot that the HttpClient is static. That wouldn't work.

nozzlegear avatar Dec 01 '18 05:12 nozzlegear

You could make it protected however, that would work in my specific case, but wouldn't really allow anybody to set their own proxy details without at least creating their own version of each service they want to use.

ericdunn avatar Dec 04 '18 19:12 ericdunn

Just thinking out loud, this package is constructing HttpRequestMessages for each request and passing those to the HttpClient. I wonder if there's a way to set the proxy on those messages. Then we could add a global or instance-specific function to give a proxy to the service, and the service can pass that to the message.

nozzlegear avatar Dec 04 '18 19:12 nozzlegear

I'd also like to explore adding the client factory in 5.0, where we can add support for netstandard2.1

nozzlegear avatar Dec 04 '18 19:12 nozzlegear