Skip to content

feat(storage): Making crc32c default for download #34002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shubhangi-google wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat(storage): Making crc32c default for download #34002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec8a9cc
making default
shubhangi-google Jun 2, 2026
998fbd2
remove pry
shubhangi-google Jun 2, 2026
b5660b6
fixing tests
shubhangi-google Jun 2, 2026
19d488e
added test
shubhangi-google Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions google-cloud-storage/lib/google/cloud/storage/file.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,7 +963,7 @@ def update generation: nil,
# omitted, a new StringIO instance will be written to and returned.
# Optional.
# @param [Symbol] verify The verification algorithm used to ensure the
# downloaded file contents are correct. Default is `:md5`.
# downloaded file contents are correct. Default is `:crc32c`.
#
# Acceptable values are:
#
Expand Down Expand Up @@ -1098,7 +1098,7 @@ def update generation: nil,
# downloaded.rewind
# downloaded.read #=> "world"
#
def download path = nil, verify: :md5, encryption_key: nil, range: nil,
def download path = nil, verify: :crc32c, encryption_key: nil, range: nil,
Copy link
Copy Markdown

@v-pratap v-pratap Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ensure the official documentation for checksumming is updated to reflect that :crc32c is now the default instead of :md5

Copy link
Copy Markdown
Contributor Author

@shubhangi-google shubhangi-google Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

skip_decompress: nil
ensure_service!
if path.nil?
Expand Down Expand Up @@ -2279,7 +2279,7 @@ def fix_rewrite_args dest_bucket, dest_path
[dest_bucket, dest_path]
end

def verify_file! file, verify = :md5
def verify_file! file, verify = :crc32c
verify_md5 = verify == :md5 || verify == :all
verify_crc32c = verify == :crc32c || verify == :all
Verifier.verify_md5! self, file if verify_md5 && md5
Expand Down
77 changes: 40 additions & 37 deletions google-cloud-storage/test/google/cloud/storage/file_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,9 @@
end

it "can download itself to a file" do
# Stub the md5 to match.
def file.md5
"X7A8HRvZUCT5gbq0KNDL8Q=="
# Stub the crc32c to match.
def file.crc32c
Digest::CRC32c.base64digest "yay!"
end

Tempfile.open "google-cloud" do |tmpfile|
Expand All @@ -237,8 +237,8 @@ def file.md5
data = "Hello world!"
gzipped_data = gzip_data data

# Stub the md5 to match.
file.gapi.md5_hash = Digest::MD5.base64digest gzipped_data
# Stub the crc32c to match.
file.gapi.crc32c = Digest::CRC32c.base64digest gzipped_data

Tempfile.open "google-cloud" do |tmpfile|
# write to the file since the mocked call won't
Expand All @@ -263,8 +263,8 @@ def file.md5
data = "Hello world!"
gzipped_data = gzip_data data

# Stub the md5 to match.
file.gapi.md5_hash = Digest::MD5.base64digest gzipped_data
# Stub the crc32c to match.
file.gapi.crc32c = Digest::CRC32c.base64digest gzipped_data

Tempfile.open "google-cloud" do |tmpfile|
# write to the file since the mocked call won't
Expand All @@ -286,14 +286,15 @@ def file.md5
end

it "can download itself to a file by path" do
# Stub the md5 to match.
def file.md5
"1B2M2Y8AsgTpgAmY7PhCfg=="
# Stub the crc32c to match.
def file.crc32c
Digest::CRC32c.base64digest "yay!"
end

Tempfile.open "google-cloud" do |tmpfile|
# write to the file since the mocked call won't
tmpfile.write "yay!"
tmpfile.rewind

mock = Minitest::Mock.new
mock.expect :get_object, [tmpfile, download_http_resp],
Expand All @@ -309,14 +310,15 @@ def file.md5
end

it "can download itself to a file with user_project set to true" do
# Stub the md5 to match.
def file_user_project.md5
"1B2M2Y8AsgTpgAmY7PhCfg=="
# Stub the crc32c to match.
def file_user_project.crc32c
Digest::CRC32c.base64digest "yay!"
end

Tempfile.open "google-cloud" do |tmpfile|
# write to the file since the mocked call won't
tmpfile.write "yay!"
tmpfile.rewind

mock = Minitest::Mock.new
mock.expect :get_object, [tmpfile, download_http_resp],
Expand All @@ -332,9 +334,9 @@ def file_user_project.md5
end

it "can download itself to an IO" do
# Stub the md5 to match.
def file.md5
"X7A8HRvZUCT5gbq0KNDL8Q=="
# Stub the crc32c to match.
def file.crc32c
Digest::CRC32c.base64digest "yay!"
end

data = "yay!"
Expand All @@ -357,8 +359,8 @@ def file.md5
gzipped_data = gzip_data data
downloadio = StringIO.new

# Stub the md5 to match.
file.gapi.md5_hash = Digest::MD5.base64digest gzipped_data
# Stub the crc32c to match.
file.gapi.crc32c = Digest::CRC32c.base64digest gzipped_data

mock = Minitest::Mock.new
mock.expect :get_object, [StringIO.new(gzipped_data), download_http_resp(gzip: true)],
Expand All @@ -378,8 +380,8 @@ def file.md5
gzipped_data = gzip_data data
downloadio = StringIO.new

# Stub the md5 to match.
file.gapi.md5_hash = Digest::MD5.base64digest gzipped_data
# Stub the crc32c to match.
file.gapi.crc32c = Digest::CRC32c.base64digest gzipped_data

mock = Minitest::Mock.new
mock.expect :get_object, [StringIO.new(gzipped_data), download_http_resp(gzip: true)],
Expand All @@ -395,9 +397,9 @@ def file.md5
end

it "can download itself by specifying an IO" do
# Stub the md5 to match.
def file.md5
"X7A8HRvZUCT5gbq0KNDL8Q=="
# Stub the crc32c to match.
def file.crc32c
Digest::CRC32c.base64digest "yay!"
end

downloadio = StringIO.new
Expand All @@ -416,14 +418,15 @@ def file.md5
end

it "can download itself with customer-supplied encryption key" do
# Stub the md5 to match.
def file.md5
"1B2M2Y8AsgTpgAmY7PhCfg=="
# Stub the crc32c to match the content "yay!".
def file.crc32c
Digest::CRC32c.base64digest "yay!"
end

Tempfile.open "google-cloud" do |tmpfile|
# write to the file since the mocked call won't
tmpfile.write "yay!"
tmpfile.rewind

mock = Minitest::Mock.new
mock.expect :get_object, [nil, download_http_resp], # using encryption keys seems to return nil
Expand Down Expand Up @@ -469,7 +472,7 @@ def file.md5
end

describe "verified downloads" do
it "verifies m5d by default" do
it "verifies crc32c by default" do
# Stub these values
def file.md5; "md5="; end
def file.crc32c; "crc32c="; end
Expand All @@ -481,17 +484,17 @@ def file.crc32c; "crc32c="; end

bucket.service.mocked_service = mock

mocked_md5 = Minitest::Mock.new
mocked_md5.expect :md5_mock, file.md5
stubbed_md5 = lambda { |_| mocked_md5.md5_mock }
stubbed_crc32c = lambda { |_| fail "Should not be called!" }
stubbed_md5 = lambda { |_| fail "Should not be called!" }
mocked_crc32c = Minitest::Mock.new
mocked_crc32c.expect :crc32c_mock, file.crc32c
stubbed_crc32c = lambda { |_| mocked_crc32c.crc32c_mock }

Google::Cloud::Storage::File::Verifier.stub :md5_for, stubbed_md5 do
Google::Cloud::Storage::File::Verifier.stub :crc32c_for, stubbed_crc32c do
file.download tmpfile
end
end
mocked_md5.verify
mocked_crc32c.verify
mock.verify
end
end
Expand Down Expand Up @@ -637,10 +640,10 @@ def file.crc32c; "crc32c="; end

bucket.service.mocked_service = mock

mocked_md5 = Minitest::Mock.new
mocked_md5.expect :md5_mock, "NOPE="
stubbed_md5 = lambda { |_| mocked_md5.md5_mock }
stubbed_crc32c = lambda { |_| fail "Should not be called!" }
stubbed_md5 = lambda { |_| fail "Should not be called!" }
mocked_crc32c = Minitest::Mock.new
mocked_crc32c.expect :crc32c_mock, "NOPE!"
stubbed_crc32c = lambda { |_| mocked_crc32c.crc32c_mock }

Google::Cloud::Storage::File::Verifier.stub :md5_for, stubbed_md5 do
Google::Cloud::Storage::File::Verifier.stub :crc32c_for, stubbed_crc32c do
Expand All @@ -649,7 +652,7 @@ def file.crc32c; "crc32c="; end
end
end
end
mocked_md5.verify
mocked_crc32c.verify
mock.verify
end
end
Expand Down
22 changes: 11 additions & 11 deletions google-cloud-storage/test/google/cloud/storage/lazy/file_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ def file.md5
end

describe "verified downloads" do
it "verifies m5d by default" do
it "verifies crc32c by default" do
Copy link
Copy Markdown

@v-pratap v-pratap Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How it was working before, it was using m5d instead of md5? Does this test validate the actual use case?

Copy link
Copy Markdown
Contributor Author

@shubhangi-google shubhangi-google Jun 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part is just description of test case which would have been typed incorrectly

# Stub these values
def file.md5; "md5="; end
def file.crc32c; "crc32c="; end
Expand All @@ -332,17 +332,17 @@ def file.crc32c; "crc32c="; end

bucket.service.mocked_service = mock

mocked_md5 = Minitest::Mock.new
mocked_md5.expect :md5_mock, file.md5
stubbed_md5 = lambda { |_| mocked_md5.md5_mock }
stubbed_crc32c = lambda { |_| fail "Should not be called!" }
stubbed_md5 = lambda { |_| fail "Should not be called!" }
mocked_crc32c = Minitest::Mock.new
mocked_crc32c.expect :crc32c_mock, file.crc32c
stubbed_crc32c = lambda { |_| mocked_crc32c.crc32c_mock }

Google::Cloud::Storage::File::Verifier.stub :md5_for, stubbed_md5 do
Google::Cloud::Storage::File::Verifier.stub :crc32c_for, stubbed_crc32c do
file.download tmpfile
end
end
mocked_md5.verify
mocked_crc32c.verify
mock.verify
end
end
Expand Down Expand Up @@ -469,10 +469,10 @@ def file.crc32c; "crc32c="; end

bucket.service.mocked_service = mock

mocked_md5 = Minitest::Mock.new
mocked_md5.expect :md5_mock, "NOPE="
stubbed_md5 = lambda { |_| mocked_md5.md5_mock }
stubbed_crc32c = lambda { |_| fail "Should not be called!" }
stubbed_md5 = lambda { |_| fail "Should not be called!" }
mocked_crc32c = Minitest::Mock.new
mocked_crc32c.expect :crc32c_mock, "NOPE!"
stubbed_crc32c = lambda { |_| mocked_crc32c.crc32c_mock }

Google::Cloud::Storage::File::Verifier.stub :md5_for, stubbed_md5 do
Google::Cloud::Storage::File::Verifier.stub :crc32c_for, stubbed_crc32c do
Expand All @@ -481,7 +481,7 @@ def file.crc32c; "crc32c="; end
end
end
end
mocked_md5.verify
mocked_crc32c.verify
mock.verify
end
end
Expand Down
39 changes: 36 additions & 3 deletions google-cloud-storage/test/google/cloud/storage/project_anonymous_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,9 @@ def stub.list_buckets *args
bucket = anonymous_storage.bucket bucket_name
file = bucket.file file_name

# Stub the md5 to match.
def file.md5
"1B2M2Y8AsgTpgAmY7PhCfg=="
# Stub the crc32c to match.
def file.crc32c
Copy link
Copy Markdown

@v-pratap v-pratap Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add non empty file verification?

Copy link
Copy Markdown
Contributor Author

@shubhangi-google shubhangi-google Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"AAAAAA=="
end

downloaded = file.download tmpfile
Expand All @@ -126,6 +126,39 @@ def file.md5
end
end

it "downloads a public file and verifies the actual crc32c" do
file_name = "public-file.txt"
Tempfile.open "google-cloud" do |tmpfile|
tmpfile.write "yay!"

mock = Minitest::Mock.new
mock.expect :get_bucket, find_bucket_gapi(bucket_name), [bucket_name], **get_bucket_args
mock.expect :get_object, find_file_gapi(bucket_name, file_name), [bucket_name, file_name], **get_object_args
mock.expect :get_object, [tmpfile, download_http_resp],
[bucket_name, file_name], download_dest: tmpfile, generation: 1234567890, user_project: nil, options: {}

anonymous_storage.service.mocked_service = mock

bucket = anonymous_storage.bucket bucket_name
file = bucket.file file_name

# Stub the crc32c to match.
def file.crc32c
"AAAAAA=="
end

downloaded = file.download tmpfile
_(downloaded).must_be_kind_of Tempfile
_(downloaded.size).must_be :>, 0

local_data = downloaded.read
local_crc32c = Digest::CRC32c.base64digest local_data
_(local_crc32c).must_equal "AAAAAA=="

mock.verify
end
end

def find_bucket_gapi name = nil
Google::Apis::StorageV1::Bucket.from_json random_bucket_hash(name: name).to_json
end
Expand Down
Loading