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
8 changes: 7 additions & 1 deletion google/cloud/bigtable/row.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ def commit(self):
"""
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.

return status_pb2.Status()
return status_pb2.Status(code=code_pb2.OK)
except GoogleAPICallError as e:
# If the RPC call returns an error, extract the error into a status object, if possible.
return status_pb2.Status(
Expand All @@ -477,6 +477,12 @@ 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),
)
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 a ValueError enforced on the client side.
# Not having any mutations gives an INVALID_ARGUMENT
row.clear()
with pytest.raises(ValueError):
row.commit()

@daniel-sanche daniel-sanche Jan 28, 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.

I'm surprised this part had to change, since it seems counter to what the docstring suggested?

Was the previous behaviour to throw an error?

resp = row.commit()
assert resp.code == StatusCode.INVALID_ARGUMENT.value[0]


def test_table_conditional_row_input_errors(data_table, rows_to_delete):
Expand Down
28 changes: 28 additions & 0 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 @@ -474,6 +474,34 @@ def test_direct_row_commit_with_unknown_exception():
assert app_profile_id == call_args.app_profile_id[0]


def test_direct_row_commit_with_invalid_argument():
from google.cloud.bigtable_v2.services.bigtable import BigtableClient
from google.rpc import code_pb2, status_pb2

project_id = "project-id"
row_key = b"row_key"
table_name = "projects/more-stuff"
app_profile_id = "app_profile_id"

credentials = _make_credentials()
client = _make_client(project=project_id, credentials=credentials, admin=True)
table = _Table(table_name, client=client, app_profile_id=app_profile_id)
row = _make_direct_row(row_key, table)

# Set mock
api = mock.create_autospec(BigtableClient)
client.table_data_client
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"
)
api.mutate_row.assert_not_called()


def _make_conditional_row(*args, **kwargs):
from google.cloud.bigtable.row import ConditionalRow

Expand Down
Loading