Skip to content
This repository was archived by the owner on Apr 1, 2026. It is now read-only.

feat: Rerouted DirectRow.commit to use MutateRow #1276

Merged
gkevinzheng merged 5 commits into from
Jan 29, 2026
Merged

feat: Rerouted DirectRow.commit to use MutateRow #1276

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
09a22fe
feat: Rerouted DirectRow.commit to use MutateRow
gkevinzheng Jan 26, 2026
9caad04
Added exception handling for client side ValueError
gkevinzheng Jan 28, 2026
038937a
Clarified docstring
gkevinzheng Jan 28, 2026
8b8a4ca
Changed no mutations to raise ValueError instead of status
gkevinzheng Jan 29, 2026
e5dc1cc
Update row.py
gkevinzheng Jan 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions google/cloud/bigtable/row.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,8 +446,7 @@ def commit(self):
"""Makes a ``MutateRow`` API request.

If no mutations have been created in the row, no request is made and a
:class:`~google.rpc.status_pb2.Status` with code INVALID_ARGUMENT is returned
instead.
ValueError is raised instead.

Mutations are applied atomically and in order, meaning that earlier
mutations can be masked / negated by later ones. Cells already present
Expand All @@ -466,6 +465,7 @@ def commit(self):
:rtype: :class:`~google.rpc.status_pb2.Status`
:returns: A response status (`google.rpc.status_pb2.Status`)
representing success or failure of the row committed.
:raises: ValueError: if no mutations have been created in the row
"""
try:
self._table._table_impl.mutate_row(self.row_key, self._get_mutations())

@daniel-sanche daniel-sanche Jan 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring says "If no mutations have been created in the row, no request is made.". But it looks like mutate_row raises a ValueError if no mutations are present.

You should probably catch and swallow that exception, and make sure this case is handled in tests

@daniel-sanche daniel-sanche Jan 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mutate_row has automatic retries. Is that expected? Did the previous implementation retry?

@gkevinzheng gkevinzheng Jan 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous implementation used Table.mutate_rows with the default retry.

Expand All @@ -479,12 +479,8 @@ def commit(self):
message=e.message,
details=e.details,
)
Comment on lines +470 to +481

@gemini-code-assist gemini-code-assist Bot Jan 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The DirectRow.commit method now relies on the underlying _table_impl.mutate_row to validate the number of mutations and handle empty mutation lists. This is a change from the previous client-side validation for MAX_MUTATIONS. However, ConditionalRow.commit (lines 591-602 in the full file) still performs client-side checks for MAX_MUTATIONS and returns early for empty mutation lists. This inconsistency in validation strategy across Row subclasses could lead to varied error behaviors and should be addressed for better maintainability and predictability. Consider aligning the validation approach for ConditionalRow.commit with DirectRow.commit, or reintroducing explicit client-side validation in DirectRow.commit if that is the desired behavior for all Row types.

except ValueError as e:
# _table_impl.mutate_row raises a ValueError if invalid arguments are provided.
return status_pb2.Status(
code=code_pb2.INVALID_ARGUMENT,
message=str(e),
)
except ValueError:
raise

@daniel-sanche daniel-sanche Jan 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this doesn't do anything, does it? Doesn't catching and raising do the same thing as not catching?

finally:
self.clear()

Expand Down
6 changes: 3 additions & 3 deletions tests/system/v2_client/test_data_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1092,10 +1092,10 @@ def test_table_direct_row_input_errors(data_table, rows_to_delete):
resp = row.commit()
assert resp.code == StatusCode.INVALID_ARGUMENT.value[0]

# Not having any mutations gives an INVALID_ARGUMENT
# Not having any mutations raises a ValueError
row.clear()
resp = row.commit()
assert resp.code == StatusCode.INVALID_ARGUMENT.value[0]
with pytest.raises(ValueError):
resp = row.commit()


def test_table_conditional_row_input_errors(data_table, rows_to_delete):
Expand Down
7 changes: 2 additions & 5 deletions tests/unit/v2_client/test_row.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,11 +494,8 @@ def test_direct_row_commit_with_invalid_argument():
client._table_data_client._gapic_client = api

# Perform the method and check the result.
result = row.commit()
assert row._mutations == []
assert result == status_pb2.Status(
code=code_pb2.Code.INVALID_ARGUMENT, message="No mutations provided"
)
with pytest.raises(ValueError, match="No mutations provided"):
row.commit()
api.mutate_row.assert_not_called()


Expand Down
Loading