Fix bool conversion Verify parameter in Tableau Hook

[ciancolo]

Hello all,
During the RC tests, I founded this bug (generated from my PR #16365). The bug is present when the user set in Extra parameter field in a Tableau Connection the parameter Verify equals to false or true. During the JSON parsing, the value is automatically read as bool, so the explicit conversion str to bool is not necessary.

[2021-07-21, 07:15:33 UTC] {taskinstance.py:1455} ERROR - Task failed with exception
Traceback (most recent call last):
  File "/opt/airflow/airflow/models/taskinstance.py", line 1182, in _run_raw_task
    self._prepare_and_execute_task_with_callbacks(context, task)
  File "/opt/airflow/airflow/models/taskinstance.py", line 1285, in _prepare_and_execute_task_with_callbacks
    result = self._execute_task(context, task_copy)
  File "/opt/airflow/airflow/models/taskinstance.py", line 1315, in _execute_task
    result = task_copy.execute(context=context)
  File "/opt/airflow/airflow/providers/tableau/operators/tableau_refresh_workbook.py", line 70, in execute
    with TableauHook(self.site_id, self.tableau_conn_id) as tableau_hook:
  File "/opt/airflow/airflow/providers/tableau/hooks/tableau.py", line 70, in __init__
    verify = bool(strtobool(verify))
  File "/usr/local/lib/python3.8/distutils/util.py", line 313, in strtobool
    val = val.lower()
AttributeError: 'bool' object has no attribute 'lower'

Code changes:

1. Added check type before conversion for Verify parameter
2. Added test in TableauHook tests to check this situation

Thank you

[uranusjr]

Not directly related to the issue, but distutils.util.strtobool is going to be deprecated in Python 3.10, so this is probably a good time as any to remove that function call entirely and implement our own boolean parsing logic.

[ciancolo]

Oh really?! I didn't know. Thank you for the hint.

I was thinking, what if we removed completely the conversion? I mean the admissible value for that Verify field with json standard are:

• true
• false
• string to the cert path

If the user will use for example "False" (as string) as value to Verify, in my opinion, the Tableau Hook should use it as a string and search the certificate in the path (of course it will fail). For me, it is not necessary to convert the parameter to bool in case the user uses it in the wrong way.

What do you think?

[uranusjr]

We can’t just remove functionality due to backward compatibility policy—we could deprecate it, but still need to keep the logic. So something like this is probably good enough:

verify = self.conn.extra_dejson.get('verify', True)
if isinstance(verify, number.Number):  # Also allow e.g. {"verify": 1}
    verify = bool(verify)
elif isinstance(verify, str):
    warnings.warn("blah blah parsing verify from a string is deprecated", DeprecationWarning)
    from distutils.util import strtobool
    verify = strtobool(verify)
else:
    raise TypeError(f"verify needs to be a number, not {verify!r}")

[github-actions]

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

[kaxil]

I am ok we do this in follow up PR too but we should definitely do it i.e replace strtobool

[eladkal]

We can’t just remove functionality due to backward compatibility policy—we could deprecate it, but still need to keep the logic

Do we need deprecation? it was added in #16365
so probably we don't need the deprecation note

[potiuk]

We can’t just remove functionality due to backward compatibility policy—we could deprecate it, but still need to keep the logic

Do we need deprecation? it was added in #16365
so probably we don't need the deprecation note

Yep. We have not released it yet. No deprecation needed.

[potiuk]

I am going to release an rc2 wave of July Providers shortly.

[potiuk]

Random failures. Merging.