From 5dcab61a4b3e96c8663a80630651d0df6741f971 Mon Sep 17 00:00:00 2001 From: Yuri Zubov Date: Wed, 8 Jun 2022 10:59:05 +0300 Subject: [PATCH 1/6] Bump up simple_command --- onesignal-ruby.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/onesignal-ruby.gemspec b/onesignal-ruby.gemspec index e3d6e27..9defbb9 100644 --- a/onesignal-ruby.gemspec +++ b/onesignal-ruby.gemspec @@ -44,5 +44,5 @@ Gem::Specification.new do |spec| spec.add_runtime_dependency 'activesupport', '>= 5.0.0', '< 8' spec.add_runtime_dependency 'faraday', '>= 1', '< 3' - spec.add_runtime_dependency 'simple_command', '~> 0', '>= 0.0.9' + spec.add_runtime_dependency 'simple_command', '>= 0.0.9' end From 8e4424caf0b5e2d97211434c3c9f67df520c5e44 Mon Sep 17 00:00:00 2001 From: Yura Tolstik Date: Thu, 2 Mar 2023 18:06:21 +0300 Subject: [PATCH 2/6] Handle 502 Server Error from OneSignal Servers --- lib/onesignal/client.rb | 13 +++++++++---- spec/onesignal/client_spec.rb | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/lib/onesignal/client.rb b/lib/onesignal/client.rb index d5b2861..0695954 100644 --- a/lib/onesignal/client.rb +++ b/lib/onesignal/client.rb @@ -46,9 +46,9 @@ def delete_player player_id end def csv_export extra_fields: nil, last_active_since: nil, segment_name: nil - post "players/csv_export?app_id=#{@app_id}", - extra_fields: extra_fields, - last_active_since: last_active_since&.to_i&.to_s, + post "players/csv_export?app_id=#{@app_id}", + extra_fields: extra_fields, + last_active_since: last_active_since&.to_i&.to_s, segment_name: segment_name end @@ -91,7 +91,12 @@ def get url end def handle_errors res - errors = JSON.parse(res.body).fetch 'errors', [] + json = begin + JSON.parse(res.body) + rescue JSON::ParserError, TypeError + {} + end + errors = json.fetch('errors', []) raise ApiError, (errors.first || "Error code #{res.status}") if res.status > 399 || errors.any? res diff --git a/spec/onesignal/client_spec.rb b/spec/onesignal/client_spec.rb index 254ef8f..dc40f9d 100644 --- a/spec/onesignal/client_spec.rb +++ b/spec/onesignal/client_spec.rb @@ -18,6 +18,13 @@ }.not_to raise_error end + it 'raises an error if the response does not have body' do + res = double :res, body: nil, status: 204 + expect { + expect(subject.send :handle_errors, res) + }.not_to raise_error + end + it 'raises an error if the response code is greater than 399' do res = double :res, body: '{ "errors": ["Internal Server Error"] }', status: 500 expect { @@ -32,6 +39,17 @@ }.to raise_error Client::ApiError, 'Error code 401' end + it 'raises an error if the response is a html' do + body = ''\ + '502 Server Error

Error: Server Error

'\ + '

The server encountered a temporary error and could not complete your request.'\ + '

Please try again in 30 seconds.

' + res = double :res, body: body, status: 502 + expect { + subject.send :handle_errors, res + }.to raise_error Client::ApiError, 'Error code 502' + end + it 'raises an error if the body contains errors' do res = double :res, body: '{ "errors": ["Internal Server Error"] }', status: 200 expect { From 3d5c3b8f9da90bb21f9aa75cf06b0787c2857bbd Mon Sep 17 00:00:00 2001 From: Yura Tolstik Date: Thu, 2 Mar 2023 19:14:45 +0300 Subject: [PATCH 3/6] Use faker from rubygems --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index 2573fc1..2833a20 100644 --- a/Gemfile +++ b/Gemfile @@ -8,5 +8,5 @@ git_source(:github) { |repo_name| "https://github.com/#{repo_name}" } gemspec group :test do - gem 'faker', git: 'https://github.com/stympy/faker.git', branch: 'master' + gem 'faker' end From b266c1b7db265d0df4fee68438660e529e4c6487 Mon Sep 17 00:00:00 2001 From: Yura Tolstik Date: Mon, 20 Mar 2023 12:03:45 +0300 Subject: [PATCH 4/6] Introduce ServerError and ClientError --- lib/onesignal/client.rb | 8 +++++++- spec/factories/segments.rb | 6 ++++-- spec/onesignal/client_spec.rb | 8 ++++---- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/lib/onesignal/client.rb b/lib/onesignal/client.rb index 0695954..f0b0fa5 100644 --- a/lib/onesignal/client.rb +++ b/lib/onesignal/client.rb @@ -5,6 +5,8 @@ module OneSignal class Client class ApiError < RuntimeError; end + class ClientError < ApiError; end + class ServerError < ApiError; end def initialize app_id, api_key, api_url @app_id = app_id @@ -97,7 +99,11 @@ def handle_errors res {} end errors = json.fetch('errors', []) - raise ApiError, (errors.first || "Error code #{res.status}") if res.status > 399 || errors.any? + if res.status > 499 + raise ServerError, errors.first || "Error code #{res.status}" + elsif errors.any? || res.status > 399 + raise ClientError, errors.first || "Error code #{res.status}" + end res end diff --git a/spec/factories/segments.rb b/spec/factories/segments.rb index d461415..566f45e 100644 --- a/spec/factories/segments.rb +++ b/spec/factories/segments.rb @@ -1,7 +1,9 @@ # frozen_string_literal: true FactoryBot.define do - factory :segment do - initialize_with { new(name: Faker::Movies::StarWars.character) } + factory :segment, class: OneSignal::Segment do + name { Faker::Movies::StarWars.character } + + initialize_with { new(attributes) } end end diff --git a/spec/onesignal/client_spec.rb b/spec/onesignal/client_spec.rb index dc40f9d..43f920e 100644 --- a/spec/onesignal/client_spec.rb +++ b/spec/onesignal/client_spec.rb @@ -29,14 +29,14 @@ res = double :res, body: '{ "errors": ["Internal Server Error"] }', status: 500 expect { subject.send :handle_errors, res - }.to raise_error Client::ApiError, 'Internal Server Error' + }.to raise_error Client::ServerError, 'Internal Server Error' end it 'raises an error if the response code is greater than 399 with default error message' do res = double :res, body: '{}', status: 401 expect { subject.send :handle_errors, res - }.to raise_error Client::ApiError, 'Error code 401' + }.to raise_error Client::ClientError, 'Error code 401' end it 'raises an error if the response is a html' do @@ -47,14 +47,14 @@ res = double :res, body: body, status: 502 expect { subject.send :handle_errors, res - }.to raise_error Client::ApiError, 'Error code 502' + }.to raise_error Client::ServerError, 'Error code 502' end it 'raises an error if the body contains errors' do res = double :res, body: '{ "errors": ["Internal Server Error"] }', status: 200 expect { subject.send :handle_errors, res - }.to raise_error Client::ApiError, 'Internal Server Error' + }.to raise_error Client::ClientError, 'Internal Server Error' end end From 826b6321fdd0dde946b604ec991647abd423a3ed Mon Sep 17 00:00:00 2001 From: Yura Tolstik Date: Fri, 14 Apr 2023 12:38:45 +0300 Subject: [PATCH 5/6] Get detailed error messages from server --- lib/onesignal/client.rb | 16 +++++++++++-- spec/onesignal/client_spec.rb | 44 ++++++++++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/lib/onesignal/client.rb b/lib/onesignal/client.rb index f0b0fa5..3c5af22 100644 --- a/lib/onesignal/client.rb +++ b/lib/onesignal/client.rb @@ -6,8 +6,16 @@ module OneSignal class Client class ApiError < RuntimeError; end class ClientError < ApiError; end + class InvalidExternalUserIdsError < ClientError; end + class InvalidPlayerIdsError < ClientError; end + class TagsLimitError < ClientError; end class ServerError < ApiError; end + ERROR_MESSAGE_MAPPING = { + 'invalid_external_user_ids' => InvalidExternalUserIdsError, + 'invalid_player_ids' => InvalidPlayerIdsError + }.freeze + def initialize app_id, api_key, api_url @app_id = app_id @api_key = api_key @@ -101,8 +109,12 @@ def handle_errors res errors = json.fetch('errors', []) if res.status > 499 raise ServerError, errors.first || "Error code #{res.status}" - elsif errors.any? || res.status > 399 - raise ClientError, errors.first || "Error code #{res.status}" + elsif errors.any? + error = errors.detect { |key, _v| ERROR_MESSAGE_MAPPING.keys.include?(key) } + raise ERROR_MESSAGE_MAPPING[error[0]].new(error[1]) if error + raise ClientError, errors.first + elsif res.status > 399 + raise ClientError, errors.first || "Error code #{res.status} #{res.body}" end res diff --git a/spec/onesignal/client_spec.rb b/spec/onesignal/client_spec.rb index 43f920e..4f7401c 100644 --- a/spec/onesignal/client_spec.rb +++ b/spec/onesignal/client_spec.rb @@ -18,14 +18,52 @@ }.not_to raise_error end - it 'raises an error if the response does not have body' do + it 'does not raise an error if the response does not have body' do res = double :res, body: nil, status: 204 expect { expect(subject.send :handle_errors, res) }.not_to raise_error end - it 'raises an error if the response code is greater than 399' do + it 'raises an InvalidExternalUserIdsError if the response contains errors' do + body = '{ + "errors": { + "invalid_external_user_ids": [ + "786956" + ] + } + }' + res = double :res, body: body, status: 200 + expect { + subject.send :handle_errors, res + }.to raise_error Client::InvalidExternalUserIdsError, '["786956"]' + end + + it 'raises an InvalidPlayerIdsError if the response contains errors' do + body = '{ + "errors": { + "invalid_player_ids" : ["6392d91a-b206-4b7b-a620-cd68e32c3a76"] + } + }' + res = double :res, body: body, status: 200 + expect { + subject.send :handle_errors, res + }.to raise_error Client::InvalidPlayerIdsError, '["6392d91a-b206-4b7b-a620-cd68e32c3a76"]' + end + + it 'raises an ClientError if the response contains errors' do + body = '{ + "errors": [ + "Message Notifications must have English language content" + ] + }' + res = double :res, body: body, status: 200 + expect { + subject.send :handle_errors, res + }.to raise_error Client::ClientError, 'Message Notifications must have English language content' + end + + it 'raises an error if the response code is greater than 499' do res = double :res, body: '{ "errors": ["Internal Server Error"] }', status: 500 expect { subject.send :handle_errors, res @@ -36,7 +74,7 @@ res = double :res, body: '{}', status: 401 expect { subject.send :handle_errors, res - }.to raise_error Client::ClientError, 'Error code 401' + }.to raise_error Client::ClientError, 'Error code 401 {}' end it 'raises an error if the response is a html' do From 79c00ece663e5df7b958c306cde07a022a78c5bb Mon Sep 17 00:00:00 2001 From: Yura Tolstik Date: Wed, 26 Apr 2023 11:13:15 +0300 Subject: [PATCH 6/6] Introduce APIRateLimitError --- lib/onesignal/client.rb | 5 ++++- spec/onesignal/client_spec.rb | 7 +++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/onesignal/client.rb b/lib/onesignal/client.rb index 3c5af22..3a6e624 100644 --- a/lib/onesignal/client.rb +++ b/lib/onesignal/client.rb @@ -6,12 +6,14 @@ module OneSignal class Client class ApiError < RuntimeError; end class ClientError < ApiError; end + class ApiRateLimitError < ClientError; end class InvalidExternalUserIdsError < ClientError; end class InvalidPlayerIdsError < ClientError; end class TagsLimitError < ClientError; end class ServerError < ApiError; end ERROR_MESSAGE_MAPPING = { + 'API rate limit exceeded' => ApiRateLimitError, 'invalid_external_user_ids' => InvalidExternalUserIdsError, 'invalid_player_ids' => InvalidPlayerIdsError }.freeze @@ -111,7 +113,8 @@ def handle_errors res raise ServerError, errors.first || "Error code #{res.status}" elsif errors.any? error = errors.detect { |key, _v| ERROR_MESSAGE_MAPPING.keys.include?(key) } - raise ERROR_MESSAGE_MAPPING[error[0]].new(error[1]) if error + raise ERROR_MESSAGE_MAPPING[error[0]].new(error[1]) if error && error.is_a?(Array) + raise ERROR_MESSAGE_MAPPING[error].new(error) if error raise ClientError, errors.first elsif res.status > 399 raise ClientError, errors.first || "Error code #{res.status} #{res.body}" diff --git a/spec/onesignal/client_spec.rb b/spec/onesignal/client_spec.rb index 4f7401c..80357a7 100644 --- a/spec/onesignal/client_spec.rb +++ b/spec/onesignal/client_spec.rb @@ -70,6 +70,13 @@ }.to raise_error Client::ServerError, 'Internal Server Error' end + it 'raises an ApiRateLimitError if the response contains errors' do + res = double :res, body: '{ "errors": ["API rate limit exceeded"] }', status: 429 + expect { + subject.send :handle_errors, res + }.to raise_error Client::ApiRateLimitError, 'API rate limit exceeded' + end + it 'raises an error if the response code is greater than 399 with default error message' do res = double :res, body: '{}', status: 401 expect {