googleapis / googleapis/google-cloud-java

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

Abierto
#14,020 0 comentarios 0 reacciones 1 asignado Reclamado por @mutianf Ver en GitHub
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

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.