Skip to content

feat(firestore): add x-goog-is-transaction routing header - #14680

Open
nikhilchowdary wants to merge 2 commits into
googleapis:mainfrom
nikhilchowdary:feat-firestore-tx-routing-header
Open

nikhilchowdary wants to merge 2 commits into
googleapis:mainfrom
nikhilchowdary:feat-firestore-tx-routing-header

Conversation

@nikhilchowdary

Copy link
Copy Markdown

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.

Attach x-goog-is-transaction header ('true' for transactional RPCs, 'false' for non-transactional RPCs) in GrpcFirestoreRpc.
@nikhilchowdary
nikhilchowdary requested review from a team as code owners October 17, 2026 05:44

@gemini-code-assist gemini-code-assist Bot left a comment

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.

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.

Comment on lines +328 to 355
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));
}
};
}

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

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>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant