[CIVP-19296] Update Python Client to use CSVImports

civisanalytics / civis-python

Civis API Python Client

BSD 3-Clause "New" or "Revised" License

34 stars 26 forks source link

[CIVP-19296] Update Python Client to use CSVImports #328

Closed zacharydfriedlander closed 5 years ago

zacharydfriedlander commented 5 years ago

In this PR:

We migrate the convenience functions for imports in civis.io from using AutoImports via the /imports and /imports/syncs endpoints to using the newly created /imports/files/csv and /files/preprocess/csv endpoints. This opens up some new opportunities for users via these endpoints, such as upserts, multi-file imports, and importing with a subset of columns.
As part of developing these new capabilities, a number of the tests for the civis.io module are also refactored from using VCR cassettes to mocking API client calls directly.
We add a new get_storage_host_id method to the APIClient. This method makes easier interacting with the new Storage hosts abstraction, used by the new CSV imports to import directly from S3.

elsander commented 5 years ago

@zacharydfriedlander, can you pull in @leeacto for review as well?

Also tagging @patr1ckm for viz.

zacharydfriedlander commented 5 years ago

If y'all have any questions about the overall functionality of the imports/files/csv endpoint, these Zendesk docs might be helpful - should probably have included these from the outset!

zacharydfriedlander commented 5 years ago

I think this is ready for another round of review!

zacharydfriedlander commented 5 years ago

In manual testing, we discovered an issue with SQL types of different precisions being incorrectly rejected, e.g. VARCHAR(42) and VARCHAR(32) are rejected even though both are VARCHARs. The fix will be two-fold:

Add an option to loosen types as we do in AutoImports to CSVImports - that will require Platform side changes.
Loosen the strictness with which types are checked client-side. Rather than comparing types for string equality, we can look at just the base type.

Just wanted to give everyone a heads up here - I have opened a PR in my fork to address point 2, and will update as work progresses.

zacharydfriedlander commented 5 years ago

After a number of other things arose, I've finally been able to get back to this - the necessary Platform side changes have been made, and I have updated civis_file_to_table to leverage the new parameter added.

You can see a sample import produced by the new code here.