googleapis / googleapis/google-cloud-java
[java-bigtable] Mutation's MAX_MUTATIONS and MAX_BYTE_SIZE do not count mutations wrapped from protos
- Lenguaje dominante
- Java
- Estrellas
- 2.1k
- Forks
- 1.2k
- Merge medio
- 1 d 23 h
- PR fusionados (30 d)
- 154
Descripción
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`.
Guía de contribución
Evaluación
Este issue todavía no se ha evaluado.