From 0a25f2f42fecce9a01cb4249770b1eee2d936294 Mon Sep 17 00:00:00 2001 From: Muhammet Ilendemli Date: Wed, 23 Jun 2021 04:41:18 +0200 Subject: [PATCH] [spaceship] parse response body as json if not already (#18766) * parse response body as json if not already parse response body as json if not already * parse if response.body is type of String * fixes broken test * check if response is empty and use empty json object if so, else try parse the body * Clean up logic and removed unneeded if statements and added tests * Add test for string json Co-authored-by: Muhammet Ilendemli Co-authored-by: Josh Holtz --- .../lib/spaceship/connect_api/api_client.rb | 35 ++++++- spaceship/spec/connect_api/api_client_spec.rb | 93 +++++++++++++++++++ 2 files changed, 123 insertions(+), 5 deletions(-) diff --git a/spaceship/lib/spaceship/connect_api/api_client.rb b/spaceship/lib/spaceship/connect_api/api_client.rb index eb33483aa7c..b9382b052da 100644 --- a/spaceship/lib/spaceship/connect_api/api_client.rb +++ b/spaceship/lib/spaceship/connect_api/api_client.rb @@ -207,16 +207,19 @@ def handle_response(response) # Overridden from Spaceship::Client def handle_error(response) + body = response.body.empty? ? {} : response.body + body = JSON.parse(body) if body.kind_of?(String) + case response.status.to_i when 401 - raise UnauthorizedAccessError, format_errors(response) if response && (response.body || {})['errors'] + raise UnauthorizedAccessError, format_errors(response) when 403 - error = (response.body['errors'] || []).first || {} + error = (body['errors'] || []).first || {} error_code = error['code'] if error_code == "FORBIDDEN.REQUIRED_AGREEMENTS_MISSING_OR_EXPIRED" - raise ProgramLicenseAgreementUpdated, format_errors(response) if response && (response.body || {})['errors'] + raise ProgramLicenseAgreementUpdated, format_errors(response) else - raise AccessForbiddenError, format_errors(response) if response && (response.body || {})['errors'] + raise AccessForbiddenError, format_errors(response) end end end @@ -280,7 +283,23 @@ def format_errors(response) # ] # } - return response.body['errors'].map do |error| + # Membership expired + # { + # "errors" : [ + # { + # "id" : "UUID", + # "status" : "403", + # "code" : "FORBIDDEN_ERROR", + # "title" : "This request is forbidden for security reasons", + # "detail" : "Team ID: 'ID' is not associated with an active membership. To check your teams membership status, sign in your account on the developer website. https://developer.apple.com/account/" + # } + # ] + # } + + body = response.body.empty? ? {} : response.body + body = JSON.parse(body) if body.kind_of?(String) + + formatted_errors = (body['errors'] || []).map do |error| messages = [[error['title'], error['detail'], error.dig("source", "pointer")].compact.join(" - ")] meta = error["meta"] || {} @@ -290,6 +309,12 @@ def format_errors(response) [[associated_error["title"], associated_error["detail"]].compact.join(" - ")] end end.flatten.join("\n") + + if formatted_errors.empty? + formatted_errors << "Unknown error" + end + + return formatted_errors end private diff --git a/spaceship/spec/connect_api/api_client_spec.rb b/spaceship/spec/connect_api/api_client_spec.rb index 30461f2e23b..87828e3615c 100644 --- a/spaceship/spec/connect_api/api_client_spec.rb +++ b/spaceship/spec/connect_api/api_client_spec.rb @@ -125,4 +125,97 @@ def stub_client_request(uri, status, body) end.to raise_error(Spaceship::AccessForbiddenError) end end + + describe "#handle_error" do + let(:mock_token) { double('token') } + let(:client) { Spaceship::ConnectAPI::APIClient.new(token: mock_token) } + let(:mock_response) { double('response') } + + describe "status of 200" do + before(:each) do + allow(mock_response).to receive(:status).and_return(200) + end + + it 'does not raise' do + allow(mock_response).to receive(:body).and_return({}) + + expect do + client.send(:handle_error, mock_response) + end.to_not(raise_error) + end + end + + describe "status of 401" do + before(:each) do + allow(mock_response).to receive(:status).and_return(401) + end + + it 'raises UnauthorizedAccessError with no errors in body' do + allow(mock_response).to receive(:body).and_return({}) + + expect do + client.send(:handle_error, mock_response) + end.to raise_error(Spaceship::UnauthorizedAccessError, /Unknown error/) + end + + it 'raises UnauthorizedAccessError when body is string' do + allow(mock_response).to receive(:body).and_return('{"errors":[{"title": "Some title", "detail": "some detail"}]}') + + expect do + client.send(:handle_error, mock_response) + end.to raise_error(Spaceship::UnauthorizedAccessError, /Some title - some detail/) + end + end + + describe "status of 403" do + before(:each) do + allow(mock_response).to receive(:status).and_return(403) + end + + it 'raises ProgramLicenseAgreementUpdated with no errors in body FORBIDDEN.REQUIRED_AGREEMENTS_MISSING_OR_EXPIRED' do + allow(mock_response).to receive(:body).and_return({ + "errors" => [ + { + "code" => "FORBIDDEN.REQUIRED_AGREEMENTS_MISSING_OR_EXPIRED" + } + ] + }) + + expect do + client.send(:handle_error, mock_response) + end.to raise_error(Spaceship::ProgramLicenseAgreementUpdated) + end + + it 'raises AccessForbiddenError with no errors in body' do + allow(mock_response).to receive(:body).and_return({}) + + expect do + client.send(:handle_error, mock_response) + end.to raise_error(Spaceship::AccessForbiddenError, /Unknown error/) + end + + it 'raises AccessForbiddenError when body is string' do + allow(mock_response).to receive(:body).and_return('{"errors":[{"title": "Some title", "detail": "some detail"}]}') + + expect do + client.send(:handle_error, mock_response) + end.to raise_error(Spaceship::AccessForbiddenError, /Some title - some detail/) + end + + it 'raises AccessForbiddenError with errors in body' do + allow(mock_response).to receive(:body).and_return({ + "errors" => [ + { + "title" => "Some title", + "detail" => "some detail" + } + ] + }) + + expect do + client.send(:handle_error, mock_response) + end.to raise_error(Spaceship::AccessForbiddenError, /Some title - some detail/) + end + end + end end