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

Issue: Inconsistency in Transaction.execute_update Documentation Regarding param_types #1276

Open
taua-almeida opened this issue Dec 19, 2024 · 0 comments
Assignees
Labels
api: spanner Issues related to the googleapis/python-spanner API.

Comments

@taua-almeida
Copy link

taua-almeida commented Dec 19, 2024

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:

(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:

"""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

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:

@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:

(Optional) maps explicit types for one or more param values.
This argument is optional, even when parameters are provided.
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner API. label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner API.
Projects
None yet
Development

No branches or pull requests

2 participants