From d6b46448549cd299a071b3207380c35c796191d8 Mon Sep 17 00:00:00 2001
From: Carlos O'Ryan <coryan@google.com>
Date: Thu, 18 Jul 2024 16:55:09 +0000
Subject: [PATCH] fix(GCS+gRPC): correctly resume ranged reads

---
 .../internal/grpc/object_request_parser.cc    |  8 +++-
 .../grpc/object_request_parser_test.cc        | 39 +++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/google/cloud/storage/internal/grpc/object_request_parser.cc b/google/cloud/storage/internal/grpc/object_request_parser.cc
index 1ee1d75d8a454..66335a3abaeae 100644
--- a/google/cloud/storage/internal/grpc/object_request_parser.cc
+++ b/google/cloud/storage/internal/grpc/object_request_parser.cc
@@ -388,9 +388,15 @@ StatusOr<google::storage::v2::ReadObjectRequest> ToProto(
   }
   if (request.HasOption<storage::ReadFromOffset>()) {
     auto const offset = request.GetOption<storage::ReadFromOffset>().value();
+    if (r.read_limit() > 0 && offset >= r.read_offset() + r.read_limit()) {
+      return internal::InvalidArgumentError(
+          "ReadRange() and ReadFromOffset() options produce an empty range",
+          GCP_ERROR_INFO());
+    }
     if (offset > r.read_offset()) {
       if (r.read_limit() > 0) {
-        r.set_read_limit(offset - r.read_offset());
+        auto const limit = r.read_limit() - (offset - r.read_offset());
+        r.set_read_limit(limit);
       }
       r.set_read_offset(offset);
     }
diff --git a/google/cloud/storage/internal/grpc/object_request_parser_test.cc b/google/cloud/storage/internal/grpc/object_request_parser_test.cc
index 4734d8f6e864b..3fc22c8ddc73f 100644
--- a/google/cloud/storage/internal/grpc/object_request_parser_test.cc
+++ b/google/cloud/storage/internal/grpc/object_request_parser_test.cc
@@ -367,6 +367,45 @@ TEST(GrpcObjectRequestParser,
   EXPECT_THAT(actual, StatusIs(StatusCode::kInvalidArgument));
 }
 
+TEST(GrpcObjectRequestParser, ReadObjectRangeMergeWithOffset) {
+  auto expected = google::storage::v2::ReadObjectRequest{};
+  EXPECT_TRUE(TextFormat::ParseFromString(
+      R"pb(
+        bucket: "projects/_/buckets/test-bucket"
+        object: "test-object"
+        read_offset: 2000
+        read_limit: 1000
+      )pb",
+      &expected));
+  auto request =
+      storage::internal::ReadObjectRangeRequest("test-bucket", "test-object")
+          .set_multiple_options(storage::ReadRange(2000, 3000));
+  EXPECT_THAT(ToProto(request), IsOkAndHolds(IsProtoEqual(expected)));
+
+  request.set_multiple_options(storage::ReadFromOffset(1900));
+  EXPECT_THAT(ToProto(request), IsOkAndHolds(IsProtoEqual(expected)));
+
+  request.set_multiple_options(storage::ReadFromOffset(2100));
+  expected.set_read_offset(2100);
+  expected.set_read_limit(900);
+  EXPECT_THAT(ToProto(request), IsOkAndHolds(IsProtoEqual(expected)));
+
+  request.set_multiple_options(storage::ReadFromOffset(2300));
+  expected.set_read_offset(2300);
+  expected.set_read_limit(700);
+  EXPECT_THAT(ToProto(request), IsOkAndHolds(IsProtoEqual(expected)));
+}
+
+TEST(GrpcObjectRequestParser, ReadOffsetEmptyRange) {
+  auto request =
+      storage::internal::ReadObjectRangeRequest("test-bucket", "test-object")
+          .set_multiple_options(storage::ReadFromOffset(3000),
+                                storage::ReadRange(2000, 3000));
+
+  auto const actual = ToProto(request);
+  EXPECT_THAT(actual, StatusIs(StatusCode::kInvalidArgument));
+}
+
 TEST(GrpcObjectRequestParser, PatchObjectRequestAllOptions) {
   auto constexpr kTextProto = R"pb(
     predefined_acl: "projectPrivate"