Switch back to default Faraday encoder and change list-records call to POST #96
Conversation
| @@ -21,9 +19,7 @@ def connection | |||
| headers: { | |||
| "Authorization" => "Bearer #{api_key}", | |||
| "User-Agent" => "Airrecord/#{Airrecord::VERSION}", | |||
| "X-API-VERSION" => "0.1.0", | |||
There was a problem hiding this comment.
Drive-by, this is no longer required and is out of date.
| }, | ||
| request: { params_encoder: Airrecord::QueryString } |
There was a problem hiding this comment.
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.
| @@ -33,7 +29,7 @@ def connection | |||
| end | |||
|
|
|||
| def escape(*args) | |||
| QueryString.escape(*args) | |||
| CGI.escape(*args) | |||
There was a problem hiding this comment.
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.
| 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' }) |
There was a problem hiding this comment.
The conversion to POST as per the notes in #95
|
Looks solid to me but I'm not in a position to test this library myself anymore. @theblang if you're confident I'll go ahead and merge? |
|
@sirupsen maybe you can do a quick smoke test on this? |
|
@Meekohi About to do some heavy smoke testing in our internal code that uses this PR's changes, so I'll report back once I'm finished! Update: Just finished a ton of internal testing that's using these changes, and it LGTM! |
|
yeah I don't have anything in production on airrrecord anymore to smoke test with unfortunately |
theblang
changed the title
|
I just noticed that Development section of the sidebar! So I removed "Fixes #95" from the commit message. But I think one of you will need to add it to Development? |
|
Sounds like I nominate @theblang to be the new maintainer ;D I'm gonna go ahead and merge. |
See comments below. I smoke tested:
all(with a single and multiple valuefieldsarray)find