googleapis / googleapis/google-cloud-python

Issue: Inconsistency in `Transaction.execute_update` Documentation Regarding `param_types`

Abierto
#15,902 1 comentario 2 reacciones 1 asignado Reclamado por @larkee Ver en GitHub
api: spanner
Lenguaje dominante
Python
Estrellas
5.4k
Forks
1.8k
Merge medio
3 d 4 h
PR fusionados (30 d)
122

Descripción

#### Overview

While reading through the `Transaction` class documentation, I noticed a potential inconsistency or missing implementation regarding the `param_types` argument in the `execute_update` method.

The documentation states the following about param_types:
```py
(Optional) maps explicit types for one or more param values;
required if parameters are passed.
```

Additionally, the docstring for the helper method `_make_params_pb`, which is used by `execute_update`, provides this information:

```py
"""Helper for :meth:`execute_update`.

:type params: dict, {str -> column value}
:param params: values for parameter replacement. Keys must match
the names used in ``dml``.

:type param_types: dict[str -> Union[dict, .types.Type]]
:param param_types:
(Optional) maps explicit types for one or more param values;
required if parameters are passed.

:rtype: Union[None, :class:`Struct`]
:returns: a struct message for the passed params, or None
:raises ValueError:
If ``param_types`` is None but ``params`` is not None.
:raises ValueError:
If ``params`` is None but ``param_types`` is not None.
"""
```

Contrary to the documentation, the` _make_params_pb` method does not appear to include any conditions to raise a ValueError. Furthermore, when I run the following code without explicitly providing `param_types`, no error is raised, and the update works as expected.

#### Question
Is the documentation incorrect in stating that `param_types` is required when params are passed, or is the implementation missing the `ValueError` raises described in the docstring?

#### Environment details

- OS type and version: macOS Sequoia 15.2
- Python version: `3.11.9`
- pip version: `24.0`
- `google-cloud-spanner` version: `3.51.0`

#### Steps to reproduce
1. Use the `Transaction.execute_update` method with the following parameters:
- @new_val = 1 and @table_id = 1.
- Do not include `param_types`.

2. Observe that no error is raised, despite the documentation indicating that `param_types` is required when `params` are provided.

#### Code example

```python
transaction.execute_update(
"""
UPDATE Table
SET Col = @new_val
WHERE TableId = @table_id
""",
params={
"new_val": new_val,
"table_id": table_id
},
)
```

#### Expected behavior
According to the documentation, param_types is required when params are passed. If param_types is not provided, a ValueError should be raised.

#### Observed behavior
No error is raised, and the query executes successfully.

#### Stack trace

No error or exception is raised, so no stack trace is available.

### Suggestions

**1. Add a `ValueError` in the implementation:**
Update the `_make_params_pb` method to raise a `ValueError` if `param_types` is `None` while params is not `None`. For example:
```py
@staticmethod
def _make_params_pb(params, param_types):
"""Helper for :meth:`execute_update`.

:type params: dict, {str -> column value}
:param params: values for parameter replacement. Keys must match
the names used in ``dml``.

:type param_types: dict[str -> Union[dict, .types.Type]]
:param param_types:
(Optional) maps explicit types for one or more param values;
required if parameters are passed.

:rtype: Union[None, :class:`Struct`]
:returns: a struct message for the passed params, or None
:raises ValueError:
If ``param_types`` is None but ``params`` is not None.
:raises ValueError:
If ``params`` is None but ``param_types`` is not None.
"""
if (params is not None and param_types is None):
raise ValueError("Both `params` and `param_types` must be provided together.")
if params:
return Struct(
fields={key: _make_value_pb(value) for key, value in params.items()}
)

return {}
```

**Update the documentation to reflect the actual behavior**
Modify the docstring in `_make_params_pb` and the `Transaction.execute_update` method to clarify that `param_types` is optional, even when params are passed, and that no error will be raised if only one of them is provided. For example:
```py
(Optional) maps explicit types for one or more param values.
This argument is optional, even when parameters are provided.
```

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.