Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Percents de-escaping isn't handled when no parameters are used in the dbapi. #608

Closed
jimfulton opened this issue Apr 14, 2021 · 4 comments · Fixed by #619
Closed

Percents de-escaping isn't handled when no parameters are used in the dbapi. #608

jimfulton opened this issue Apr 14, 2021 · 4 comments · Fixed by #619
Assignees
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. dbapi priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@jimfulton
Copy link
Contributor

>>> import google.cloud.bigquery.dbapi
>>> conn = google.cloud.bigquery.dbapi.connect()
/home/jim/p/g/python-bigquery/google/cloud/bigquery/client.py:444: UserWarning: Cannot create BigQuery Storage client, the dependency google-cloud-bigquery-storage is not installed.
  warnings.warn(
>>> cursor = conn.cursor()
>>> cursor.execute("select 'foo %%', %(x)s", dict(x=1))
>>> cursor.fetchall()
[Row(('foo %', 1), {'f0_': 0, 'f1_': 1})]
>>> cursor.execute("select 'foo %%'", dict(x=1))
>>> cursor.fetchall()
[Row(('foo %',), {'f0_': 0})]
>>> cursor.execute("select 'foo %%'")
>>> cursor.fetchall()
[Row(('foo %%',), {'f0_': 0})]

Bonus:

if people don't escape:

>>> cursor.execute("select 'foo %'")
>>> cursor.fetchall()
[Row(('foo %',), {'f0_': 0})]
>>> cursor.execute("select 'foo %'", dict(x=1))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/jim/p/g/python-bigquery/google/cloud/bigquery/dbapi/_helpers.py", line 273, in with_closed_check
    return method(self, *args, **kwargs)
  File "/home/jim/p/g/python-bigquery/google/cloud/bigquery/dbapi/cursor.py", line 172, in execute
    formatted_operation = _format_operation(operation, parameters=parameters)
  File "/home/jim/p/g/python-bigquery/google/cloud/bigquery/dbapi/cursor.py", line 460, in _format_operation
    return _format_operation_dict(operation, parameters)
  File "/home/jim/p/g/python-bigquery/google/cloud/bigquery/dbapi/cursor.py", line 434, in _format_operation_dict
    return operation % formatted_params
ValueError: unsupported format character ''' (0x27) at index 13
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Apr 14, 2021
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Apr 15, 2021
@plamut plamut added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Apr 16, 2021
@plamut plamut self-assigned this Apr 16, 2021
@jimfulton
Copy link
Contributor Author

FTR, I have a local fix. It's pretty simple. In cursor.py:

@@ -441,7 +461,7 @@ def _format_operation(operation, parameters=None):
             ``parameters`` argument.
     """
     if parameters is None or len(parameters) == 0:
-        return operation
+        return operation.replace("%%", "%")
 
     if isinstance(parameters, collections_abc.Mapping):

@plamut
Copy link
Contributor

plamut commented Apr 16, 2021

@jimfulton So the expected behavior is to always de-escape in absence of parameters, no matter what?

It might be breaking for some of the existing parameter-less queries that happen to contain %%, but it will at least be clear that de-escaping is consistently always applied.

@jimfulton
Copy link
Contributor Author

I think de-escaping has to be done consistently.

I tripped over this while testing the SQLAlchemy integration, which assumes consistent de-escaping.

@plamut
Copy link
Contributor

plamut commented Apr 16, 2021

Fair enough. We do need to assure a good changelog message, though (note to self when merging the PR).

gcf-merge-on-green bot pushed a commit that referenced this issue Apr 16, 2021
Fixes #608.

Percents in the query string are now always de-escaped, regardless of whether any query parameters are passed or not.

In addition, misformatting placeholders that don't match parameter values now consistently raise `ProgrammingError`.

**PR checklist:**
- [x] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/python-bigquery/issues/new/choose) before writing your code!  That way we can discuss the change, evaluate designs, and agree on the general idea
- [x] Ensure the tests and linter pass
- [x] Code coverage does not decrease (if any source code was changed)
- [x] Appropriate docs were updated (if necessary)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. dbapi priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants