fix(GCS+gRPC): correctly resume ranged reads

googleapis · Jul 18, 2024 · d6b4644 · d6b4644
1 parent eb2cacc
commit d6b4644
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 1 deletion.
diff --git 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
@@ -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"