From d6b46448549cd299a071b3207380c35c796191d8 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Thu, 18 Jul 2024 16:55:09 +0000 Subject: [PATCH 1/4] 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 ToProto( } if (request.HasOption()) { auto const offset = request.GetOption().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" From 5ac0e9a426d93ba292d5e5ea4bf09fc151d93a06 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Thu, 18 Jul 2024 22:08:34 +0000 Subject: [PATCH 2/4] Address review comments --- .../internal/grpc/object_request_parser.cc | 35 +++++++++---------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/google/cloud/storage/internal/grpc/object_request_parser.cc b/google/cloud/storage/internal/grpc/object_request_parser.cc index 66335a3abaeae..fbf741cab0dcd 100644 --- a/google/cloud/storage/internal/grpc/object_request_parser.cc +++ b/google/cloud/storage/internal/grpc/object_request_parser.cc @@ -377,30 +377,29 @@ StatusOr ToProto( if (request.HasOption()) { r.set_generation(request.GetOption().value()); } - if (request.HasOption()) { + auto const offset = request.GetOption().value_or(0); + if (!request.HasOption()) { + r.set_read_offset(offset); + } else { auto const range = request.GetOption().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()) { auto const offset = request.GetOption().value(); r.set_read_offset(-offset); } - if (request.HasOption()) { - auto const offset = request.GetOption().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) { - auto const limit = r.read_limit() - (offset - r.read_offset()); - r.set_read_limit(limit); - } - r.set_read_offset(offset); - } - } SetGenerationConditions(r, request); SetMetagenerationConditions(r, request); From 917bc6b7f601405e4914b5e66c38e411757f0d17 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Thu, 18 Jul 2024 22:20:59 +0000 Subject: [PATCH 3/4] Add missing #include --- google/cloud/storage/internal/grpc/object_request_parser.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/google/cloud/storage/internal/grpc/object_request_parser.cc b/google/cloud/storage/internal/grpc/object_request_parser.cc index fbf741cab0dcd..5f9f798111fee 100644 --- 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" From 1fe132c481b4cd8879803e5a5bc4e1f7d41d5e5a Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Thu, 18 Jul 2024 22:24:32 +0000 Subject: [PATCH 4/4] Address review comments --- google/cloud/storage/internal/grpc/object_request_parser.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google/cloud/storage/internal/grpc/object_request_parser.cc b/google/cloud/storage/internal/grpc/object_request_parser.cc index 5f9f798111fee..b2bd6db03df79 100644 --- a/google/cloud/storage/internal/grpc/object_request_parser.cc +++ b/google/cloud/storage/internal/grpc/object_request_parser.cc @@ -388,8 +388,8 @@ StatusOr ToProto( 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, ")"), + ", limit=", merged_limit, ", from offset=", offset, + " range=[", range.begin, ",", range.end, "]"), GCP_ERROR_INFO()); }