Repository navigation
feat(firestore): add x-goog-is-transaction routing header - #14680
nikhilchowdary wants to merge 2 commits into
Conversation
Attach x-goog-is-transaction header ('true' for transactional RPCs, 'false' for non-transactional RPCs) in GrpcFirestoreRpc.
There was a problem hiding this comment.
Code Review
This pull request adds transaction-specific routing headers (x-goog-is-transaction) to various Firestore RPC callables in GrpcFirestoreRpc and introduces comprehensive unit tests to verify this behavior. The review feedback recommends refactoring the anonymous classes used for routing headers into named static nested classes. This prevents fragile reflection-based test assertions in GrpcFirestoreRpcTest from relying on compiler-generated class names like GrpcFirestoreRpc$1.
| private static <RequestT, ResponseT> UnaryCallable<RequestT, ResponseT> withRoutingHeaders( | ||
| UnaryCallable<RequestT, ResponseT> callable, Predicate<RequestT> isTransaction) { | ||
| return new UnaryCallable<RequestT, ResponseT>() { | ||
| @Override | ||
| public ApiFuture<ResponseT> futureCall(RequestT request, ApiCallContext thisCallContext) { | ||
| ApiCallContext defaultContext = | ||
| isTransaction.test(request) ? TRANSACTION_CALL_CONTEXT : NON_TRANSACTION_CALL_CONTEXT; | ||
| return callable.futureCall(request, defaultContext.merge(thisCallContext)); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| private static <RequestT, ResponseT> | ||
| ServerStreamingCallable<RequestT, ResponseT> withRoutingHeaders( | ||
| ServerStreamingCallable<RequestT, ResponseT> callable, | ||
| Predicate<RequestT> isTransaction) { | ||
| return new ServerStreamingCallable<RequestT, ResponseT>() { | ||
| @Override | ||
| public void call( | ||
| RequestT request, | ||
| ResponseObserver<ResponseT> responseObserver, | ||
| ApiCallContext thisCallContext) { | ||
| ApiCallContext defaultContext = | ||
| isTransaction.test(request) ? TRANSACTION_CALL_CONTEXT : NON_TRANSACTION_CALL_CONTEXT; | ||
| callable.call(request, responseObserver, defaultContext.merge(thisCallContext)); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Using anonymous classes here is fragile because reflection-based tests (such as GrpcFirestoreRpcTest.getRetryData) rely on compiler-generated class names like GrpcFirestoreRpc$1 and GrpcFirestoreRpc$2. Any future addition or reordering of anonymous classes in this file will silently break these tests. Replacing them with named static nested classes makes the design robust and the class names stable.
private static <RequestT, ResponseT> UnaryCallable<RequestT, ResponseT> withRoutingHeaders(
UnaryCallable<RequestT, ResponseT> callable, Predicate<RequestT> isTransaction) {
return new RoutingMetadataUnaryCallable<>(callable, isTransaction);
}
private static <RequestT, ResponseT>
ServerStreamingCallable<RequestT, ResponseT> withRoutingHeaders(
ServerStreamingCallable<RequestT, ResponseT> callable,
Predicate<RequestT> isTransaction) {
return new RoutingMetadataServerStreamingCallable<>(callable, isTransaction);
}
private static class RoutingMetadataUnaryCallable<RequestT, ResponseT>
extends UnaryCallable<RequestT, ResponseT> {
private final UnaryCallable<RequestT, ResponseT> callable;
private final Predicate<RequestT> isTransaction;
RoutingMetadataUnaryCallable(
UnaryCallable<RequestT, ResponseT> callable, Predicate<RequestT> isTransaction) {
this.callable = callable;
this.isTransaction = isTransaction;
}
@Override
public ApiFuture<ResponseT> futureCall(RequestT request, ApiCallContext thisCallContext) {
ApiCallContext defaultContext =
isTransaction.test(request) ? TRANSACTION_CALL_CONTEXT : NON_TRANSACTION_CALL_CONTEXT;
return callable.futureCall(request, defaultContext.merge(thisCallContext));
}
}
private static class RoutingMetadataServerStreamingCallable<RequestT, ResponseT>
extends ServerStreamingCallable<RequestT, ResponseT> {
private final ServerStreamingCallable<RequestT, ResponseT> callable;
private final Predicate<RequestT> isTransaction;
RoutingMetadataServerStreamingCallable(
ServerStreamingCallable<RequestT, ResponseT> callable,
Predicate<RequestT> isTransaction) {
this.callable = callable;
this.isTransaction = isTransaction;
}
@Override
public void call(
RequestT request,
ResponseObserver<ResponseT> responseObserver,
ApiCallContext thisCallContext) {
ApiCallContext defaultContext =
isTransaction.test(request) ? TRANSACTION_CALL_CONTEXT : NON_TRANSACTION_CALL_CONTEXT;
callable.call(request, responseObserver, defaultContext.merge(thisCallContext));
}
}Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Add x-goog-is-transaction routing header in Java SDK to differentiate transactional requests and non-transactional requests from firestore. Having a routing header for transactions helps in routing grpc requests at GFE.