search

civisanalytics / civis-python

Civis API Python Client
BSD 3-Clause "New" or "Revised" License
34 stars 26 forks source link

get_table_id does not correctly handle quoted schemas and tablenames #276

Closed jacksonllee closed 5 years ago

jacksonllee commented 5 years ago

The get_table_id function throws a ValueError if the schema and tablename are quoted, which doesn't seem to be the correct behavior.

Civis library version

1.9.2

How to reproduce the bug

In [1]: import civis

In [2]: client = civis.APIClient()

In [3]: client.get_table_id(table='foo.bar', database='baz')
Out[3]: 123

In [4]: client.get_table_id(table='"foo"."bar"', database='baz')
ValueError: Given table "foo"."bar" is not an exact match for returned table foo.bar.

Desired behavior

In [1]: import civis

In [2]: client = civis.APIClient()

In [3]: client.get_table_id(table='foo.bar', database='baz')
Out[3]: 123

In [4]: client.get_table_id(table='"foo"."bar"', database='baz')
Out[4]: 123

Likely cause/fix

While get_table_id derives the schema and tablename via civis.io.split_schema_tablename:

https://github.com/civisanalytics/civis-python/blob/eb5d46c8ce14f1bb87b889b69c7fd9e1c751d777/civis/civis.py#L271

which (correctly) gives us 'foo' as the schema and 'bar' as the tablename (both without quotes) with '"foo"."bar"' (with quotes) as the input, it looks like the following bit (whose purpose is unclear to me) can be removed for the desired behavior:

https://github.com/civisanalytics/civis-python/blob/eb5d46c8ce14f1bb87b889b69c7fd9e1c751d777/civis/civis.py#L277-L280

That being said, perhaps there's more to it, and the current maintainers would probably know more.

stephen-hoover commented 5 years ago

My guess is that the if table != found_table section was guarding against errors in schema/tablename separation. That code has been there since the initial release. We'd originally been splitting on periods to separate those sections, and that's fragile. Accounting for quoted identifiers was a more recent feature (https://github.com/civisanalytics/civis-python/pull/220). It looks like the if table != found_table check is no longer needed.