Repository: trinodb/trino-python-client
Pull request #263 (2022-10-13 12:39:54)

Checking for config headers to be both present and non-empty
<!-- Thank you for submitting a pull request! Find more information in our development guide at https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md and contact us on #python-client in Slack. -->
<!-- Provide an overview of the PR for maintainers and reviewers. -->
Addresses #262 .

Handles cases where these config headers are present in the request, but with an empty string as their value. Currently, an error is raised when trying to parse the empty strings. This PR addresses this by checking that these headers are not only present, but not empty.

These headers being present with empty values is not normally expected, although we'd like to protect against it for cases like a proxy sitting in between Trino and the client that is altering the headers (which is the case for me).

Notes: 
- Added checks for all headers in `process` that might hit this issue because the values are parsed. HEADER_SET_SESSION, HEADER_SET_ROLE and HEADER_ADDED_PREPARE are currently failing to parse if the header value is empty
- The test reproduces the error, but I don't fully understand the mocking that is happening. Looking for guidance if there is a different preferred way of testing this

<!-- Provide a user-friendly explanation, keep it brief if it isn't user-visible. -->
## Non-technical explanation
Protecting against headers being present but without data


<!-- Mark the appropriate option with an (x). Propose a release note if you can. -->
## Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

```markdown
* Fix value error with present but empty headers. ([#262](https://github.com/trinodb/trino-python-client/issues/262))
```


cla-bot[bot]: Thank you for your pull request and welcome to the Trino community. We require contributors to sign our [Contributor License Agreement](https://github.com/trinodb/cla/raw/master/Trino%20Foundation%20Individual%20CLA.pdf), and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla
cla-bot[bot]: Thank you for your pull request and welcome to the Trino community. We require contributors to sign our [Contributor License Agreement](https://github.com/trinodb/cla/raw/master/Trino%20Foundation%20Individual%20CLA.pdf), and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla
derek-pyne: Morning! Just checking in if this is a change we'd like to include, or something we want to discuss more :) 
hashhar: @derek-pyne I've been travelling so did not get a chance to re-review/merge. I'll get to this starting the weekend/Monday.
hashhar: @cla-bot check
cla-bot[bot]: The cla-bot has been summoned, and re-checked this pull request!
derek-pyne: > @derek-pyne I've been travelling so did not get a chance to re-review/merge. I'll get to this starting the weekend/Monday.
> 
> Sorry about the long wait.

All good! No rush, just checking in
hashhar: Squashed the commits. Will merge once CI is done. Thanks for the fix.

Patch:
diff --git a/trino/client.py b/trino/client.py
index 0073b3e..18e81c9 100644
--- a/trino/client.py
+++ b/trino/client.py
@@ -225,7 +225,7 @@ def get_session_property_values(headers, header):
     kvs = get_header_values(headers, header)
     return [
         (k.strip(), urllib.parse.unquote_plus(v.strip()))
-        for k, v in (kv.split("=", 1) for kv in kvs)
+        for k, v in (kv.split("=", 1) for kv in kvs if kv)
     ]
 
 
@@ -233,7 +233,7 @@ def get_prepared_statement_values(headers, header):
     kvs = get_header_values(headers, header)
     return [
         (k.strip(), urllib.parse.unquote_plus(v.strip()))
-        for k, v in (kv.split("=", 1) for kv in kvs)
+        for k, v in (kv.split("=", 1) for kv in kvs if kv)
     ]
 
 
@@ -241,7 +241,7 @@ def get_roles_values(headers, header):
     kvs = get_header_values(headers, header)
     return [
         (k.strip(), urllib.parse.unquote_plus(v.strip()))
-        for k, v in (kv.split("=", 1) for kv in kvs)
+        for k, v in (kv.split("=", 1) for kv in kvs if kv)
     ]
 
 


Test patch:
diff --git a/tests/unit/test_http.py b/tests/unit/test_http.py
index 9753bab..b19b11b 100644
--- a/tests/unit/test_http.py
+++ b/tests/unit/test_http.py
@@ -11,7 +11,12 @@
 # limitations under the License.
 
 from trino import constants
-from trino.client import get_header_values, get_session_property_values
+from trino.client import (
+    get_header_values,
+    get_prepared_statement_values,
+    get_roles_values,
+    get_session_property_values,
+)
 
 
 def test_get_header_values():
@@ -24,3 +29,21 @@ def test_get_session_property_values():
     headers = {constants.HEADER_SET_SESSION: "a=1, b=2, c=more%3Dv1%2Cv2"}
     values = get_session_property_values(headers, constants.HEADER_SET_SESSION)
     assert values == [("a", "1"), ("b", "2"), ("c", "more=v1,v2")]
+
+
+def test_get_session_property_values_ignores_empty_values():
+    headers = {constants.HEADER_SET_SESSION: ""}
+    values = get_session_property_values(headers, constants.HEADER_SET_SESSION)
+    assert len(values) == 0
+
+
+def test_get_prepared_statement_values_ignores_empty_values():
+    headers = {constants.HEADER_SET_SESSION: ""}
+    values = get_prepared_statement_values(headers, constants.HEADER_SET_SESSION)
+    assert len(values) == 0
+
+
+def test_get_roles_values_ignores_empty_values():
+    headers = {constants.HEADER_SET_SESSION: ""}
+    values = get_roles_values(headers, constants.HEADER_SET_SESSION)
+    assert len(values) == 0