Skip to content

Commit

Permalink
Check contentType != null before adding object to rewrite
Browse files Browse the repository at this point in the history
  • Loading branch information
mziccard committed Oct 28, 2015
1 parent 43460b9 commit f90e70c
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -376,16 +376,15 @@ public void run(Storage storage, CopyRequest request) {

@Override
CopyRequest parse(String... args) {
if (args.length != 5) {
if (args.length != 4) {
throw new IllegalArgumentException();
}
return CopyRequest.of(args[0], args[1],
BlobInfo.builder(args[2], args[3]).contentType(args[4]).build());
return CopyRequest.of(args[0], args[1], BlobInfo.builder(args[2], args[3]).build());
}

@Override
public String params() {
return "<from_bucket> <from_path> <to_bucket> <to_path> <to_content_type>";
return "<from_bucket> <from_path> <to_bucket> <to_path>";
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ private RewriteResponse rewrite(RewriteRequest req, String token) throws Storage
? req.megabytesRewrittenPerCall * MEGABYTE : null;
com.google.api.services.storage.model.RewriteResponse rewriteReponse = storage.objects()
.rewrite(req.source.getBucket(), req.source.getName(), req.target.getBucket(),
req.target.getName(), req.target)
req.target.getName(), req.target.getContentType() != null ? req.target : null)

This comment has been minimized.

Copy link
@aozarov

aozarov Oct 28, 2015

Yes, now it is consistent with the previous copy, but I do wonder if that is the right thing to do (silently
ignore any other overridable content such as metadata if content-type is missing...)

At the very least we should probably document CopyRequest.Builder.target(BlobInfo) (is that the only place
where one could provide BlobInfo for a target?) with this kind of side-effect.

Another possible way to make this issue less surprising could be by changing CopyRequest.Builder
to have 2 separate type of setters for target: target(BlobId) and target(BlobInfo, BlobTargetOption...) [and/or target(BlobInfo, Iterable<BlobTargetOption>)]. In that case, we could document [and possible throw an IllegalArgumentException] if the given BlobInfo is missing content-type.

/cc @BrandonY

This comment has been minimized.

Copy link
@mziccard

mziccard Oct 28, 2015

Author Owner

Yes we should at least document this. We also have CopyRequest.of(String sourceBucket, String sourceBlob, BlobInfo target) that allows to set a BlobInfo for target.

I don't dislike this second option but we almost never do this kind of checks across the library. Even though in this case we are checking for something that has to deal with our own code and not the remote services. If we go this way we should probably document and throw in CopyRequest.of as well.

This comment has been minimized.

Copy link
@aozarov

aozarov Oct 29, 2015

created googleapis#306

.setRewriteToken(token)
.setMaxBytesRewrittenPerCall(maxBytesRewrittenPerCall)
.setProjection(DEFAULT_PROJECTION)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -655,6 +655,16 @@ public static CopyRequest of(BlobId sourceBlobId, BlobInfo target) {
return builder().source(sourceBlobId).target(target).build();
}

public static CopyRequest of(String sourceBucket, String sourceBlob, String targetBlob) {
return of(sourceBucket, sourceBlob,
BlobInfo.builder(BlobId.of(sourceBucket, targetBlob)).build());
}

public static CopyRequest of(BlobId sourceBlobId, String targetBlob) {
return of(sourceBlobId,
BlobInfo.builder(BlobId.of(sourceBlobId.bucket(), targetBlob)).build());
}

public static Builder builder() {
return new Builder();
}
Expand Down

0 comments on commit f90e70c

Please sign in to comment.