Skip to content
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.

Sign up for GitHub

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

ADF-3639 Ruby: Report on client agent + versions #40

Merged
merged 2 commits into from
Sep 4, 2024
Merged

ADF-3639 Ruby: Report on client agent + versions #40

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
426aca6
ADF-3639 Ruby: Report on client agent + versions
Yousuf-H Sep 3, 2024
17cc256
ADF-3639 PR fix
Yousuf-H Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions lib/addressfinder/address_autocomplete.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,7 @@ def encoded_params
end

def execute_request
request = Net::HTTP::Get.new(request_uri)

response = http.request(request)
response = http.request(request_uri)

self.response_body = response.body
self.response_status = response.code
Expand Down
4 changes: 1 addition & 3 deletions lib/addressfinder/address_info.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,7 @@ def encoded_params
end

def execute_request
request = Net::HTTP::Get.new(request_uri)

response = http.request(request)
response = http.request(request_uri)

self.response_body = response.body
self.response_status = response.code
Expand Down
4 changes: 1 addition & 3 deletions lib/addressfinder/address_search.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,7 @@ def encoded_params
end

def execute_request
request = Net::HTTP::Get.new(request_uri)

response = http.request(request)
response = http.request(request_uri)

self.response_body = response.body
self.response_status = response.code
Expand Down
9 changes: 9 additions & 0 deletions lib/addressfinder/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ class Configuration
attr_accessor :retries
attr_accessor :retry_delay

attr_reader :ca
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you are trying to prevent modification of the ca by customers but this is Ruby so everything can be changed ;) We do not care too much if customers override the ca value anyway so I think you can go back to attr_accessor :ca here and make the code fit the existing form nicely. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just wondering if we let the customers modify the client_agent does that make the data collected about which clients or versions are making API requests unreliable? I thought the ca returned the Addressfinder ruby gem version and not a ruby version? Maybe I misunderstood you?

"Ruby/#{AddressFinder::VERSION}"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Customer's can talk to the API directly and in that case they will not provide ca anyway so we are happy with the numbers being semi accurate. We are after ballpark figures and do not care much if a customer or two do not provide accurate client agents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay sweet, I'll make the changes 👍🏽


def initialize
self.hostname = 'api.addressfinder.io'
self.port = 443
Expand All @@ -23,6 +25,13 @@ def initialize
self.retry_delay = 5
self.default_country = 'nz'
self.verification_version = 'v1'
self.ca = "Ruby/#{AddressFinder::VERSION}"
end

private

def ca=(value)
@ca = value
end
end
end
22 changes: 20 additions & 2 deletions lib/addressfinder/http.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,16 @@ def start(&block)
end
end

def request(args)
def request(request_uri)
raise ArgumentError, "request_uri is nil" if request_uri.nil?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we checking for nil here? We did not use to do it before plus we are not checking all edge cases such as empty string so maybe I am missing something here.

Passing nil to URI() will blow up later down the road so the added value of this check is small? I'm keen on hearing your opinion Yousuf.

Copy link
Contributor Author

@Yousuf-H Yousuf-H Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right, looking into it; URI has a raise Argument error it throws. I added the request_uri is nil as an early exit strategy but since we are not checking all edge cases as you said we might as well let the URI handle the nils as well.

  def URI(uri)
    if uri.is_a?(URI::Generic)
      uri
    elsif uri = String.try_convert(uri)
      URI.parse(uri)
    else
      raise ArgumentError,
        "bad argument (expected URI object or URI string)"
    end
  end

retries = 0
begin
re_establish_connection if @connection_is_bad
net_http.request(args)

uri = build_uri(request_uri)
request = Net::HTTP::Get.new(uri)

net_http.request(request)
rescue Net::ReadTimeout, Net::OpenTimeout, SocketError => error
if retries < config.retries
retries += 1
Expand All @@ -40,6 +45,19 @@ def re_establish_connection
net_http.start
end

def build_uri(request_uri)
uri = URI(request_uri)
encoded_ca = URI.encode_www_form_component(config.ca)

if uri.query
uri.query += "&ca=#{encoded_ca}"
else
uri.query = "ca=#{encoded_ca}"
end

uri.to_s
end

def net_http
@net_http ||= begin
http = Net::HTTP.new(config.hostname, config.port, config.proxy_host,
Expand Down
4 changes: 1 addition & 3 deletions lib/addressfinder/location_info.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,7 @@ def encoded_params
end

def execute_request
request = Net::HTTP::Get.new(request_uri)

response = http.request(request)
response = http.request(request_uri)

self.response_body = response.body
self.response_status = response.code
Expand Down
4 changes: 1 addition & 3 deletions lib/addressfinder/location_search.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,7 @@ def encoded_params
end

def execute_request
request = Net::HTTP::Get.new(request_uri)

response = http.request(request)
response = http.request(request_uri)

self.response_body = response.body
self.response_status = response.code
Expand Down
22 changes: 10 additions & 12 deletions lib/addressfinder/v1/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,33 +20,31 @@ def perform
build_request
execute_request
build_result

self
end

private

attr_reader :request_uri, :params, :http, :path
attr_accessor :response_body, :response_status
attr_writer :result

def build_request
@request_uri = "#{path}?#{encoded_params}"
end

def encoded_params
Util.encode_and_join_params(params)
end

def execute_request
request = Net::HTTP::Get.new(request_uri)

response = http.request(request)

response = http.request(request_uri)

self.response_body = response.body
self.response_status = response.code
end

def build_result
case response_status
when '200'
Expand All @@ -55,15 +53,15 @@ def build_result
raise AddressFinder::RequestRejectedError.new(response_status, response_body)
end
end

def response_hash
@_response_hash ||= MultiJson.load(response_body)
end

def config
@_config ||= AddressFinder.configuration
end

class Result < OpenStruct
end
end
Expand Down
4 changes: 1 addition & 3 deletions lib/addressfinder/v2/au/verification.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,7 @@ def build_request
end

def execute_request
request = Net::HTTP::Get.new(request_uri)

response = http.request(request)
response = http.request(request_uri)

self.response_body = response.body
self.response_status = response.code
Expand Down
4 changes: 1 addition & 3 deletions lib/addressfinder/verification.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,7 @@ def build_request
end

def execute_request
request = Net::HTTP::Get.new(request_uri)

response = http.request(request)
response = http.request(request_uri)

self.response_body = response.body
self.response_status = response.code
Expand Down
13 changes: 13 additions & 0 deletions spec/lib/addressfinder/configuration_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
require 'spec_helper'

RSpec.describe AddressFinder::Configuration do
it 'sets the client agent with the gem version' do
config = AddressFinder::Configuration.new
expect(config.ca).to eq("Ruby/#{AddressFinder::VERSION}")
end

it 'does not allow the client agent to be modified' do
config = AddressFinder::Configuration.new
expect { config.ca = "CustomAgent/1.0" }.to raise_error(NoMethodError)
end
end
19 changes: 19 additions & 0 deletions spec/lib/addressfinder/http_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
require 'spec_helper'

RSpec.describe AddressFinder::HTTP do
let(:config) { AddressFinder::Configuration.new }
let(:http) { described_class.new(config) }
let(:request_uri) { "https://api.addressfinder.io?param=value" }

describe '#build_uri' do
it 'appends the ca parameter to the query string' do
uri_with_ca = http.send(:build_uri, request_uri)
expect(uri_with_ca).to include("ca=Ruby%2F#{AddressFinder::VERSION}")
end

it 'preserves existing query parameters' do
uri_with_ca = http.send(:build_uri, request_uri)
expect(uri_with_ca).to include("param=value")
end
end
end
6 changes: 4 additions & 2 deletions spec/lib/addressfinder_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
end

it "safely passes arguments through" do
stub_request(:get, Addressable::Template.new("https://api.addressfinder.io/api/nz/address/verification{?args*}")).to_return(:status => 200, :body => "{}", :headers => {})
nz_verification_endpoint = "https://api.addressfinder.io/api/nz/address/verification"
stub_request(:get, /\A#{nz_verification_endpoint}/).to_return(:status => 200, :body => "{}", :headers => {})
subject
end
end
Expand All @@ -44,7 +45,8 @@
end

it "safely passes arguments through" do
stub_request(:get, Addressable::Template.new("https://api.addressfinder.io/api/au/address/v2/verification{?args*}")).to_return(:status => 200, :body => "{}", :headers => {})
au_verification_endpoint = "https://api.addressfinder.io/api/au/address/v2/verification"
stub_request(:get, /\A#{au_verification_endpoint}/).to_return(:status => 200, :body => "{}", :headers => {})
subject
end
end
Expand Down
Loading