From a676702c5214770dc0969cf6ace1b647ea2db836 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Wed, 22 Jul 2015 07:40:23 +0100 Subject: [PATCH] Refactor constituency api to eliminate encoding errors Changes the api to do the following: * Follow redirect responses from the parliament api * Encode postcodes to eliminate invalid uri errors * Don't raise errors, just return [] or nil * Notify AppSignal if we get anything other than a 404 error --- app/controllers/local_petitions_controller.rb | 4 +- app/lib/constituency_api.rb | 128 ++++++++++++------ app/models/signature.rb | 5 +- .../local_petitions_controller_spec.rb | 2 +- spec/lib/constituency_api_spec.rb | 20 +-- spec/support/constituency_api_helpers.rb | 43 +++--- 6 files changed, 128 insertions(+), 74 deletions(-) diff --git a/app/controllers/local_petitions_controller.rb b/app/controllers/local_petitions_controller.rb index 48c4a96cf..3f1afcabc 100644 --- a/app/controllers/local_petitions_controller.rb +++ b/app/controllers/local_petitions_controller.rb @@ -22,9 +22,7 @@ def postcode? end def find_constituency - @constituency = ConstituencyApi::Client.constituency(@postcode) - rescue ConstituencyApi::Error => e - Rails.logger.error("Failed to fetch constituency - #{e}") + @constituency = ConstituencyApi.constituency(@postcode) end def constituency? diff --git a/app/lib/constituency_api.rb b/app/lib/constituency_api.rb index c218c2eb9..9e60e0b91 100644 --- a/app/lib/constituency_api.rb +++ b/app/lib/constituency_api.rb @@ -1,8 +1,8 @@ +require 'faraday' +require 'nokogiri' require 'postcode_sanitizer' module ConstituencyApi - class Error < RuntimeError; end - class Constituency attr_reader :id, :name, :mp @@ -18,73 +18,121 @@ def ==(other) class Mp attr_reader :id, :name, :start_date - URL = "http://www.parliament.uk/biographies/commons" + URL = "http://www.parliament.uk/biographies/commons/%{name}/%{id}" def initialize(id, name, start_date) @id, @name, @start_date = id, name, start_date.to_date end def url - "#{URL}/#{name.parameterize}/#{id}" + URL % { name: name.parameterize, id: id } end def ==(other) - other.is_a?(self.class) && id == other.id && name == other.name && start_date && other.start_date + other.is_a?(self.class) && id == other.id && name == other.name && start_date == other.start_date end alias_method :eql?, :== end class Client - include Faraday - URL = 'http://data.parliament.uk/membersdataplatform/services/mnis/Constituencies' + API_HOST = 'http://data.parliament.uk' + API_ENDPOINT = '/membersdataplatform/services/mnis/Constituencies/%{postcode}/' TIMEOUT = 5 - def self.constituency(postcode) - constituencies(postcode).first + def call(postcode) + faraday.get(path(postcode)) do |request| + request.options[:timeout] = TIMEOUT + request.options[:open_timeout] = TIMEOUT + end end - def self.constituencies(postcode) - response = call_api(postcode) - parse_constituencies(response) + private + + def faraday + Faraday.new(API_HOST) do |f| + f.response :follow_redirects + f.response :raise_error + f.adapter Faraday.default_adapter + end end - def self.parse_constituencies(response) - return [] unless response["Constituencies"] - constituencies = response["Constituencies"]["Constituency"] - Array.wrap(constituencies).map { |c| Constituency.new(c["Constituency_Id"], c["Name"], last_mp(c)) } + def path(postcode) + API_ENDPOINT % { postcode: escape_path(postcode) } end - def self.call_api(postcode) - sanitized_postcode = PostcodeSanitizer.call(postcode) - response = Faraday.new(URL).get("#{sanitized_postcode}/") do |req| - req.options[:timeout] = TIMEOUT - req.options[:open_timeout] = TIMEOUT - end - unless response.status == 200 - raise Error.new("Unexpected response from API:"\ - "status #{response.status}"\ - "body #{response.body}"\ - "request #{URL}/#{sanitized_postcode}/") + def escape_path(value) + Rack::Utils.escape_path(value) + end + end + + class Query + CONSTITUENCIES = '//Constituencies/Constituency' + CONSTITUENCY_ID = './Constituency_Id' + CONSTITUENCY_NAME = './Name' + + CURRENT_MP = './RepresentingMembers/RepresentingMember[1]' + MP_ID = './Member_Id' + MP_NAME = './Member' + MP_DATE = './StartDate' + + def initialize(postcode) + @postcode = postcode + end + + def fetch + response = client.call(postcode) + + if response.success? + parse(response.body) + else + [] end - Hash.from_xml(response.body) - rescue Faraday::Error::TimeoutError - raise Error.new("Timeout after #{TIMEOUT} seconds") + rescue Faraday::Error::ResourceNotFound => e + return [] rescue Faraday::Error => e - raise Error.new("Network error - #{e}") + Appsignal.send_exception(e) if defined?(Appsignal) + return [] end - def self.last_mp(constituency_hash) - mps = parse_mps(constituency_hash) - mps.select(&:start_date).sort_by(&:start_date).last + private + + def client + @client ||= Client.new end - def self.parse_mps(response) - return [] unless response["RepresentingMembers"] - mps = response["RepresentingMembers"]["RepresentingMember"] - Array.wrap(mps).map { |m| Mp.new(m["Member_Id"], m["Member"], m["StartDate"]) } + def postcode + PostcodeSanitizer.call(@postcode) end - private_class_method :parse_constituencies, :call_api, :parse_mps, :last_mp + def parse(body) + xml = Nokogiri::XML(body) + + xml.xpath(CONSTITUENCIES).map do |node| + id = node.xpath(CONSTITUENCY_ID).text + name = node.xpath(CONSTITUENCY_NAME).text + + if mp = node.at_xpath(CURRENT_MP) + Constituency.new(id, name, parse_mp(mp)) + else + Constituency.new(id, name) + end + end + end + + def parse_mp(node) + id = node.xpath(MP_ID).text + name = node.xpath(MP_NAME).text + date = node.xpath(MP_DATE).text + + Mp.new(id, name, date) + end + end + + def self.constituency(postcode) + constituencies(postcode).first end -end + def self.constituencies(postcode) + Query.new(postcode).fetch + end +end diff --git a/app/models/signature.rb b/app/models/signature.rb index 55dfd4438..ced54a17a 100644 --- a/app/models/signature.rb +++ b/app/models/signature.rb @@ -116,10 +116,7 @@ def unsubscribe! end def constituency - @constituency ||= ConstituencyApi::Client.constituencies(self.postcode).first - rescue ConstituencyApi::Error => e - Rails.logger.error("Failed to fetch constituency - #{e}") - nil + @constituency ||= ConstituencyApi.constituency(postcode) end def set_constituency_id diff --git a/spec/controllers/local_petitions_controller_spec.rb b/spec/controllers/local_petitions_controller_spec.rb index 9945897a4..5203dc0ae 100644 --- a/spec/controllers/local_petitions_controller_spec.rb +++ b/spec/controllers/local_petitions_controller_spec.rb @@ -68,7 +68,7 @@ shared_examples_for 'a local petitions controller that does not try to lookup a constituency' do it 'does not communicate with the API' do - expect(ConstituencyApi::Client).not_to receive(:constituency) + expect(ConstituencyApi).not_to receive(:constituency) get :index, params end diff --git a/spec/lib/constituency_api_spec.rb b/spec/lib/constituency_api_spec.rb index 6c65f685b..7c0520ad2 100644 --- a/spec/lib/constituency_api_spec.rb +++ b/spec/lib/constituency_api_spec.rb @@ -14,13 +14,13 @@ let(:mp) { ConstituencyApi::Mp.new("1536", "Emily Thornberry MP", Date.new(2015, 5, 7)) } it "returns the URL for the mp" do - expect(mp.url).to eq "#{ConstituencyApi::Mp::URL}/emily-thornberry-mp/1536" + expect(mp.url).to eq "http://www.parliament.uk/biographies/commons/emily-thornberry-mp/1536" end end end describe "query methods" do - let(:api) { ConstituencyApi::Client } + let(:api) { ConstituencyApi } let(:url) { "http://data.parliament.uk" } let(:endpoint) { "#{url}/membersdataplatform/services/mnis/Constituencies" } @@ -162,8 +162,8 @@ def constituency(id, name, mp = nil, &block) stub_request(:get, /.*data.parliament.uk.*/).to_timeout end - it "raises a ConstituencyApi::Error" do - expect{ api.constituencies("N1") }.to raise_error(ConstituencyApi::Error) + it "returns an empty array" do + expect(api.constituencies("N1")).to eq([]) end end @@ -172,8 +172,8 @@ def constituency(id, name, mp = nil, &block) stub_request(:get, /.*data.parliament.uk.*/).to_raise(Faraday::Error::ConnectionFailed) end - it "raises a ConstituencyApi::Error" do - expect{ api.constituencies("N1") }.to raise_error(ConstituencyApi::Error) + it "returns an empty array" do + expect(api.constituencies("N1")).to eq([]) end end @@ -182,8 +182,8 @@ def constituency(id, name, mp = nil, &block) stub_request(:get, /.*data.parliament.uk.*/).to_raise(Faraday::Error::ResourceNotFound) end - it "raises a ConstituencyApi::Error" do - expect{ api.constituencies("N1") }.to raise_error(ConstituencyApi::Error) + it "returns an empty array" do + expect(api.constituencies("N1")).to eq([]) end end @@ -192,8 +192,8 @@ def constituency(id, name, mp = nil, &block) stub_request(:get, /.*data.parliament.uk.*/).to_return(status: 500, body: "") end - it "raises a ConstituencyApi::Error" do - expect{ api.constituencies("N1") }.to raise_error(ConstituencyApi::Error) + it "returns an empty array" do + expect(api.constituencies("N1")).to eq([]) end end end diff --git a/spec/support/constituency_api_helpers.rb b/spec/support/constituency_api_helpers.rb index 3f2646bdb..99977270a 100644 --- a/spec/support/constituency_api_helpers.rb +++ b/spec/support/constituency_api_helpers.rb @@ -9,37 +9,48 @@ def stub_constituency(postcode, *constituency_params) else ConstituencyApi::Constituency.new(*constituency_params) end - allow(ConstituencyApi::Client).to receive(:constituencies). - with(PostcodeSanitizer.call(postcode)). - and_return([constituency]) + allow(ConstituencyApi).to receive(:constituencies). + with(PostcodeSanitizer.call(postcode)). + and_return([constituency]) end def stub_constituencies(partial_postcode, *constituencies) - allow(ConstituencyApi::Client).to receive(:constituencies). - with(PostcodeSanitizer.call(partial_postcode)). - and_return(constituencies) + allow(ConstituencyApi).to receive(:constituencies). + with(PostcodeSanitizer.call(partial_postcode)). + and_return(constituencies) end def stub_no_constituencies(postcode) - allow(ConstituencyApi::Client).to receive(:constituencies). - with(PostcodeSanitizer.call(postcode)). - and_return([]) + allow(ConstituencyApi).to receive(:constituencies). + with(PostcodeSanitizer.call(postcode)). + and_return([]) end def stub_broken_api - allow(ConstituencyApi::Client).to receive(:constituencies). - and_raise(ConstituencyApi::Error) + allow(ConstituencyApi).to receive(:constituencies).and_return([]) end end module NetworkLevel - def api_url - ConstituencyApi::Client::URL + def api_host + ConstituencyApi::Client::API_HOST + end + + def api_endpoint + ConstituencyApi::Client::API_ENDPOINT + end + + def api_url(postcode) + "#{api_host}#{api_endpoint}" % { postcode: sanitize_and_escape_postcode(postcode) } + end + + def sanitize_and_escape_postcode(postcode) + Rack::Utils.escape_path(PostcodeSanitizer.call(postcode)) end def stub_constituency_from_file(postcode, filename) - stub_request(:get, "#{ api_url }/#{PostcodeSanitizer.call(postcode)}/").to_return(status: 200, body: IO.read(filename)) + stub_request(:get, api_url(postcode)).to_return(status: 200, body: IO.read(filename)) end def stub_constituency(postcode, constituency_id, constituency_name, mp_id: '0001', mp_name: 'A. N. Other MP', mp_start_date: '2015-05-07T00:00:00') @@ -61,7 +72,7 @@ def stub_constituency(postcode, constituency_id, constituency_name, mp_id: '0001 RESPONSE - stub_request(:get, "#{ api_url }/#{PostcodeSanitizer.call(postcode)}/").to_return(status: 200, body: api_response) + stub_request(:get, api_url(postcode)).to_return(status: 200, body: api_response) end def stub_no_constituencies(postcode) @@ -69,7 +80,7 @@ def stub_no_constituencies(postcode) end def stub_broken_api - stub_request(:get, %r[#{ Regexp.escape(api_url) }/*]).to_return(status: 500) + stub_request(:get, %r[#{api_host}/*]).to_return(status: 500) end end end