googleapis / googleapis/google-cloud-java

[java-bigtable] Mutation's MAX_MUTATIONS and MAX_BYTE_SIZE do not count mutations wrapped from protos

Đang mở
#14,020 0 bình luận 0 reaction 1 người được giao Được @mutianf nhận Xem trên GitHub
Ngôn ngữ chính
Java
Star
2.1k
Fork
1.2k
Merge trung bình
1 ngày 23 giờ
Pull request đã merge (30 ngày)
154

Mô tả

Version: `google-cloud-bigtable` 2.80.0. `Mutation.java` is byte-identical at `main` today, so the
line numbers below apply there too.

### Summary

`Mutation.MAX_MUTATIONS` and `Mutation.MAX_BYTE_SIZE` are enforced in `addMutation`, against
counters that only `addMutation` maintains. Three factories add mutations without going through it,
so mutations wrapped from existing protos do not count towards either limit. For the mutation count
this is caught later by a separate check; for the byte size there is no check anywhere else, so the
200 MB guard simply does not apply to those mutations.

### Detail

The counters and the guard:

```java
// Mutation.java:329-340
private void addMutation(com.google.bigtable.v2.Mutation mutation) {
Preconditions.checkState(numMutations + 1 <= MAX_MUTATIONS, "Too many mutations per row");
Preconditions.checkState(
byteSize + mutation.getSerializedSize() <= MAX_BYTE_SIZE,
"Byte size of mutations is too large");

numMutations++;
byteSize += mutation.getSerializedSize();

mutations.add(mutation);
}
```

Three factories add to the same list directly, leaving both counters at zero:

```java
// Mutation.java:85-91, 98-105, 115-121
public static Mutation fromProtoUnsafe(List protos) {
Mutation mutation = new Mutation(true);
mutation.mutations.addAll(protos); // numMutations and byteSize untouched
return mutation;
}
// ... the Iterable overload and the package-private fromProto(List) are the same shape
```

They are reachable publicly through `Mutation.fromProtoUnsafe(...)` (`@BetaApi`),
`RowMutationEntry.createFromMutationUnsafe(...)` (`@BetaApi`) and
`RowMutation.fromProto(MutateRowRequest)`, which is the one in-tree caller of the package-private
`fromProto`.

### Reproduction

Measured, not inferred — a probe on 2.80.0, five mutations wrapped from protos and then
`MAX_MUTATIONS` more added through `setCell`:

```text
MAX_MUTATIONS=100000
seeded=5 addedViaSetCell=100000 threw=null
actual mutations in list = 100005
toProto() rejected: IllegalArgumentException: Too many mutations, got 100005, limit is 100000
```

So the guard in `addMutation` never fires, and the row grows past the limit unchecked.

### What is and is not caught downstream

I want to be precise about the impact, because the two limits differ:

- **Mutation count is backstopped**, so it does surface — but late, from a different place and as a
different exception type. `RowMutationEntry.toProto()` (`RowMutationEntry.java:205-209`) and
`BulkMutation.add` (`BulkMutation.java:103-106`) both re-check against `getMutations().size()`,
which is the real count. The user gets an `IllegalArgumentException` at send time rather than an
`IllegalStateException` from the `setCell` that actually crossed the line.
- **Byte size is not backstopped at all.** `MAX_BYTE_SIZE` appears only in `addMutation`. A
`Mutation` seeded from protos can exceed 200 MB — with or without further `setCell` calls — and
nothing client-side objects; the request is built and sent, and the server rejects it. That is
the half of this that actually loses the guard rather than deferring it.

### Suggested fix

Count wrapped protos in the three factories, so the counters describe the whole row:

```java
private void countTowardsLimits(com.google.bigtable.v2.Mutation mutation) {
numMutations++;
byteSize += mutation.getSerializedSize();
}
```

called from `addMutation` and from each `fromProto` factory. Deliberately *not* a `checkState` in
the factories themselves: that would make wrapping an over-limit proto throw where it currently
does not, which is a bigger behaviour change than restoring the guard needs.

This does change behaviour: a caller that wraps protos and then adds more mutations will now hit
`IllegalStateException` at the `setCell` that crosses a limit, where today it either fails later
(count) or not at all (bytes). I think that is the intent of the limits — fail fast client-side
rather than send a request that cannot succeed — but it is your call, and I would understand
wanting it behind a release note.

I have this implemented with tests that fail without it, and will open a PR referencing this issue.

### Related

Two other findings in the same area are filed separately and are independent of this one: the
duplicate proto construction in `MutateRowsBatchingDescriptor.createResource()`, and the absence of
a serialized-size accessor on `RowMutationEntry`.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.