Skip to content

Commit 447d35d

Browse files
BenWhiteheadJesseLovelacetritone
authored
fix(1.113.14-sp): backport critical bug fixes (#993)
* feat: Remove client side vaildation for lifecycle conditions (#816) * Remove client side vaildation for lifecycle conditions * fix lint and suggest updating (cherry picked from commit 5ec84cc) * fix: update BucketInfo translation code to properly handle lifecycle rules (#852) Fixes #850 (cherry picked from commit 3b1df1d) * fix: improve error detection and reporting for BlobWriteChannel retry state (#846) Add new checks to ensure a more informative error than NullPointerException is thrown if the StorageObject or it's size are unable to be resolved on the last chunk. Fixes #839 (cherry picked from commit d0f2184) * fix: correct lastChunk retry logic in BlobWriteChannel (#918) Add new method StorageRpc#queryResumableUpload which allows getting a shallow StorageObject for a resumable upload session which is complete. Update BlobWriteChannel to use StoageRpc#queryResumableUpload instead of StorageRpc#get when attempting to validate the upload size of an object when it determines the upload is complete and is on the last chunk. If a BlobWriteChannel is opened with a conditional like IfGenerationMatch it is not possible to simply get the object, as the object can drift generationally while the resumable upload is being performed. Related to #839 (cherry picked from commit ab0228c) * test: remove error string matching (#861) It looks like the text for this error on the backend has changed (sometimes) from "Precondition Failed" to "At least one of the pre-conditions you specified did not hold". I don't think it's really necessary to check the exact message in any case given that we do check for a code of 412, which implies a precondition failure. I added a check of the error Reason instead, which is more standardized. Fixes #853 (cherry picked from commit 146a3d3) Co-authored-by: JesseLovelace <43148100+JesseLovelace@users.noreply.github.com> Co-authored-by: Chris Cotter <cjcotter@google.com>
1 parent 8614975 commit 447d35d

File tree

9 files changed

+408
-61
lines changed

9 files changed

+408
-61
lines changed

‎google-cloud-storage/clirr-ignored-differences.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,4 +31,9 @@
3131
<method>long getCurrentUploadOffset(java.lang.String)</method>
3232
<differenceType>7012</differenceType>
3333
</difference>
34+
<difference>
35+
<className>com/google/cloud/storage/spi/v1/StorageRpc</className>
36+
<method>com.google.api.services.storage.model.StorageObject queryCompletedResumableUpload(java.lang.String, long)</method>
37+
<differenceType>7012</differenceType>
38+
</difference>
3439
</differences>

‎google-cloud-storage/src/main/java/com/google/cloud/storage/BlobWriteChannel.java‎

Lines changed: 85 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
import com.google.cloud.RetryHelper;
2626
import com.google.cloud.WriteChannel;
2727
import com.google.cloud.storage.spi.v1.StorageRpc;
28-
import com.google.common.collect.Maps;
28+
import java.math.BigInteger;
2929
import java.net.URL;
3030
import java.util.Map;
3131
import java.util.concurrent.Callable;
@@ -77,20 +77,52 @@ private long getRemotePosition() {
7777
return getOptions().getStorageRpcV1().getCurrentUploadOffset(getUploadId());
7878
}
7979

80-
private StorageObject getRemoteStorageObject() {
81-
return getOptions()
82-
.getStorageRpcV1()
83-
.get(getEntity().toPb(), Maps.newEnumMap(StorageRpc.Option.class));
80+
private static StorageException unrecoverableState(
81+
String uploadId,
82+
int chunkOffset,
83+
int chunkLength,
84+
long localPosition,
85+
long remotePosition,
86+
boolean last) {
87+
return unrecoverableState(
88+
uploadId,
89+
chunkOffset,
90+
chunkLength,
91+
localPosition,
92+
remotePosition,
93+
last,
94+
"Unable to recover in upload.\nThis may be a symptom of multiple clients uploading to the same upload session.");
95+
}
96+
97+
private static StorageException errorResolvingMetadataLastChunk(
98+
String uploadId,
99+
int chunkOffset,
100+
int chunkLength,
101+
long localPosition,
102+
long remotePosition,
103+
boolean last) {
104+
return unrecoverableState(
105+
uploadId,
106+
chunkOffset,
107+
chunkLength,
108+
localPosition,
109+
remotePosition,
110+
last,
111+
"Unable to load object metadata to determine if last chunk was successfully written");
84112
}
85113

86-
private StorageException unrecoverableState(
87-
int chunkOffset, int chunkLength, long localPosition, long remotePosition, boolean last) {
114+
private static StorageException unrecoverableState(
115+
String uploadId,
116+
int chunkOffset,
117+
int chunkLength,
118+
long localPosition,
119+
long remotePosition,
120+
boolean last,
121+
String message) {
88122
StringBuilder sb = new StringBuilder();
89-
sb.append("Unable to recover in upload.\n");
90-
sb.append(
91-
"This may be a symptom of multiple clients uploading to the same upload session.\n\n");
123+
sb.append(message).append("\n\n");
92124
sb.append("For debugging purposes:\n");
93-
sb.append("uploadId: ").append(getUploadId()).append('\n');
125+
sb.append("uploadId: ").append(uploadId).append('\n');
94126
sb.append("chunkOffset: ").append(chunkOffset).append('\n');
95127
sb.append("chunkLength: ").append(chunkLength).append('\n');
96128
sb.append("localOffset: ").append(localPosition).append('\n');
@@ -162,7 +194,7 @@ public void run() {
162194
// Get remote offset from API
163195
final long localPosition = getPosition();
164196
// For each request it should be possible to retry from its location in this code
165-
final long remotePosition = isRetrying() ? getRemotePosition() : getPosition();
197+
final long remotePosition = isRetrying() ? getRemotePosition() : localPosition;
166198
final int chunkOffset = (int) (remotePosition - localPosition);
167199
final int chunkLength = length - chunkOffset;
168200
final boolean uploadAlreadyComplete = remotePosition == -1;
@@ -173,13 +205,45 @@ public void run() {
173205
if (uploadAlreadyComplete && lastChunk) {
174206
// Case 6
175207
// Request object metadata if not available
208+
long totalBytes = getPosition() + length;
176209
if (storageObject == null) {
177-
storageObject = getRemoteStorageObject();
210+
storageObject =
211+
getOptions()
212+
.getStorageRpcV1()
213+
.queryCompletedResumableUpload(getUploadId(), totalBytes);
214+
}
215+
// the following checks are defined here explicitly to provide a more
216+
// informative if either storageObject is unable to be resolved or it's size is
217+
// unable to be determined. This scenario is a very rare case of failure that
218+
// can arise when packets are lost.
219+
if (storageObject == null) {
220+
throw errorResolvingMetadataLastChunk(
221+
getUploadId(),
222+
chunkOffset,
223+
chunkLength,
224+
localPosition,
225+
remotePosition,
226+
lastChunk);
178227
}
179228
// Verify that with the final chunk we match the blob length
180-
if (storageObject.getSize().longValue() != getPosition() + length) {
229+
BigInteger size = storageObject.getSize();
230+
if (size == null) {
231+
throw errorResolvingMetadataLastChunk(
232+
getUploadId(),
233+
chunkOffset,
234+
chunkLength,
235+
localPosition,
236+
remotePosition,
237+
lastChunk);
238+
}
239+
if (size.longValue() != totalBytes) {
181240
throw unrecoverableState(
182-
chunkOffset, chunkLength, localPosition, remotePosition, lastChunk);
241+
getUploadId(),
242+
chunkOffset,
243+
chunkLength,
244+
localPosition,
245+
remotePosition,
246+
lastChunk);
183247
}
184248
retrying = false;
185249
} else if (uploadAlreadyComplete && !lastChunk && !checkingForLastChunk) {
@@ -201,7 +265,12 @@ public void run() {
201265
} else {
202266
// Case 4 && Case 8 && Case 9
203267
throw unrecoverableState(
204-
chunkOffset, chunkLength, localPosition, remotePosition, lastChunk);
268+
getUploadId(),
269+
chunkOffset,
270+
chunkLength,
271+
localPosition,
272+
remotePosition,
273+
lastChunk);
205274
}
206275
}
207276
}),

‎google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java‎

Lines changed: 53 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,13 @@
4343
import java.io.ObjectInputStream;
4444
import java.io.ObjectOutputStream;
4545
import java.io.Serializable;
46+
import java.util.Collections;
4647
import java.util.HashSet;
4748
import java.util.List;
4849
import java.util.Map;
4950
import java.util.Objects;
5051
import java.util.Set;
52+
import java.util.logging.Logger;
5153

5254
/**
5355
* Google Storage bucket metadata;
@@ -101,6 +103,8 @@ public com.google.api.services.storage.model.Bucket apply(BucketInfo bucketInfo)
101103
private final String locationType;
102104
private final Logging logging;
103105

106+
private static final Logger log = Logger.getLogger(BucketInfo.class.getName());
107+
104108
/**
105109
* The Bucket's IAM Configuration.
106110
*
@@ -356,9 +360,11 @@ public LifecycleRule(LifecycleAction action, LifecycleCondition condition) {
356360
&& condition.getNoncurrentTimeBefore() == null
357361
&& condition.getCustomTimeBefore() == null
358362
&& condition.getDaysSinceCustomTime() == null) {
359-
throw new IllegalArgumentException(
360-
"You must specify at least one condition to use object lifecycle "
361-
+ "management. Please see https://cloud.google.com/storage/docs/lifecycle for details.");
363+
log.warning(
364+
"Creating a lifecycle condition with no supported conditions:\n"
365+
+ this
366+
+ "\nAttempting to update with this rule may cause errors. Please update "
367+
+ " to the latest version of google-cloud-storage");
362368
}
363369

364370
this.lifecycleAction = action;
@@ -1833,33 +1839,52 @@ public ObjectAccessControl apply(Acl acl) {
18331839
website.setNotFoundPage(notFoundPage);
18341840
bucketPb.setWebsite(website);
18351841
}
1836-
Set<Rule> rules = new HashSet<>();
1837-
if (deleteRules != null) {
1838-
rules.addAll(
1839-
transform(
1840-
deleteRules,
1841-
new Function<DeleteRule, Rule>() {
1842-
@Override
1843-
public Rule apply(DeleteRule deleteRule) {
1844-
return deleteRule.toPb();
1845-
}
1846-
}));
1847-
}
1848-
if (lifecycleRules != null) {
1849-
rules.addAll(
1850-
transform(
1851-
lifecycleRules,
1852-
new Function<LifecycleRule, Rule>() {
1853-
@Override
1854-
public Rule apply(LifecycleRule lifecycleRule) {
1855-
return lifecycleRule.toPb();
1856-
}
1857-
}));
1858-
}
18591842

1860-
if (rules != null) {
1843+
if (deleteRules != null || lifecycleRules != null) {
18611844
Lifecycle lifecycle = new Lifecycle();
1862-
lifecycle.setRule(ImmutableList.copyOf(rules));
1845+
1846+
// Here we determine if we need to "clear" any defined Lifecycle rules by explicitly setting
1847+
// the Rule list of lifecycle to the empty list.
1848+
// In order for us to clear the rules, one of the three following must be true:
1849+
// 1. deleteRules is null while lifecycleRules is non-null and empty
1850+
// 2. lifecycleRules is null while deleteRules is non-null and empty
1851+
// 3. lifecycleRules is non-null and empty while deleteRules is non-null and empty
1852+
// If none of the above three is true, we will interpret as the Lifecycle rules being
1853+
// updated to the defined set of DeleteRule and LifecycleRule.
1854+
if ((deleteRules == null && lifecycleRules.isEmpty())
1855+
|| (lifecycleRules == null && deleteRules.isEmpty())
1856+
|| (deleteRules != null && deleteRules.isEmpty() && lifecycleRules.isEmpty())) {
1857+
lifecycle.setRule(Collections.<Rule>emptyList());
1858+
} else {
1859+
Set<Rule> rules = new HashSet<>();
1860+
if (deleteRules != null) {
1861+
rules.addAll(
1862+
transform(
1863+
deleteRules,
1864+
new Function<DeleteRule, Rule>() {
1865+
@Override
1866+
public Rule apply(DeleteRule deleteRule) {
1867+
return deleteRule.toPb();
1868+
}
1869+
}));
1870+
}
1871+
if (lifecycleRules != null) {
1872+
rules.addAll(
1873+
transform(
1874+
lifecycleRules,
1875+
new Function<LifecycleRule, Rule>() {
1876+
@Override
1877+
public Rule apply(LifecycleRule lifecycleRule) {
1878+
return lifecycleRule.toPb();
1879+
}
1880+
}));
1881+
}
1882+
1883+
if (!rules.isEmpty()) {
1884+
lifecycle.setRule(ImmutableList.copyOf(rules));
1885+
}
1886+
}
1887+
18631888
bucketPb.setLifecycle(lifecycle);
18641889
}
18651890

‎google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java‎

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -799,6 +799,25 @@ public long getCurrentUploadOffset(String uploadId) {
799799
}
800800
}
801801

802+
@Override
803+
public StorageObject queryCompletedResumableUpload(String uploadId, long totalBytes) {
804+
try {
805+
GenericUrl url = new GenericUrl(uploadId);
806+
HttpRequest req = storage.getRequestFactory().buildPutRequest(url, new EmptyContent());
807+
req.getHeaders().setContentRange(String.format("bytes */%s", totalBytes));
808+
req.setParser(storage.getObjectParser());
809+
HttpResponse response = req.execute();
810+
// If the response is 200
811+
if (response.getStatusCode() == 200) {
812+
return response.parseAs(StorageObject.class);
813+
} else {
814+
throw buildStorageException(response.getStatusCode(), response.getStatusMessage());
815+
}
816+
} catch (IOException ex) {
817+
throw translate(ex);
818+
}
819+
}
820+
802821
@Override
803822
public StorageObject writeWithResponse(
804823
String uploadId,
@@ -864,10 +883,7 @@ public StorageObject writeWithResponse(
864883
if (exception != null) {
865884
throw exception;
866885
}
867-
GoogleJsonError error = new GoogleJsonError();
868-
error.setCode(code);
869-
error.setMessage(message);
870-
throw translate(error);
886+
throw buildStorageException(code, message);
871887
}
872888
} catch (IOException ex) {
873889
span.setStatus(Status.UNKNOWN.withDescription(ex.getMessage()));
@@ -914,10 +930,7 @@ public String open(StorageObject object, Map<Option, ?> options) {
914930
setEncryptionHeaders(requestHeaders, "x-goog-encryption-", options);
915931
HttpResponse response = httpRequest.execute();
916932
if (response.getStatusCode() != 200) {
917-
GoogleJsonError error = new GoogleJsonError();
918-
error.setCode(response.getStatusCode());
919-
error.setMessage(response.getStatusMessage());
920-
throw translate(error);
933+
throw buildStorageException(response.getStatusCode(), response.getStatusMessage());
921934
}
922935
return response.getHeaders().getLocation();
923936
} catch (IOException ex) {
@@ -947,10 +960,7 @@ public String open(String signedURL) {
947960
requestHeaders.set("x-goog-resumable", "start");
948961
HttpResponse response = httpRequest.execute();
949962
if (response.getStatusCode() != 201) {
950-
GoogleJsonError error = new GoogleJsonError();
951-
error.setCode(response.getStatusCode());
952-
error.setMessage(response.getStatusMessage());
953-
throw translate(error);
963+
throw buildStorageException(response.getStatusCode(), response.getStatusMessage());
954964
}
955965
return response.getHeaders().getLocation();
956966
} catch (IOException ex) {
@@ -1610,4 +1620,11 @@ public ServiceAccount getServiceAccount(String projectId) {
16101620
span.end(HttpStorageRpcSpans.END_SPAN_OPTIONS);
16111621
}
16121622
}
1623+
1624+
private static StorageException buildStorageException(int statusCode, String statusMessage) {
1625+
GoogleJsonError error = new GoogleJsonError();
1626+
error.setCode(statusCode);
1627+
error.setMessage(statusMessage);
1628+
return translate(error);
1629+
}
16131630
}

‎google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,24 @@ void write(
337337
*/
338338
long getCurrentUploadOffset(String uploadId);
339339

340+
/**
341+
* Attempts to retrieve the StorageObject from a completed resumable upload. When a resumable
342+
* upload completes, the response will be the up-to-date StorageObject metadata. This up-to-date
343+
* metadata can then be used to validate the total size of the object along with new generation
344+
* and other information.
345+
*
346+
* <p>If for any reason, the response to the final PUT to a resumable upload is not received, this
347+
* method can be used to query for the up-to-date StorageObject. If the upload is complete, this
348+
* method can be used to access the StorageObject independently from any other liveness or
349+
* conditional criteria requirements that are otherwise applicable when using {@link
350+
* #get(StorageObject, Map)}.
351+
*
352+
* @param uploadId resumable upload ID URL
353+
* @param totalBytes the total number of bytes that should have been written.
354+
* @throws StorageException if the upload is incomplete or does not exist
355+
*/
356+
StorageObject queryCompletedResumableUpload(String uploadId, long totalBytes);
357+
340358
/**
341359
* Writes the provided bytes to a storage object at the provided location. If {@code last=true}
342360
* returns metadata of the updated object, otherwise returns null.

‎google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,11 @@ public long getCurrentUploadOffset(String uploadId) {
144144
throw new UnsupportedOperationException("Not implemented yet");
145145
}
146146

147+
@Override
148+
public StorageObject queryCompletedResumableUpload(String uploadId, long totalBytes) {
149+
throw new UnsupportedOperationException("Not implemented yet");
150+
}
151+
147152
@Override
148153
public StorageObject writeWithResponse(
149154
String uploadId,

0 commit comments

Comments
 (0)