Skip to content

Conversation

@senid231
Copy link

@senid231 senid231 commented Apr 23, 2025

Description

Fixes #1375

Adds the ability to configure a proxy server for all ShopifyAPI HTTP requests

ShopifyAPI::Context.setup(
  # other params...
  httparty_params: {
    timeout: 60, # Set the open/read/write timeout for HTTP requests
    open_timeout: 60, # Set the open timeout for HTTP requests
    read_timeout: 60, # Set the read timeout for HTTP requests
    write_timeout: 60, # Set the write timeout for HTTP requests
    debug_output: false, # Set to true to enable debug output for HTTP requests
    http_proxyaddr: "http://proxy.example.com:8080", # Set the HTTP proxy address
    http_proxyport: 8080, # Set the HTTP proxy port
    http_proxyuser: "username", # Set the HTTP proxy username
    http_proxypass: "password", # Set the HTTP proxy password
  }
)

How has this been tested?

proxy server configuration can't be tested via WebMock because it ignores proxy settings

Checklist:

  • My commit message follows the pattern described in here
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the project documentation.
  • I have added a changelog line.

@senid231 senid231 requested a review from a team as a code owner April 23, 2025 09:05
Allows to override HTTP request params such as timeout, debug_output, or proxy settings.
@senid231 senid231 force-pushed the context-add-httparty-params branch from 978c053 to 8ed225d Compare April 23, 2025 09:09
@venraom
Copy link

venraom commented Sep 25, 2025

@CodiumAI-Agent /improve

@CodiumAI-Agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
General
Fix proxy address example

Remove the scheme and port from http_proxyaddr in the example to match HTTParty's
expected separate fields. Supplying a URL string here while also setting
http_proxyport can cause misconfiguration.

README.md [74-76]

-http_proxyaddr: "http://proxy.example.com:8080", # Set the HTTP proxy address
+http_proxyaddr: "proxy.example.com", # Set the HTTP proxy address (host only, no scheme/port)
 http_proxyport: 8080, # Set the HTTP proxy port
Suggestion importance[1-10]: 7

__

Why: Correcting the README example to exclude scheme/port from http_proxyaddr prevents misconfiguration for users; it's accurate and improves developer experience.

Medium
Possible issue
Make parameter optional in sig

Make httparty_params optional in the Sorbet signature to match the default value and
usage. Declaring it as required while providing a default {} can cause type-check
inconsistencies and break existing callers.

lib/shopify_api/context.rb [44-53]

 sig do
   params(
     api_key: String,
     api_secret_key: String,
     api_version: String,
     scope: T.any(String, T::Array[String]),
     is_private: T::Boolean,
     is_embedded: T::Boolean,
     log_level: T.any(String, Symbol),
     logger: ShopifyAPI::Logger,
     host_name: T.nilable(String),
     host: T.nilable(String),
     private_shop: T.nilable(String),
     user_agent_prefix: T.nilable(String),
     old_api_secret_key: T.nilable(String),
     api_host: T.nilable(String),
     response_as_struct: T.nilable(T::Boolean),
     rest_disabled: T.nilable(T::Boolean),
-    httparty_params: T::Hash[Symbol, T.untyped]
+    httparty_params: T.nilable(T::Hash[Symbol, T.untyped])
   ).void
 end
Suggestion importance[1-10]: 6

__

Why: Making httparty_params nilable aligns with providing a default {} in setup, improving type consistency; the change is reasonable and low risk but not critical.

Low
Safely splat optional params

Guard against nil when splatting httparty_params. If the Sorbet signature is made
nilable or future changes set it to nil, double-splatting nil raises an error.

lib/shopify_api/clients/http_client.rb [51-58]

 res = T.cast(HTTParty.send(
   request.http_method,
   parsed_uri.to_s,
-  **Context.httparty_params,
+  **(Context.httparty_params || {}),
   headers: headers,
   query: request.query,
   body: request.body.class == Hash ? T.unsafe(request.body).to_json : request.body,
 ), HTTParty::Response)
Suggestion importance[1-10]: 5

__

Why: Guarding the double-splat avoids potential runtime errors if httparty_params ever becomes nil, but with current code it's always a Hash, so impact is modest.

Low

@venraom
Copy link

venraom commented Sep 25, 2025

@CodiumAI-Agent /review

@kubaw
Copy link

kubaw commented Oct 17, 2025

Would it be possible to merge this? We're really looking forward to this feature.

@CodiumAI-Agent
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

1375 - PR Code Verified

Compliant requirements:

  • Allow configuring a proxy server for all ShopifyAPI HTTP requests.
  • Expose a way to pass through HTTParty options without affecting global HTTParty state.
  • Provide documentation on how to configure proxy and related HTTP params.
  • Add tests to cover the new configuration path.

Requires further human verification:

  • Manual verification with a real proxy environment (since WebMock ignores proxy settings).
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

Passing Context.httparty_params with ** splat alongside explicit headers/query/body relies on params shape not colliding with HTTParty's expected options. If a user includes any of these keys, behavior could be overridden unintentionally; also ensure nil/empty hash defaults are handled safely.

res = T.cast(HTTParty.send(
  request.http_method,
  parsed_uri.to_s,
  **Context.httparty_params,
  headers: headers,
  query: request.query,
  body: request.body.class == Hash ? T.unsafe(request.body).to_json : request.body,
), HTTParty::Response)
Type Signature

The sig for setup defines httparty_params as a required T::Hash[Symbol, T.untyped], but the default is {}. Consider making it T.nilable(...) or with default keyword to prevent Sorbet mismatch in callers omitting it.

  host_name: T.nilable(String),
  host: T.nilable(String),
  private_shop: T.nilable(String),
  user_agent_prefix: T.nilable(String),
  old_api_secret_key: T.nilable(String),
  api_host: T.nilable(String),
  response_as_struct: T.nilable(T::Boolean),
  rest_disabled: T.nilable(T::Boolean),
  httparty_params: T::Hash[Symbol, T.untyped]
).void
Docs Accuracy

Example shows http_proxyaddr containing a scheme and port while also setting http_proxyport separately; for HTTParty/Net::HTTP the address should typically be a hostname. Clarify to avoid misconfiguration.

Optionally, you can override HTTP request params such as timeout, debug_output, or proxy settings.  
```ruby
ShopifyAPI::Context.setup(
  # other params...
  httparty_params: {
    timeout: 60, # Set the open/read/write timeout for HTTP requests
    open_timeout: 60, # Set the open timeout for HTTP requests
    read_timeout: 60, # Set the read timeout for HTTP requests
    write_timeout: 60, # Set the write timeout for HTTP requests
    debug_output: false, # Set to true to enable debug output for HTTP requests
    http_proxyaddr: "http://proxy.example.com:8080", # Set the HTTP proxy address
    http_proxyport: 8080, # Set the HTTP proxy port
    http_proxyuser: "username", # Set the HTTP proxy username
    http_proxypass: "password", # Set the HTTP proxy password
  }
)

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.

Add proxy support for HTTP requests

4 participants