-
Notifications
You must be signed in to change notification settings - Fork 4
Enhance error reporting #16
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
base: master
Are you sure you want to change the base?
Changes from all commits
6c04a55
8f80716
3073549
8b798fa
8986a96
4dfc15b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -215,47 +215,81 @@ def persistent? | |||||
| # @param [Hash] params custom search inputs | ||||||
| # @return [String|Hash] raw HTML or decoded response as JSON / Hash | ||||||
| def get(endpoint, decoder = :json, params = {}) | ||||||
| # execute get via open socket | ||||||
| response = if persistent? | ||||||
| @socket.get(endpoint, params: query(params)) | ||||||
| else | ||||||
| HTTP.timeout(timeout).get("https://#{BACKEND}#{endpoint}", params: query(params)) | ||||||
| end | ||||||
|
|
||||||
| # decode response using JSON native parser | ||||||
| response = execute_request(endpoint, params) | ||||||
| handle_response(response, decoder, endpoint, params) | ||||||
| end | ||||||
|
|
||||||
| def execute_request(endpoint, params) | ||||||
| if persistent? | ||||||
| @socket.get(endpoint, params: query(params)) | ||||||
| else | ||||||
| url = "https://#{BACKEND}#{endpoint}" | ||||||
| HTTP.timeout(timeout).get(url, params: query(params)) | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| def handle_response(response, decoder, endpoint, params) | ||||||
| case decoder | ||||||
| when :json | ||||||
| # read http response | ||||||
| begin | ||||||
| # user can turn on/off JSON keys to symbols | ||||||
| # this is more memory efficient, but not always needed | ||||||
| symbolize_names = params.key?(:symbolize_names) ? params[:symbolize_names] : true | ||||||
|
|
||||||
| # parse JSON response with Ruby standard library | ||||||
| data = JSON.parse(response.body, symbolize_names: symbolize_names) | ||||||
| if data.instance_of?(Hash) && data.key?(:error) | ||||||
| raise SerpApiError, "HTTP request failed with error: #{data[:error]} from url: https://#{BACKEND}#{endpoint}, params: #{params}, decoder: #{decoder}, response status: #{response.status} " | ||||||
| elsif response.status != 200 | ||||||
| raise SerpApiError, "HTTP request failed with response status: #{response.status} reponse: #{data} on get url: https://#{BACKEND}#{endpoint}, params: #{params}, decoder: #{decoder}" | ||||||
| end | ||||||
| rescue JSON::ParserError | ||||||
| raise SerpApiError, "JSON parse error: #{response.body} on get url: https://#{BACKEND}#{endpoint}, params: #{params}, decoder: #{decoder}, response status: #{response.status}" | ||||||
| end | ||||||
|
|
||||||
| # discard response body | ||||||
| response.flush if persistent? | ||||||
|
|
||||||
| data | ||||||
| process_json_response(response, endpoint, params) | ||||||
| when :html | ||||||
| # html decoder | ||||||
| if response.status != 200 | ||||||
| raise SerpApiError, "HTTP request failed with response status: #{response.status} reponse: #{data} on get url: https://#{BACKEND}#{endpoint}, params: #{params}, decoder: #{decoder}" | ||||||
| end | ||||||
|
|
||||||
| response.body | ||||||
| process_html_response(response, endpoint, params) | ||||||
| else | ||||||
| raise SerpApiError, "not supported decoder: #{decoder}, available: :json, :html" | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| def process_json_response(response, endpoint, params) | ||||||
| symbolize = params.fetch(:symbolize_names, true) | ||||||
|
|
||||||
| begin | ||||||
| data = JSON.parse(response.body, symbolize_names: symbolize) | ||||||
| validate_json_content!(data, response, endpoint, params) | ||||||
| rescue JSON::ParserError | ||||||
| raise_parser_error(response, endpoint, params) | ||||||
| end | ||||||
|
|
||||||
| response.flush if persistent? | ||||||
| data | ||||||
| end | ||||||
|
|
||||||
| def process_html_response(response, endpoint, params) | ||||||
| raise_http_error(response, nil, endpoint, params, decoder: :html) if response.status != 200 | ||||||
| response.body | ||||||
| end | ||||||
|
|
||||||
| def validate_json_content!(data, response, endpoint, params) | ||||||
| # Check for API-level error inside the JSON | ||||||
| if data.is_a?(Hash) && data.key?(:error) | ||||||
| raise_http_error(response, data, endpoint, params, explicit_error: data[:error]) | ||||||
| # Check for HTTP-level error | ||||||
|
||||||
| # Check for HTTP-level error |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When data is not a Hash (e.g., if the JSON response is an array or primitive value), the fallback data[:error] on line 278 may not behave as expected. While this is unlikely given typical API responses, consider either checking if data is a Hash before accessing data[:error], or simplifying this to just use explicit_error since the fallback case (data[:error]) would be nil anyway when called from line 267 (where we know data doesn't have an :error key or isn't a Hash).
| serpapi_error: explicit_error || (data ? data[:error] : nil), | |
| serpapi_error: explicit_error || (data.is_a?(Hash) ? data[:error] : nil), |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The safe navigation operator &. on line 281 only guards against nil, not against data being a type that doesn't respond to dig (like String or Integer). If the JSON response is unexpectedly a primitive value, this would raise a NoMethodError. Consider wrapping this in a check: data.is_a?(Hash) ? data.dig(:search_metadata, :id) : nil or handling this more defensively.
| search_id: data&.dig(:search_metadata, :id), | |
| search_id: data.is_a?(Hash) ? data.dig(:search_metadata, :id) : nil, |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,13 +2,89 @@ | |||||
| # frozen_string_literal: true | ||||||
|
|
||||||
| module SerpApi | ||||||
| # SerpApiError wraps any errors related to the SerpApi client. | ||||||
| # Custom error class for SerpApi-related errors. | ||||||
| # Inherits from StandardError. | ||||||
| # Includes optional attributes for detailed error context. | ||||||
| # Attributes: | ||||||
| # - serpapi_error: String error message from SerpApi (optional) | ||||||
| # - search_params: Hash of search parameters used (optional) | ||||||
| # - response_status: Integer HTTP or response status code (optional) | ||||||
| # - search_id: String id returned by the service for the search (optional) | ||||||
| # - decoder: Symbol representing the decoder/format used (optional) (e.g. :json) | ||||||
| class SerpApiError < StandardError | ||||||
| # List the specific types of errors handled by the Error class. | ||||||
| # - HTTP response errors from SerpApi.com | ||||||
| # - Missing API key | ||||||
| # - Credit limit | ||||||
| # - Incorrect query | ||||||
| # - more ... | ||||||
| attr_reader :serpapi_error, :search_params, :response_status, :search_id, :decoder | ||||||
|
|
||||||
| # All attributes are optional keyword arguments. | ||||||
| # | ||||||
| # @param message [String, nil] an optional human message passed to StandardError | ||||||
| # @param serpapi_error [String, nil] optional error string coming from SerpAPI | ||||||
| # @param search_params [Hash, nil] optional hash of the search parameters used | ||||||
| # @param response_status [Integer, nil] optional HTTP or response status code | ||||||
| # @param search_id [String, nil] optional id returned by the service for the search | ||||||
| # @param decoder [String, nil] optional decoder/format used (e.g. "json") | ||||||
|
||||||
| # @param decoder [String, nil] optional decoder/format used (e.g. "json") | |
| # @param decoder [Symbol, nil] optional decoder/format used (e.g. :json) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling freeze on a Symbol is unnecessary since symbols are already immutable in Ruby. While this doesn't cause any issues, you can remove the freeze call on line 79 for consistency with Ruby idioms.
| value.freeze | |
| value |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| module SerpApi | ||
| # Current version of the gem | ||
| VERSION = '1.0.2'.freeze | ||
| VERSION = '1.0.3'.freeze | ||
| end |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -73,7 +73,7 @@ | |||
| it 'get endpoint error' do | ||||
| expect { | ||||
| client.send(:get, '/search', :json, {}) | ||||
| }.to raise_error(SerpApi::SerpApiError).with_message(/HTTP request failed with error: Missing query `q` parameter./) | ||||
| }.to raise_error(SerpApi::SerpApiError).with_message(/HTTP request failed with status: 400.* error: Missing query `q` parameter./) | ||||
|
||||
| end | ||||
|
|
||||
| it 'get bad endpoint' do | ||||
|
|
@@ -90,7 +90,7 @@ | |||
| begin | ||||
| client.send(:get, '/invalid', :html, {}) | ||||
| rescue SerpApi::SerpApiError => e | ||||
| expect(e.message).to include(/HTTP request failed with response status: 404 Not Found reponse/), "got #{e.message}" | ||||
| expect(e.message).to include("HTTP request failed with status: 404") | ||||
| rescue => e | ||||
| raise("wrong exception: #{e}") | ||||
| end | ||||
|
|
@@ -134,6 +134,8 @@ | |||
| expect { client.search(q: 'Invalid Query') }.to raise_error(SerpApi::SerpApiError) | ||||
| end | ||||
|
|
||||
|
|
||||
|
|
||||
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted to satisfy Rubocop