feat(gRPC): implement BidiWriteObject

[cojenco]

Fixes #568

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8480153) 98.60% compared to head (47ec9fe) 98.63%.
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
+ Coverage   98.60%   98.63%   +0.03%     
==========================================
  Files          50       50              
  Lines        8072     8284     +212     
==========================================
+ Hits         7959     8171     +212     
  Misses        113      113              

Flag	Coverage Δ
unittests	98.63% <100.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[coryan]

A couple of optional things and a question.

[coryan]

Checking this after doing upload.media += content seems like a bug.

Seems like this check should be around line 350 or so?

[cojenco]

This is to handle cases with missing BidiWriteObjectRequest. I probably went overboard, and there's also a test case for this gcs.upload.Upload.process_bidi_write_object_grpc(db, [], context)

storage-testbench/tests/test_upload.py

Lines 646 to 653 in 65105b6

	def test_init_object_write_grpc_empty(self):
	db = unittest.mock.Mock()
	context = self.mock_context()
	upload, _ = gcs.upload.Upload.init_write_object_grpc(db, [], context)
	self.assertIsNone(upload)
	context.abort.assert_called_once_with(
	grpc.StatusCode.INVALID_ARGUMENT, unittest.mock.ANY
	)

[coryan]

Right, but if upload is None then line 359 would have raised an exception before you got here, right? You probably want to detect the problem before the first use of upload. Did I miss something? If so, please just merge, otherwise please reorder the test to prevent crashes exceptions.

[cojenco]

Inside the request loop on L307, we're checking if upload is None before the first use.

This extra check is outside the request loop. It covers the rare case of missing requests, and prevents crashes in the attempt to finalize an upload

[coryan]

Right, but if upload is None then line 359 would have raised an exception before you got here, right? You probably want to detect the problem before the first use of upload. Did I miss something? If so, please just merge, otherwise please reorder the test to prevent crashes exceptions.