Merge branch 'error-refactor' into 'master'

Christopher Vollick created

Error refactor

See merge request soprani.ca/sgx-bwmsgsv2!13

Change summary

lib/bandwidth_error.rb | 24 ++++++++++++++++++++++++
sgx-bwmsgsv2.rb        | 42 ++++++++++++------------------------------
test/test_component.rb | 14 +++++++++++++-
3 files changed, 49 insertions(+), 31 deletions(-)

Detailed changes

lib/bandwidth_error.rb 🔗

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+class BandwidthError < StandardError
+	attr_reader :code
+
+	def self.for(http_code, body)
+		parsed_body = JSON.parse(body) rescue nil
+		if parsed_body.is_a?(Hash)
+			new(
+				http_code,
+				parsed_body["description"].to_s.strip + " " +
+				(parsed_body.fetch("fieldErrors", []))
+					.map { |err| err["description"] }.join(" ")
+			)
+		else
+			new(http_code, "Error #{http_code}")
+		end
+	end
+
+	def initialize(code, msg)
+		super(msg)
+		@code = code
+	end
+end

sgx-bwmsgsv2.rb 🔗

@@ -35,6 +35,7 @@ require 'log4r'
 
 require 'em_promise'
 
+require_relative 'lib/bandwidth_error'
 require_relative 'lib/registration_repo'
 
 Sentry.init
@@ -169,24 +170,6 @@ module SGXbwmsgsv2
 		panic(e)
 	end
 
-	def self.error_msg(orig, query_node, type, name, _text=nil)
-		orig.type = :error
-
-		error = Nokogiri::XML::Node.new 'error', orig.document
-		error['type'] = type
-		orig.add_child(error)
-
-		suberr = Nokogiri::XML::Node.new name, orig.document
-		suberr['xmlns'] = 'urn:ietf:params:xml:ns:xmpp-stanzas'
-		error.add_child(suberr)
-
-		orig.add_child(query_node) if query_node
-
-		# TODO: add some explanatory xml:lang='en' text (see text param)
-		puts "RESPONSE3: #{orig.inspect}"
-		return orig
-	end
-
 	# workqueue_count MUST be 0 or else Blather uses threads!
 	setup ARGV[0], ARGV[1], ARGV[2], ARGV[3], nil, nil, workqueue_count: 0
 
@@ -253,7 +236,9 @@ module SGXbwmsgsv2
 					http
 				end
 			else
-				EMPromise.reject(http.response_header.status)
+				EMPromise.reject(
+					BandwidthError.for(http.response_header.status, http.response)
+				)
 			end
 		}
 	end
@@ -312,10 +297,9 @@ module SGXbwmsgsv2
 			)),
 			{'Content-Type' => 'application/json'},
 			[201]
-		).catch {
-			# TODO: add text; mention code number
+		).catch { |e|
 			EMPromise.reject(
-				[:cancel, 'internal-server-error']
+				[:cancel, 'internal-server-error', e.message]
 			)
 		}
 	end
@@ -418,8 +402,8 @@ module SGXbwmsgsv2
 				to_catapult_possible_oob(m, num_dest, *creds)
 			end
 		}.catch { |e|
-			if e.is_a?(Array) && e.length == 2
-				write_to_stream error_msg(m.reply, m.body, *e)
+			if e.is_a?(Array) && (e.length == 2 || e.length == 3)
+				write_to_stream m.as_error(e[1], e[0], e[2])
 			else
 				EMPromise.reject(e)
 			end
@@ -621,18 +605,16 @@ module SGXbwmsgsv2
 					phone_num
 				)
 			}
-		}.catch { |e|
-			EMPromise.reject(case e
+		}.catch_only(BandwidthError) { |e|
+			EMPromise.reject(case e.code
 			when 401
 				# TODO: add text re bad credentials
 				[:auth, 'not-authorized']
 			when 404
 				# TODO: add text re number not found or disabled
 				[:cancel, 'item-not-found']
-			when Integer
-				[:modify, 'not-acceptable']
 			else
-				e
+				[:modify, 'not-acceptable']
 			end)
 		}
 	end
@@ -726,7 +708,7 @@ module SGXbwmsgsv2
 			EMPromise.reject(:done)
 		end.catch { |e|
 			if e.is_a?(Array) && (e.length == 2 || e.length == 3)
-				write_to_stream error_msg(i.reply, qn, *e)
+				write_to_stream i.as_error(e[1], e[0], e[2])
 			elsif e != :done
 				EMPromise.reject(e)
 			end

test/test_component.rb 🔗

@@ -32,6 +32,10 @@ class ComponentTest < Minitest::Test
 		).element_name
 	end
 
+	def xmpp_error_text(error)
+		error.find_first("ns:text", ns: Blather::StanzaError::STANZA_ERR_NS)&.text
+	end
+
 	def process_stanza(s)
 		SGXbwmsgsv2.send(:client).receive_data(s)
 		raise $panic if $panic
@@ -62,7 +66,10 @@ class ComponentTest < Minitest::Test
 			text: "a"*4096,
 			applicationId: nil,
 			tag: " "
-		}).to_return(status: 400)
+		}).to_return(status: 400, body: JSON.dump(
+			description: "Bad text.",
+			fieldErrors: [{ description: "4096 not allowed" }]
+		))
 
 		m = Blather::Stanza::Message.new("+15551234567@component", "a"*4096)
 		m.from = "test@example.com"
@@ -76,6 +83,7 @@ class ComponentTest < Minitest::Test
 		error = stanza.find_first("error")
 		assert_equal "cancel", error["type"]
 		assert_equal "internal-server-error", xmpp_error_name(error)
+		assert_equal "Bad text. 4096 not allowed", xmpp_error_text(error)
 	end
 	em :test_message_too_long
 
@@ -251,6 +259,10 @@ class ComponentTest < Minitest::Test
 		error = stanza.find_first("error")
 		assert_equal "cancel", error["type"]
 		assert_equal "conflict", xmpp_error_name(error)
+		assert_equal(
+			"Another user exists for +15550000000",
+			xmpp_error_text(error)
+		)
 	end
 	em :test_ibr_conflict
 end