fix(GCS+gRPC): correctly resume ranged reads (googleapis#14495)

cuiy0006 · Jul 22, 2024 · ef52b74 · ef52b74
1 parent e30a274
commit ef52b74
Show file tree

Hide file tree

Showing 2 changed files with 57 additions and 12 deletions.
diff --git a/google/cloud/storage/internal/grpc/object_request_parser.cc b/google/cloud/storage/internal/grpc/object_request_parser.cc
@@ -19,6 +19,7 @@
 #include "google/cloud/storage/internal/grpc/object_metadata_parser.h"
 #include "google/cloud/storage/internal/object_access_control_parser.h"
 #include "google/cloud/storage/internal/patch_builder_details.h"
+#include "google/cloud/internal/absl_str_cat_quiet.h"
 #include "google/cloud/internal/invoke_result.h"
 #include "google/cloud/internal/make_status.h"
 #include "google/cloud/internal/time_utils.h"
@@ -377,24 +378,29 @@ StatusOr<google::storage::v2::ReadObjectRequest> ToProto(
   if (request.HasOption<storage::Generation>()) {
     r.set_generation(request.GetOption<storage::Generation>().value());
   }
-  if (request.HasOption<storage::ReadRange>()) {
+  auto const offset = request.GetOption<storage::ReadFromOffset>().value_or(0);
+  if (!request.HasOption<storage::ReadRange>()) {
+    r.set_read_offset(offset);
+  } else {
     auto const range = request.GetOption<storage::ReadRange>().value();
-    r.set_read_offset(range.begin);
-    r.set_read_limit(range.end - range.begin);
+    auto const merged_offset = (std::max)(range.begin, offset);
+    auto const merged_limit = range.end - merged_offset;
+    if (merged_limit <= 0) {
+      return internal::InvalidArgumentError(
+          absl::StrCat("Invalid gRPC range offset=", merged_offset,
+                       ", limit=", merged_limit, ", from offset=", offset,
+                       " range=[", range.begin, ",", range.end, "]"),
+          GCP_ERROR_INFO());
+    }
+
+    r.set_read_offset(merged_offset);
+    r.set_read_limit(merged_limit);
   }
+
   if (request.HasOption<storage::ReadLast>()) {
     auto const offset = request.GetOption<storage::ReadLast>().value();
     r.set_read_offset(-offset);
   }
-  if (request.HasOption<storage::ReadFromOffset>()) {
-    auto const offset = request.GetOption<storage::ReadFromOffset>().value();
-    if (offset > r.read_offset()) {
-      if (r.read_limit() > 0) {
-        r.set_read_limit(offset - r.read_offset());
-      }
-      r.set_read_offset(offset);
-    }
-  }
   SetGenerationConditions(r, request);
   SetMetagenerationConditions(r, request);
 

diff --git 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"