Re: [jclouds] Add support for Azure Copy Blob (#514)
@demobox Thanks for the feedback and sorry for my delayed responses; please see the latest commit. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514#issuecomment-87526600
Re: [jclouds] Add support for Azure Copy Blob (#514)
> +
> +import java.util.Map;
> +
> +import com.google.common.base.Optional;
> +import com.google.common.collect.ImmutableMap;
> +
> +/**
> + * @see http://msdn.microsoft.com/en-us/library/dd894037.aspx"; />
> + */
> +// TODO:
> +// x-ms-source-if-modified-since
> +// x-ms-source-if-unmodified-since
> +// x-ms-source-if-match
> +// x-ms-source-if-none-match
> +public final class CopyBlobOptions {
> + private final Optional> userMetadata;
This is an interesting corner case -- Azure does not seem to allow replacing
user metadata with an empty map?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r27364672
Re: [jclouds] Add support for Azure Copy Blob (#514)
> +package org.jclouds.azureblob.binders;
> +
> +import java.util.Map;
> +
> +import org.jclouds.azure.storage.reference.AzureStorageHeaders;
> +import org.jclouds.azureblob.options.CopyBlobOptions;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.rest.Binder;
> +
> +import com.google.common.base.Optional;
> +
> +/** Binds options to a copyBlob request. */
> +public class BindAzureCopyOptionsToRequest implements Binder {
> + @Override
> + public R bindToRequest(R request, Object input) {
> + Optional optional = (Optional) input;
Changed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r27364624
Re: [jclouds] Add support for Azure Copy Blob (#514)
> + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +package org.jclouds.azureblob.options; > + > +import java.util.Map; > + > +import com.google.common.base.Optional; > +import com.google.common.collect.ImmutableMap; > + > +/** > + * @see http://msdn.microsoft.com/en-us/library/dd894037.aspx"; /> Done. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514/files#r27364616
Re: [jclouds] Add support for Azure Copy Blob (#514)
> @@ -375,4 +380,23 @@ public void testBlockOperations() throws Exception {
>assertEquals(1, blocks.getBlocks().get(2).getContentLength());
>getApi().deleteContainer(blockContainer);
> }
> +
> + @Test(timeOut = 5 * 60 * 1000, dependsOnMethods = {
> "testCreateContainer", "testCreatePublicContainer" })
> + public void testCopyBlob() throws Exception {
> + ByteSource byteSource = TestUtils.randomByteSource().slice(0, 1024);
> +
> + // create blob
> + AzureBlob object = getApi().newBlob();
> + object.getProperties().setName("from");
> + object.setPayload(byteSource.read());
> + getApi().putBlob(privateContainer, object);
> +
> + // copy blob
> + URI copySource = view.getSigner().signGetBlob(privateContainer,
> "from").getEndpoint();
> + getApi().copyBlob(copySource, privateContainer, "to",
> Optional.absent());
Normal test teardown handles this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r27364638
Re: [jclouds] Add support for Azure Copy Blob (#514)
> +public class BindAzureCopyOptionsToRequest implements Binder {
> + @Override
> + public R bindToRequest(R request, Object input) {
> + Optional optional = (Optional) input;
> + if (!optional.isPresent()) {
> + return request;
> + }
> +
> + HttpRequest.Builder builder = request.toBuilder();
> + CopyBlobOptions options = optional.get();
> + Optional> userMetadata = options.getUserMetadata();
> + if (userMetadata.isPresent()) {
> + for (Map.Entry entry :
> userMetadata.get().entrySet()) {
> +builder.addHeader(AzureStorageHeaders.USER_METADATA_PREFIX +
> entry.getKey(), entry.getValue());
> + }
> + }
Correct.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r27364619
Re: [jclouds] Add support for Azure Copy Blob (#514)
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +package org.jclouds.azureblob.options; > + > +import java.util.Map; > + > +import com.google.common.base.Optional; > +import com.google.common.collect.ImmutableMap; > + > +/** > + * @see http://msdn.microsoft.com/en-us/library/dd894037.aspx"; /> > + */ > +// TODO: Removed/implemented. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514/files#r27364611
Re: [jclouds] Add support for Azure Copy Blob (#514)
> @@ -254,4 +257,8 @@ > */ > boolean blobExists(String container, String name); > > + /** > +* @throws ContainerNotFoundException if the container is not present. > +*/ > + void copyBlob(URI copySource, String toContainer, String toName, > Optional options); Changed to use a single non-`Optional` parameter and added `CopyBlobOptions.NONE`. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514/files#r27364606
Re: [jclouds] Add support for Azure Copy Blob (#514)
Thanks for the pull request but it's release week in jclouds and that means it's time to clean up the PR queue. This PR will be over 6 months old as of April 1. If you intend to continue work on it, please make a comment by April 2. Otherwise it will be closed on April 3. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514#issuecomment-85715395
Re: [jclouds] Add support for Azure Copy Blob (#514)
sorry that unasyncing probably screwed up this branch. let me know if you want a hand rebasing. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514#issuecomment-57894560
Re: [jclouds] Add support for Azure Copy Blob (#514)
> @@ -375,4 +380,23 @@ public void testBlockOperations() throws Exception {
>assertEquals(1, blocks.getBlocks().get(2).getContentLength());
>getApi().deleteContainer(blockContainer);
> }
> +
> + @Test(timeOut = 5 * 60 * 1000, dependsOnMethods = {
> "testCreateContainer", "testCreatePublicContainer" })
> + public void testCopyBlob() throws Exception {
> + ByteSource byteSource = TestUtils.randomByteSource().slice(0, 1024);
> +
> + // create blob
> + AzureBlob object = getApi().newBlob();
> + object.getProperties().setName("from");
> + object.setPayload(byteSource.read());
> + getApi().putBlob(privateContainer, object);
> +
> + // copy blob
> + URI copySource = view.getSigner().signGetBlob(privateContainer,
> "from").getEndpoint();
> + getApi().copyBlob(copySource, privateContainer, "to",
> Optional.absent());
Do we need to clean this up afterwards? Or is that already done somewhere?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r17994124
Re: [jclouds] Add support for Azure Copy Blob (#514)
> +
> +import java.util.Map;
> +
> +import com.google.common.base.Optional;
> +import com.google.common.collect.ImmutableMap;
> +
> +/**
> + * @see http://msdn.microsoft.com/en-us/library/dd894037.aspx"; />
> + */
> +// TODO:
> +// x-ms-source-if-modified-since
> +// x-ms-source-if-unmodified-since
> +// x-ms-source-if-match
> +// x-ms-source-if-none-match
> +public final class CopyBlobOptions {
> + private final Optional> userMetadata;
Instead of an optional, how about simply an empty map by default? That would
have the same effect - no headers getting added to the request - unless I'm
missing something? And would avoid a few checks?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r17994046
Re: [jclouds] Add support for Azure Copy Blob (#514)
> @@ -254,4 +257,8 @@ > */ > boolean blobExists(String container, String name); > > + /** > +* @throws ContainerNotFoundException if the container is not present. > +*/ > + void copyBlob(URI copySource, String toContainer, String toName, > Optional options); We're preferring `Optional` to an overload here? Is this a pattern we use elsewhere? Otherwise, how about two methods: ``` copyBlob(URI copySource, String toContainer, String toName); copyBlob(URI copySource, String toContainer, String toName, CopyBlobOptions options); ``` or ``` copyBlob(URI copySource, String toContainer, String toName, CopyBlobOptions... options); ``` ? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514/files#r17993973
Re: [jclouds] Add support for Azure Copy Blob (#514)
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +package org.jclouds.azureblob.options; > + > +import java.util.Map; > + > +import com.google.common.base.Optional; > +import com.google.common.collect.ImmutableMap; > + > +/** > + * @see http://msdn.microsoft.com/en-us/library/dd894037.aspx"; /> > + */ > +// TODO: Do we keep TODOs? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514/files#r17993866
Re: [jclouds] Add support for Azure Copy Blob (#514)
> + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +package org.jclouds.azureblob.options; > + > +import java.util.Map; > + > +import com.google.common.base.Optional; > +import com.google.common.collect.ImmutableMap; > + > +/** > + * @see http://msdn.microsoft.com/en-us/library/dd894037.aspx"; /> Remove external URLs? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514/files#r17993857
Re: [jclouds] Add support for Azure Copy Blob (#514)
> +public class BindAzureCopyOptionsToRequest implements Binder {
> + @Override
> + public R bindToRequest(R request, Object input) {
> + Optional optional = (Optional) input;
> + if (!optional.isPresent()) {
> + return request;
> + }
> +
> + HttpRequest.Builder builder = request.toBuilder();
> + CopyBlobOptions options = optional.get();
> + Optional> userMetadata = options.getUserMetadata();
> + if (userMetadata.isPresent()) {
> + for (Map.Entry entry :
> userMetadata.get().entrySet()) {
> +builder.addHeader(AzureStorageHeaders.USER_METADATA_PREFIX +
> entry.getKey(), entry.getValue());
> + }
> + }
Just to confirm: what we're doing here is copying the user metadata from
`options` into the request as headers..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r17993839
Re: [jclouds] Add support for Azure Copy Blob (#514)
> +package org.jclouds.azureblob.binders;
> +
> +import java.util.Map;
> +
> +import org.jclouds.azure.storage.reference.AzureStorageHeaders;
> +import org.jclouds.azureblob.options.CopyBlobOptions;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.rest.Binder;
> +
> +import com.google.common.base.Optional;
> +
> +/** Binds options to a copyBlob request. */
> +public class BindAzureCopyOptionsToRequest implements Binder {
> + @Override
> + public R bindToRequest(R request, Object input) {
> + Optional optional = (Optional) input;
[minor] `optionalOptions` or so?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r17993750
Re: [jclouds] Add support for Azure Copy Blob (#514)
> +package org.jclouds.azureblob.binders;
> +
> +import java.util.Map;
> +
> +import org.jclouds.azure.storage.reference.AzureStorageHeaders;
> +import org.jclouds.azureblob.options.CopyBlobOptions;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.rest.Binder;
> +
> +import com.google.common.base.Optional;
> +
> +/** Binds options to a copyBlob request. */
> +public class BindAzureCopyOptionsToRequest implements Binder {
> + @Override
> + public R bindToRequest(R request, Object input) {
> + Optional optional = (Optional) input;
We don't usually have something like a `checkArg` or `checkState` to verify the
type here, I guess?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r17993721
Re: [jclouds] Add support for Azure Copy Blob (#514)
[jclouds-pull-requests #1158](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1158/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514#issuecomment-55030602
Re: [jclouds] Add support for Azure Copy Blob (#514)
[jclouds-pull-requests-java-6 #69](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/69/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514#issuecomment-55020580
Re: [jclouds] Add support for Azure Copy Blob (#514)
Going with a URI-based copy source. This allows copies between different accounts via signed URLs. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514#issuecomment-55011739
Re: [jclouds] Add support for Azure Copy Blob (#514)
> @@ -325,4 +330,16 @@
> ListenableFuture
> getBlockList(@PathParam("container")
> @ParamValidators(ContainerNameValidator.class) String container,
> @PathParam("name")
> String name);
>
> + /**
> +* @see AzureBlobClient#copyBlob
> +*/
> + @Named("CopyBlob")
> + @PUT
> + @Path("{toContainer}/{toName}")
> + @OverrideRequestFilters
> + @RequestFilters(value = { AddCopySourceHeader.class,
> SharedKeyLiteAuthentication.class })
Surprisingly, this works, thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r17316411
Re: [jclouds] Add support for Azure Copy Blob (#514)
> @@ -325,4 +330,16 @@
> ListenableFuture
> getBlockList(@PathParam("container")
> @ParamValidators(ContainerNameValidator.class) String container,
> @PathParam("name")
> String name);
>
> + /**
> +* @see AzureBlobClient#copyBlob
> +*/
> + @Named("CopyBlob")
> + @PUT
> + @Path("{toContainer}/{toName}")
> + @OverrideRequestFilters
> + @RequestFilters(value = { AddCopySourceHeader.class,
> SharedKeyLiteAuthentication.class })
@gaul mind opening a JIRA to improve that and assign it to me?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r17316265
Re: [jclouds] Add support for Azure Copy Blob (#514)
> @@ -325,4 +330,16 @@
> ListenableFuture
> getBlockList(@PathParam("container")
> @ParamValidators(ContainerNameValidator.class) String container,
> @PathParam("name")
> String name);
>
> + /**
> +* @see AzureBlobClient#copyBlob
> +*/
> + @Named("CopyBlob")
> + @PUT
> + @Path("{toContainer}/{toName}")
> + @OverrideRequestFilters
> + @RequestFilters(value = { AddCopySourceHeader.class,
> SharedKeyLiteAuthentication.class })
What if you instead use `@PathParam` instead of `@HeaderParam` on the `from*`
args?
```java
@Named("CopyBlob")
@PUT
@Path("{toContainer}/{toName}")
@Headers(keys = AzureStorageHeaders.COPY_SOURCE, values =
"/{fromContainer}/{fromName}")
ListenableFuture copyBlob(
@PathParam("fromContainer")
@ParamValidators(ContainerNameValidator.class) String fromContainer,
@PathParam("fromName") String fromName,
@PathParam("toContainer") @ParamValidators(ContainerNameValidator.class)
String toContainer,
@PathParam("toName") String toName,
Optional options);
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r17315947
Re: [jclouds] Add support for Azure Copy Blob (#514)
> @@ -325,4 +330,16 @@
> ListenableFuture
> getBlockList(@PathParam("container")
> @ParamValidators(ContainerNameValidator.class) String container,
> @PathParam("name")
> String name);
>
> + /**
> +* @see AzureBlobClient#copyBlob
> +*/
> + @Named("CopyBlob")
> + @PUT
> + @Path("{toContainer}/{toName}")
> + @OverrideRequestFilters
> + @RequestFilters(value = { AddCopySourceHeader.class,
> SharedKeyLiteAuthentication.class })
Unfortunately `RestAnnotationProcessor.buildHeaders` does not replace headers
with headerParams. The following:
```java
@Named("CopyBlob")
@PUT
@Path("{toContainer}/{toName}")
@Headers(keys = AzureStorageHeaders.COPY_SOURCE, values =
"/{fromContainer}/{fromName}")
ListenableFuture copyBlob(
@HeaderParam("fromContainer")
@ParamValidators(ContainerNameValidator.class) String fromContainer,
@HeaderParam("fromName") String fromName,
@PathParam("toContainer")
@ParamValidators(ContainerNameValidator.class) String toContainer,
@PathParam("toName") String toName,
Optional options);
```
results in:
```
fromContainer: fromcontainer
fromName: fromblob
x-ms-copy-source: /{fromContainer}/{fromName}
x-ms-version: 2012-02-12
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r17315624
Re: [jclouds] Add support for Azure Copy Blob (#514)
> @@ -325,4 +330,16 @@
> ListenableFuture
> getBlockList(@PathParam("container")
> @ParamValidators(ContainerNameValidator.class) String container,
> @PathParam("name")
> String name);
>
> + /**
> +* @see AzureBlobClient#copyBlob
> +*/
> + @Named("CopyBlob")
> + @PUT
> + @Path("{toContainer}/{toName}")
> + @OverrideRequestFilters
> + @RequestFilters(value = { AddCopySourceHeader.class,
> SharedKeyLiteAuthentication.class })
@andrewgaul That syntax is a bit strange, and I know what your intent is here.
Rather than using a `@RequestFilter`, what about just adding this simple header
annotation? `@Headers(keys = COPY_SOURCE, values =
"/{fromContainer}/{fromName}")``
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r17314515
Re: [jclouds] Add support for Azure Copy Blob (#514)
[jclouds » jclouds #1609](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1609/) FAILURE Looks like there's a problem with this pull request [(what's this?)](https://www.cloudbees.com/what-is-buildhive) --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514#issuecomment-54738923
Re: [jclouds] Add support for Azure Copy Blob (#514)
[jclouds-pull-requests #1155](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1155/) FAILURE Looks like there's a problem with this pull request --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514#issuecomment-54738511
Re: [jclouds] Add support for Azure Copy Blob (#514)
[jclouds » jclouds #1608](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1608/) FAILURE Looks like there's a problem with this pull request [(what's this?)](https://www.cloudbees.com/what-is-buildhive) --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514#issuecomment-54738364
Re: [jclouds] Add support for Azure Copy Blob (#514)
[jclouds-pull-requests-java-6 #67](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/67/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514#issuecomment-54737921
Re: [jclouds] Add support for Azure Copy Blob (#514)
I am still digesting the variants of `CopyBlobOptions`; it seems like Azure and S3 implement the same ETag variants and copy/replace user metadata operations. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514#issuecomment-54737804
Re: [jclouds] Add support for Azure Copy Blob (#514)
> @@ -325,4 +330,16 @@
> ListenableFuture
> getBlockList(@PathParam("container")
> @ParamValidators(ContainerNameValidator.class) String container,
> @PathParam("name")
> String name);
>
> + /**
> +* @see AzureBlobClient#copyBlob
> +*/
> + @Named("CopyBlob")
> + @PUT
> + @Path("{toContainer}/{toName}")
> + @OverrideRequestFilters
> + @RequestFilters(value = { AddCopySourceHeader.class,
> SharedKeyLiteAuthentication.class })
Unsure about `AddCopySourceHeader`; this feels like a clumsy way to implement a
compound header. I modeled the interface after the S3 equivalent but perhaps
this is the wrong approach. If I change `fromContainer` and `fromName` to
`fromUrl` I can avoid this entirely and expose the full power of the Azure
interface. Thoughts?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/514/files#r17213002
Re: [jclouds] Add support for Azure Copy Blob (#514)
[jclouds-pull-requests-java-6 #66](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/66/) SUCCESS This pull request looks good --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514#issuecomment-54737736
[jclouds] Add support for Azure Copy Blob (#514)
Presently lacks support for CopyBlobOptions. API reference: http://msdn.microsoft.com/en-us/library/dd894037.aspx You can merge this Pull Request by running: git pull https://github.com/andrewgaul/jclouds azure-copy-blob Or you can view, comment on it, or merge it online at: https://github.com/jclouds/jclouds/pull/514 -- Commit Summary -- * Add support for Azure Copy Blob -- File Changes -- M common/azure/src/main/java/org/jclouds/azure/storage/reference/AzureStorageHeaders.java (2) M providers/azureblob/src/main/java/org/jclouds/azureblob/AzureBlobAsyncClient.java (18) M providers/azureblob/src/main/java/org/jclouds/azureblob/AzureBlobClient.java (6) A providers/azureblob/src/main/java/org/jclouds/azureblob/filters/AddCopySourceHeader.java (45) M providers/azureblob/src/test/java/org/jclouds/azureblob/AzureBlobAsyncClientTest.java (20) M providers/azureblob/src/test/java/org/jclouds/azureblob/AzureBlobClientLiveTest.java (23) -- Patch Links -- https://github.com/jclouds/jclouds/pull/514.patch https://github.com/jclouds/jclouds/pull/514.diff --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/514
