Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

add optional ShopifyAPI::Context::httparty_params#1376

Open
senid231 wants to merge 2 commits into
Shopify:main from
senid231:context-add-httparty-params
Open

add optional ShopifyAPI::Context::httparty_params #1376
senid231 wants to merge 2 commits into
Shopify:main from
senid231:context-add-httparty-params

Conversation

@senid231

@senid231 senid231 commented Apr 23, 2025
edited
Loading

Copy link
Copy Markdown

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.

kubaw reacted with thumbs up emoji

venraom commented Sep 25, 2025

Copy link
Copy Markdown

@CodiumAI-Agent /improve

QodoAI-Agent reacted with eyes emoji

Copy link
Copy Markdown

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 commented Sep 25, 2025

Copy link
Copy Markdown

@CodiumAI-Agent /review

QodoAI-Agent reacted with eyes emoji

kubaw commented Oct 17, 2025

Copy link
Copy Markdown

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

wpsamurai reacted with thumbs up emoji

Copy link
Copy Markdown

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
 }
)

senid231 added 2 commits June 11, 2026 15:35
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 8ed225d to bc3a922 Compare June 11, 2026 12:49

Copy link
Copy Markdown
Author

PR rebased and code review applied

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add proxy support for HTTP requests

AltStyle によって変換されたページ (->オリジナル) /