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

Jump to bottom

Switch back to default Faraday encoder and change list-records call to POST #96

Merged
merged 1 commit into from
Jan 11, 2023
Merged

Switch back to default Faraday encoder and change list-records call to POST #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 1 addition & 5 deletions lib/airrecord/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
require 'uri'
require_relative 'query_string'
require_relative 'faraday_rate_limiter'

module Airrecord
Expand All @@ -21,9 +19,7 @@ def connection
headers: {
"Authorization" => "Bearer #{api_key}",
"User-Agent" => "Airrecord/#{Airrecord::VERSION}",
"X-API-VERSION" => "0.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drive-by, this is no longer required and is out of date.

},
request: { params_encoder: Airrecord::QueryString }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Airtable expects that arrays in query strings be encoded with indices.

Turns out this isn't the case anymore (checked various examples in the API docs). And interestingly, it caused a bizarre issue with our list-records call where increasing the fields param count to about 22+ (which is still well under the 16k request character limit) would cause a 422 Invalid request: parameter validation failed. Check your request data.. So I just gutted Airrecord::QueryString to switch back to the default Faraday encoding, which is ?foo[]=1&foo[]=2 (interesting side note, they have actually added the ability to specify indexes in the encoding). After switching, our list-records call started working again.

Superseding all of this is the fact that this PR is switching the list-records call to POST, which also fixes our issue. But seems worth it to stop maintaining a custom encoder if we don't have to.

) do |conn|
if Airrecord.throttle?
conn.request :airrecord_rate_limiter, requests_per_second: AIRTABLE_RPS_LIMIT
Expand All @@ -33,7 +29,7 @@ def connection
end

def escape(*args)
QueryString.escape(*args)
CGI.escape(*args)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to be URI.escape before the change to our custom encoder. That no longer exists, but CGI.escape seems appropriate for our use here. See this SO answer.

end

def parse(body)
Expand Down
45 changes: 0 additions & 45 deletions lib/airrecord/query_string.rb
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions lib/airrecord/table.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ def records(filter: nil, sort: nil, view: nil, offset: nil, paginate: true, fiel
options[:maxRecords] = max_records if max_records
options[:pageSize] = page_size if page_size

path = "/v0/#{base_key}/#{client.escape(table_name)}"
response = client.connection.get(path, options)
path = "/v0/#{base_key}/#{client.escape(table_name)}/listRecords"
response = client.connection.post(path, options.to_json, { 'Content-Type' => 'application/json' })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The conversion to POST as per the notes in #95

parsed_response = client.parse(response.body)

if response.success?
Expand Down
8 changes: 4 additions & 4 deletions test/associations_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def test_has_many_associations
{ "id" => "rec2", "Name" => "Good brew" },
{ "id" => "rec1", "Name" => "Decent brew" }
]
stub_request(brews, table: Brew)
stub_records_request(brews, table: Brew)

assert_equal 2, tea.brews.size
assert_kind_of Airrecord::Table, tea.brews.first
Expand All @@ -50,7 +50,7 @@ def test_has_many_associations

def test_has_many_handles_empty_associations
tea = Tea.new("Name" => "Gunpowder")
stub_request([{ "id" => "brew1", "Name" => "unrelated" }], table: Brew)
stub_records_request([{ "id" => "brew1", "Name" => "unrelated" }], table: Brew)
assert_equal 0, tea.brews.size
end

Expand Down Expand Up @@ -82,7 +82,7 @@ def test_build_association_from_strings

tea.create

stub_request([{ id: "rec2" }, { id: "rec1" }], table: Brew)
stub_records_request([{ id: "rec2" }, { id: "rec1" }], table: Brew)
assert_equal 2, tea.brews.count
end

Expand Down Expand Up @@ -110,7 +110,7 @@ def test_build_has_many_association_from_setter
tea.brews = brews

brew_fields = brews.map { |brew| brew.fields.merge("id" => brew.id) }
stub_request(brew_fields, table: Brew)
stub_records_request(brew_fields, table: Brew)

assert_equal 2, tea.brews.size
assert_kind_of Airrecord::Table, tea.brews.first
Expand Down
70 changes: 0 additions & 70 deletions test/query_string_test.rb
View file
Open in desktop

This file was deleted.

24 changes: 12 additions & 12 deletions test/table_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def setup
builder.adapter :test, @stubs
}

stub_request([{"Name" => "omg", "Notes" => "hello world"}, {"Name" => "more", "Notes" => "walrus"}])
stub_records_request([{"Name" => "omg", "Notes" => "hello world"}, {"Name" => "more", "Notes" => "walrus"}])
end

def test_table_overrides_key
Expand All @@ -48,41 +48,41 @@ def test_different_clients_with_different_api_keys
end

def test_filter_records
stub_request([{"Name" => "yes"}, {"Name" => "no"}])
stub_records_request([{"Name" => "yes"}, {"Name" => "no"}])

records = @table.records(filter: "Name")
assert_equal "yes", records[0]["Name"]
end

def test_sort_records
stub_request([{"Name" => "a"}, {"Name" => "b"}])
stub_records_request([{"Name" => "a"}, {"Name" => "b"}])

records = @table.records(sort: { "Name" => 'asc' })
assert_equal "a", records[0]["Name"]
assert_equal "b", records[1]["Name"]
end

def test_view_records
stub_request([{"Name" => "a"}, {"Name" => "a"}])
stub_records_request([{"Name" => "a"}, {"Name" => "a"}])

records = @table.records(view: 'A')
assert_equal "a", records[0]["Name"]
assert_equal "a", records[1]["Name"]
end

def test_follow_pagination_by_default
stub_request([{"Name" => "1"}, {"Name" => "2"}], offset: 'dasfuhiu')
stub_request([{"Name" => "3"}, {"Name" => "4"}], offset: 'odjafio', clear: false)
stub_request([{"Name" => "5"}, {"Name" => "6"}], clear: false)
stub_records_request([{"Name" => "1"}, {"Name" => "2"}], offset: 'dasfuhiu')
stub_records_request([{"Name" => "3"}, {"Name" => "4"}], offset: 'odjafio', clear: false)
stub_records_request([{"Name" => "5"}, {"Name" => "6"}], clear: false)

records = @table.records
assert_equal 6, records.size
end

def test_dont_follow_pagination_if_disabled
stub_request([{"Name" => "1"}, {"Name" => "2"}], offset: 'dasfuhiu')
stub_request([{"Name" => "3"}, {"Name" => "4"}], offset: 'odjafio', clear: false)
stub_request([{"Name" => "5"}, {"Name" => "6"}], clear: false)
stub_records_request([{"Name" => "1"}, {"Name" => "2"}], offset: 'dasfuhiu')
stub_records_request([{"Name" => "3"}, {"Name" => "4"}], offset: 'odjafio', clear: false)
stub_records_request([{"Name" => "5"}, {"Name" => "6"}], clear: false)

records = @table.records(paginate: false)
assert_equal 2, records.size
Expand Down Expand Up @@ -314,7 +314,7 @@ def test_find_many
end

def test_find_many_makes_no_network_call_when_ids_are_empty
stub_request([], status: 500)
stub_records_request([], status: 500)

assert_equal([], @table.find_many([]))
end
Expand Down Expand Up @@ -353,7 +353,7 @@ def test_error_handles_errors_without_body
end

def test_dates_are_not_type_casted
stub_request([{"Name" => "omg", "Created" => Time.now.to_s}])
stub_records_request([{"Name" => "omg", "Created" => Time.now.to_s}])

record = first_record
assert_instance_of String, record["Created"]
Expand Down
6 changes: 3 additions & 3 deletions test/test_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def stub_patch_request(record, updated_keys, table: @table, status: 200, headers
end

# TODO: Problem, can't stub on params.
def stub_request(records, table: @table, status: 200, headers: {}, offset: nil, clear: true)
def stub_records_request(records, table: @table, status: 200, headers: {}, offset: nil, clear: true)
@stubs.instance_variable_set(:@stack, {}) if clear

body = {
Expand All @@ -59,7 +59,7 @@ def stub_request(records, table: @table, status: 200, headers: {}, offset: nil,
offset: offset,
}.to_json

@stubs.get("/v0/#{table.base_key}/#{table.table_name}") do |env|
@stubs.post("/v0/#{table.base_key}/#{table.table_name}/listRecords") do |env|
[status, headers, body]
end
end
Expand Down Expand Up @@ -87,7 +87,7 @@ def stub_error_request(type:, message:, status: 401, headers: {}, table: @table)
}
}.to_json

@stubs.get("/v0/#{table.base_key}/#{table.table_name}") do |env|
@stubs.post("/v0/#{table.base_key}/#{table.table_name}/listRecords") do |env|
[status, headers, body]
end
end
Expand Down