Repository: googleapis/python-bigquery
Pull request #1987 (2024-08-05 14:35:26)

fix: Do not set job timeout extra property if None
### Description
`job_timeout_ms` declared as optional ([see docs](https://cloud.google.com/python/docs/reference/bigquery/latest/google.cloud.bigquery.job.CopyJobConfig#google_cloud_bigquery_job_CopyJobConfig_job_timeout_ms)), but setting it as None results in error:
```
google.api_core.exceptions.BadRequest: 400 POST https://bigquery.googleapis.com/bigquery/v2/projects/my-bq-project/jobs?prettyPrint=false: Invalid value at 'job.configuration.job_timeout_ms.value' (TYPE_INT64), "None"

Location: None
Job ID: 5f5bf5bd-dc77-7eda-a2d3-69420a6bf6d6
 [{'@type': 'type.googleapis.com/google.rpc.BadRequest', 'fieldViolations': [{'field': 'job.configuration.job_timeout_ms.value', 'description': 'Invalid value at \'job.configuration.job_timeout_ms.value\' (TYPE_INT64), "None"'}]}]
```

because of [allowing None](https://github.com/googleapis/python-bigquery/blob/main/google/cloud/bigquery/job/base.py#L215), but not handling it properly. This fix doesn't cast None to a string in setter. Getter still works as expected, but we do not send invalid value in remote call.

### Simple script to reproduce
```python
from typing import Optional

from google.cloud import bigquery

query = "SELECT * FROM `project.table` LIMIT 1"
client = bigquery.Client()
config = bigquery.QueryJobConfig(use_query_cache=True, allow_large_results=True)
my_timeout: Optional[int] = None
config.job_timeout_ms = my_timeout
client.query(query, job_config=config)
```


google-cla[bot]: Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this [failed invocation](https://github.com/googleapis/python-bigquery/pull/1987/checks?check_run_id=28357457560) of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.
conventional-commit-lint-gcf[bot]: <!-- probot comment [1356997]-->
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use `automerge` label. Good luck human!

 -- conventional-commit-lint bot
https://conventionalcommits.org/
bmwant: @Linchin updated, could you review?
bmwant: @Linchin rebased on the latest main branch
Linchin: Thank you for making the contribution and fix this bug for us! I wonder if you would like to lint the code, as this seems to be the only test that's failing. I haven't had the time to do it. To lint you need to install nox and run `nox -s lint` in the repo. 


bmwant: Fixed linting, you can re-run checks now

Patch:
diff --git a/google/cloud/bigquery/job/base.py b/google/cloud/bigquery/job/base.py
index f165fd03..e5f68c84 100644
--- a/google/cloud/bigquery/job/base.py
+++ b/google/cloud/bigquery/job/base.py
@@ -218,8 +218,11 @@ class _JobConfig(object):
                 err.__traceback__
             )
 
-        """ Docs indicate a string is expected by the API """
-        self._properties["jobTimeoutMs"] = str(value)
+        if value is not None:
+            # docs indicate a string is expected by the API
+            self._properties["jobTimeoutMs"] = str(value)
+        else:
+            self._properties.pop("jobTimeoutMs", None)
 
     @property
     def labels(self):


Test patch:
diff --git a/tests/unit/job/test_base.py b/tests/unit/job/test_base.py
index a7337afd..2d2f0c13 100644
--- a/tests/unit/job/test_base.py
+++ b/tests/unit/job/test_base.py
@@ -1320,3 +1320,21 @@ class Test_JobConfig(unittest.TestCase):
         # Confirm that integers get converted to strings.
         job_config.job_timeout_ms = 5000
         assert job_config.job_timeout_ms == "5000"  # int is converted to string
+
+    def test_job_timeout_is_none_when_set_none(self):
+        job_config = self._make_one()
+        job_config.job_timeout_ms = None
+        # Confirm value is None and not literal string 'None'
+        assert job_config.job_timeout_ms is None
+
+    def test_job_timeout_properties(self):
+        # Make sure any value stored in properties is erased
+        # when setting job_timeout to None.
+        job_config = self._make_one()
+        job_config.job_timeout_ms = 4200
+        assert job_config.job_timeout_ms == "4200"
+        assert job_config._properties.get("jobTimeoutMs") == "4200"
+
+        job_config.job_timeout_ms = None
+        assert job_config.job_timeout_ms is None
+        assert "jobTimeoutMs" not in job_config._properties