francois2metz/steampipe-plugin-airtable

Feedback and review

e-gineer opened this issue · 6 comments

e-gineer commented

Hey @francois2metz ... really great work on this new type of plugin!

I installed it easily and played around. Here are some suggestions:

  • Provide an example in the config for table names, e.g. [ "Bugs and issues" ], it wasn't clear to be whether to do the Bugs%20and%20issues format instead.
  • Convert table names to snake case for easier querying? I ended up with table names like airtable_bugs%20_and%20_issues. I believe it would be better if these were simpler like airtable_bugs_and_issues.
  • Add created_time column to the records in table. {Name: "created_time", Type: proto.ColumnType_TIMESTAMP, Description: "Time when the record was created."},
  • Use go mod tidy to remove unneeded require statements from go.mod.
  • Add a GetConfig to the table table. It will be much more efficient when getting a single item and appears to be supported by the SDK.
  • Provide a SQL example in the docs for joining records across two tables.
  • Provide a SQL example in the docs for getting a single record by ID.
  • Provide a SQL example in the docs to get the 5 rows created most recently.

Airtable tables can get very large, and the API has good support for limits, filters, etc. I recommend:

  • Add support for d.QueryStatus.RowsRemaining() to stop paging queries etc once the query is cancelled or the number of rows has been reached. (New in Steampipe SDK v1.6.1).

Harder, but (mostly?) doable:

  • The metadata API would let us query all the databases, so we could add an airtable_base table (or similar).
  • The metadata API allows us to query the fields for each table. We could use that to create true column definitions from the airtable columns. This would make the tables much more familiar to airtable users and simpler to query. Imagine if the query was select name from airtable_design_projects, so much simpler than the JSON.
  • With specific columns in place, embrace optional key columns and URL parameters to API json query. This will allow for much better targeting of data rows - a huge performance gain.

We may have more later. Happy to discuss any of these or help! :-)

francois2metz commented

Thanks for the review. I'll work on it.

On the metadata API, there is 2 problems right now:

  1. I don't have access to the metadata API right now
  2. Querying the metadata API require an network request, not sure it will works well
francois2metz commented

I have done all the points (but the go mod tidy didn't have any effect) except the use of the metadata API.

cbruno10 commented

Hey @francois2metz , thanks for making the changes quickly! Sorry it took a while for me to get back to you with our other suggestions after some more testing. Please see below for additional suggestions, and let me know if you have any questions.

Connection Configuration

  • In airtable/connection_config.go and config/airtable.spc, can you please update databaseid to database_id, as we typically use snake case for config options.

Error Handling

  • When an item doesn't exist in a table, is it possible to handle the 404 error and return an empty row, instead of returning the error? Here's what we saw in some of our tests:
select id, created_time, jsonb_pretty(fields) as fields from airtable_features where id = 'recpICqN81XFoQJ61'

Error: status 404, err: HTTP request failure on /v0/appCJx0MKL2MsYjul/Features/recpICqN81XFoQJ61 with status 404
Body: {"error":{"type":"MODEL_ID_NOT_FOUND"}}

Similarly, when a table doesn't exist or the database ID provided in the config doesn't exist, are you able to handle these as well, e.g.,

select id, created_time, jsonb_pretty(fields) as fields from airtable_bugs_and_issues where id = 'recpICqN81XFoQJ61'

Error: status 404, err: HTTP request failure on /v0/appQLhYOg7THhtehb/Bugs%!a(MISSING)nd%!I(MISSING)ssues/recpICqN81XFoQJ61 with status 404
Body: {"error":{"type":"TABLE_NOT_FOUND","message":"Could not find table Bugs and Issues in application appQLhYOg7THhtehb"}}

select id, created_time, jsonb_pretty(fields) as fields from airtable_bugs_and_issues where id = 'recpICqN81XFoQJ61'

Error: status 404, err: HTTP request failure on /v0/appQLhYOg7THhteh/Bugs%!a(MISSING)nd%!I(MISSING)ssues/recpICqN81XFoQJ61 with status 404
Body: {"error":"NOT_FOUND"}

API Handling

    // API can return maximum 100 records in a single page
    maxResult := int64(100)

    limit := d.QueryContext.Limit
    if d.QueryContext.Limit != nil {
        if *limit < maxResult {
            maxResult = *limit
        }
    }

Examples

  • In docs/tables/airtable_table.md, in the last example, the last line has #>>'{}', is this meant to be part of the query?
francois2metz commented

Hi @cbruno10,

Thanks a lot for the review and the suggestions.

I did all the stuff above and I released the version 0.0.3.

Does Airtable's API support retry and backoff today? If so, is this already handled in the SDK or something you can add?

The SDK handle the rate limit. I never ran into an Airtable error (and I use this plugin quite often).

In docs/tables/airtable_table.md, in the last example, the last line has #>>'{}', is this meant to be part of the query?

Yes it is. It's a way to get the real string value out of a jsonb string field 🤯

cbruno10 commented

Thanks for the quick updates @francois2metz !

We recently released the CSV plugin (https://hub.steampipe.io/plugins/turbot/csv), and based off of that release, we have some additional suggestions for compatibility with the CLI + Hub:

  • Add SchemaMode: plugin.SchemaModeDynamic in the plugin function (like in https://github.com/turbot/steampipe-plugin-csv/blob/main/csv/plugin.go#L23), which allows the plugin to automatically reload the schema every time Steampipe starts (works with Steampipe CLI >= v0.9.0, set to release later this week, but backward compatible).
  • Update Steampipe SDK to v1.7.0, which contains the dynamic schema mode mentioned above
  • For the airtable_table.md, can you please update the filename, table name, and contents to match https://github.com/turbot/steampipe-plugin-csv/blob/main/docs/tables/%7Bcsv_filename%7D.md? These changes are meant to reflect the dynamic nature of the table. Some content in the linked example doc are specific to CSV files, so please change/exclude anything unrelated to your table.
  • Add og_image in docs/index.md, set to /images/plugins/francois2metz/airtable-social-graphic.png
  • Update brand_color in docs/index.md to #18BFFF

I believe that after these changes are we done, we'll be ready for publishing the initial release.

If you have any questions, please let me know!

francois2metz commented

All done! Closing