You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Jeremy Daggett <no...@github.com> on 2013/07/17 22:58:20 UTC
[jclouds-labs-openstack] New Swift Account API, along with models,
and expect/live tests (#17)
Here is the initial rework of the Swift Account API. Feedback welcomed!
You can merge this Pull Request by running:
git pull https://github.com/rackerlabs/jclouds-labs-openstack account-api
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-openstack/pull/17
-- Commit Summary --
* New Swift Account API, along with models, and expect/live tests
-- File Changes --
M openstack-swift/pom.xml (5)
A openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/binders/BindAccountMetadataToDeleteToHeaders.java (86)
A openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/binders/BindAccountMetadataToHeaders.java (74)
M openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/domain/Account.java (113)
A openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/domain/Metadata.java (58)
A openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/domain/internal/SwiftResource.java (231)
M openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/features/AccountApi.java (89)
A openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/functions/ParseAccountFromHeaders.java (80)
A openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/functions/ParseAccountMetadataFromHeaders.java (54)
D openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/functions/ParseAccountMetadataResponseFromHeaders.java (40)
A openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/reference/SwiftConstants.java (83)
A openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/reference/SwiftHeaders.java (109)
M openstack-swift/src/test/java/org/jclouds/openstack/swift/v1/PasswordAuthenticationExpectTest.java (6)
M openstack-swift/src/test/java/org/jclouds/openstack/swift/v1/features/AccountApiExpectTest.java (204)
M openstack-swift/src/test/java/org/jclouds/openstack/swift/v1/features/AccountApiLiveTest.java (77)
M openstack-swift/src/test/java/org/jclouds/openstack/swift/v1/features/ContainerApiExpectTest.java (45)
M openstack-swift/src/test/java/org/jclouds/openstack/swift/v1/internal/BaseSwiftApiExpectTest.java (5)
M openstack-swift/src/test/java/org/jclouds/openstack/swift/v1/internal/BaseSwiftExpectTest.java (127)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-openstack/pull/17.patch
https://github.com/jclouds/jclouds-labs-openstack/pull/17.diff
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + * Static utility method to convert a Python timestamp to a Java {@link Date}. For example,
> + * the timestamp returned from the API is a String in the form "1368738007.90714".
> + * This method handles the necessary conversion.
> + *
> + * @param inTimestamp the timestamp to convert.
> + *
> + * @return the converted {@link Date}
> + *
> + * @see Python's <a href="http://docs.python.org/2/library/time.html"><code>time.time()</code></a> method
> + */
> + public static Date convertTimestamp(String inTimestamp) {
> + checkNotNull(inTimestamp, "inTimestamp");
> + Float timestamp = Float.parseFloat(inTimestamp) * 1000;
> +
> + return new Date(timestamp.longValue());
> + }
Extract these three utility methods to another class? They don't seem to contain any SwiftResource-specific code.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383535
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Zack Shoylev <no...@github.com>.
> @@ -65,6 +67,6 @@ public void testListContainersWhenResponseIs2xx() throws Exception {
>
> assertEquals(apiWhenContainersExist.getContainerApiForRegion("region-a.geo-1").list().toString(),
> new ParseContainerListTest().expected().toString());
> - }
> + }
whitespace nit
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5431913
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Gaul <no...@github.com>.
> + checkNotNull(inTimestamp, "inTimestamp");
> + Float timestamp = Float.parseFloat(inTimestamp) * 1000;
> +
> + return new Date(timestamp.longValue());
> + }
> +
> + @Override
> + public int hashCode() {
> + return Objects.hashCode(transactionId, timestamp, metadata);
> + }
> +
> + @Override
> + public boolean equals(Object obj) {
> + if (this == obj) return true;
> + if (obj == null || getClass() != obj.getClass()) return false;
> + SwiftResource that = SwiftResource.class.cast(obj);
Same comments about equals().
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6380879
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> + FN2 = new Function<String, String>() {
> +
> + @Override
> + public String apply(String element) {
> + return "";
> + }
> + };
> + }
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof Map<?,?>, "this binder is only valid for Maps!");
> + checkNotNull(request, "request");
> +
> + Map<String, String> userMetadata = Maps2.transformKeys((Map<String, String>) input, FN);
Eclipse doesn't seem to like the fact that I tried to remove the cast. Leaving in for now...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251426
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> protected long bytesUsed;
> -
> +
Eclipse formatter... Fixing.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6252018
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Gaul <no...@github.com>.
> @@ -115,6 +138,26 @@ public String toString() {
> }
>
> protected ToStringHelper string() {
> - return toStringHelper("").add("containerCount", getContainerCount()).add("bytesUsed", getBytesUsed());
> + return super.string()
> + .add("containerCount", getContainerCount())
> + .add("objectCount", getObjectCount())
> + .add("bytesUsed", getBytesUsed());
> + }
> +
> + @Override
> + public boolean equals(Object object) {
> + if (this == object) {
> + return true;
> + }
> + if (object instanceof Account) {
> + final Account other = Account.class.cast(object);
Do you want to use the cast operator instead of the class cast method:
http://stackoverflow.com/questions/1555326/java-class-cast-vs-cast-operator
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5325800
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + * Binds the Account Metadata to the HTTP request headers.
> + *
> + * @see {@link Binder}
> + * @see {@link Account}
> + */
> +@Singleton
> +public class BindAccountMetadataToHeaders implements Binder {
> +
> + private final Function<String, String> transformMetadataKeysFN;
> +
> + public BindAccountMetadataToHeaders() {
> +
> + transformMetadataKeysFN = new Function<String, String>() {
> +
> + @Override
> + public String apply(String element) {
Looks like a function used in multiple places. Move out to a util class?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383363
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #35](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/35/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-24118791
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -65,6 +67,6 @@ public void testListContainersWhenResponseIs2xx() throws Exception {
>
> assertEquals(apiWhenContainersExist.getContainerApiForRegion("region-a.geo-1").list().toString(),
> new ParseContainerListTest().expected().toString());
> - }
> + }
See above comment...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6252001
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -46,7 +48,7 @@ protected Properties setupProperties() {
> contextProperties.setProperty("jclouds.keystone.credential-type", "passwordCredentials");
> return contextProperties;
> }
> -
> +
See above comment...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6252000
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> +import org.jclouds.rest.Binder;
> +import org.jclouds.util.Maps2;
> +
> +import com.google.common.base.Function;
> +import com.google.common.collect.Multimaps;
> +
> +/**
> + * Binds the Account Metadata to the HTTP request headers.
> + *
> + * @see {@link Binder}
> + * @see {@link Account}
> + * @see {@link Metadata}
> + */
> +@Singleton
> +public class BindAccountMetadataToHeaders implements Binder {
> + private final Function<String, String> FN;
Give a more descriptive name?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5397926
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Zack Shoylev <no...@github.com>.
> +
> + protected ToStringHelper string() {
> + return Objects.toStringHelper(this)
> + .add("transactionId", transactionId).add("timestamp", timestamp).add("metadata", metadata);
> + }
> +
> + @Override
> + public int compareTo(SwiftResource that) {
> + if (that == null)
> + return 1;
> + if (this == that)
> + return 0;
> + return this.getTransactionId().compareTo(that.getTransactionId());
> + }
> +
> +}
New line
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5431799
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + int MAX_OBJECTS = 10000;
> +
> + /**
> + * The maximum length of a container name is 256 (bytes).
> + */
> + int MAX_LENGTH_CONTAINER_NAME = 256;
> +
> + /**
> + * The maximum length of an object name is 1024 (bytes).
> + */
> + int MAX_LENGTH_OBJECT_NAME = 1024;
> +
> + /**
> + * The maximum length of HTTP requests is 5 (gigabytes).
> + */
> + int MAX_LENGTH_HTTP_REQUEST = 5;
Perhaps add the unit to the constant name, to avoid this kind of potential confusion? E.g. `MAX_HTTP_REQUEST_SIZE_GB`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383756
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> + * This class represents a generic model for Metadata.
> + *
> + * Requests made to the various metadata APIs, will utilize this generic metadata.
> + *
> + * <p/>Prefixed headers, such as {@link SwiftHeaders#ACCOUNT_META_PREFIX},
> + * {@link SwiftHeaders#CONTAINER_META_PREFIX}, and {@link SwiftHeaders#ACCOUNT_META_PREFIX}
> + * will stripped via one of the parsing classes in {@link org.jclouds.openstack.swift.functions}
> + * when metadata is requested from the API.
> + *
> + * <p/>The Metadata related API calls, <code>createOrUpdateMetadata(Metadata)</code> and
> + * <code>deleteMetadata(Metadata)</code> will bind the prefixes to the values using one of the
> + * binders in {@link org.jclouds.openstack.swift.v1.binders}.
> + *
> + * @see {@link AccountApi}
> + */
> +public class Metadata extends ForwardingMap<String, String> {
My hopes for universal Metadata in all OpenStack projects was recently dashed. :( Changing to Map<String, String>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251480
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> +
> + return metadata;
> + }
> +
> + /**
> + * Static utility method to convert a Python timestamp to a Java {@link Date}. For example,
> + * the timestamp returned from the API is a String in the form "1368738007.90714".
> + * This method handles the necessary conversion.
> + *
> + * @param inTimestamp the timestamp to convert.
> + *
> + * @return the converted {@link Date}
> + *
> + * @see Python's <a href="http://docs.python.org/2/library/time.html"><code>time.time()</code></a> method
> + */
> + public static Date convertTimestamp(String inTimestamp) {
I think "jclouds style" for all these three methods would be to create helper or utility Functions instead, and put them in a separate class.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383569
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> */
> + @Named("account:get")
> + @Consumes
The default value of "@Consumes" is WILDCARD. Changing to MediaType.APPLICATION_JSON.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251269
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -115,6 +138,26 @@ public String toString() {
> }
>
> protected ToStringHelper string() {
> - return toStringHelper("").add("containerCount", getContainerCount()).add("bytesUsed", getBytesUsed());
> + return super.string()
> + .add("containerCount", getContainerCount())
> + .add("objectCount", getObjectCount())
> + .add("bytesUsed", getBytesUsed());
> + }
> +
> + @Override
> + public boolean equals(Object object) {
> + if (this == object) {
> + return true;
> + }
> + if (object instanceof Account) {
> + final Account other = Account.class.cast(object);
I borrowed this code from another provider. I will make necessary changes here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251288
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + * For an integer value n, limits the number of results to at
> + * most n values.
> + */
> + String LIMIT = "limit";
> +
> + /**
> + * Given a string value x, return object names greater in value than
> + * the specified marker.
> + */
> + String MARKER = "marker";
> +
> + /**
> + * Given a string value x, return object names lower in value than the
> + * specified end marker.
> + */
> + String END_MARKER = "end_marker";
Could you explain how the value of this constant relates to the doc comment? Sorry, confused here...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383685
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> +
> + return (inLowercase.startsWith(ACCOUNT_METADATA_PREFIX))
> + ? inLowercase : ACCOUNT_METADATA_PREFIX + inLowercase;
> + }
> +
> + @Override
> + public String toString() {
> + return "prefix: " + ACCOUNT_METADATA_PREFIX;
> + }
> + };
> + }
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof Map<?,?>, "this binder is only valid for Maps!");
See comment above
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5397935
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Adrian Cole <no...@github.com>.
deprecate is best.
On Thu, Sep 26, 2013 at 6:48 PM, Andrew Phillips
<no...@github.com>wrote:
> IMHO we should stop writing expect tests and switch 100% to MWS tests,
> especially for blobstores.
>
> Good to know! One to remember for the next round of tests...
>
> Would it make sense somehow hooking MWS in "underneath" expect tests, or
> do we simply deprecate that and go with MWS from here..?
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-25217324>
> .
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-25218744
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Gaul <no...@github.com>.
> + * @see {@link Account}
> + * @see {@link Metadata}
> + */
> +@Singleton
> +public class BindAccountMetadataToDeleteToHeaders implements Binder {
> + private final Function<String, String> FN;
> + private final Function<String, String> FN2;
> +
> + public BindAccountMetadataToDeleteToHeaders() {
> + FN = new Function<String, String>() {
> +
> + @Override
> + public String apply(String element) {
> + String inLowercase = element.toLowerCase();
> +
> + return (!inLowercase.startsWith(ACCOUNT_METADATA_PREFIX))
How can this test succeed since ACCOUNT_METADATA_PREFIX has mixed-case characters?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5325748
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + * 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.openstack.swift.v1.reference;
> +
> +/**
> + * This interface represents Swift HTTP header values.
> + *
> + * @author Jeremy Daggett
> + */
> +public interface SwiftHeaders {
> +
> + /** Storage URL */
> + public static final String STORAGE_URL = "X-Storage-Url";
Don't need `public` in an interface?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383793
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + * beginning with the substring x.
> + */
> + String PREFIX = "prefix";
> +
> + /**
> + * For a character c, return all the object names nested in the container
> + * (without the need for the directory marker objects).
> + */
> + String DELIMITER = "delimiter";
> +
> + /**
> + * For a string value x, return the object names nested in the pseudo path.
> + * Equivalent to setting delimiter to '/' and prefix to the path with a '/'
> + * on the end.
> + */
> + String PATH = "path";
Same as for the above constants...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383688
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Gaul <no...@github.com>.
> + && Objects.equal(this.metadata, that.metadata);
> + }
> +
> + @Override
> + public String toString() {
> + return string().toString();
> + }
> +
> + protected ToStringHelper string() {
> + return Objects.toStringHelper(this)
> + .add("transactionId", transactionId).add("timestamp", timestamp).add("metadata", metadata);
> + }
> +
> + @Override
> + public int compareTo(SwiftResource that) {
> + if (that == null)
Better to throw NullPointerException?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5325846
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> +
> +import com.google.common.base.Function;
> +import com.google.common.collect.Maps;
> +import com.google.common.collect.Multimaps;
> +
> +/**
> + * Binds the Account Metadata to delete to the HTTP request headers.
> + *
> + * @see {@link Binder}
> + * @see {@link Account}
> + * @see {@link Metadata}
> + */
> +@Singleton
> +public class BindAccountMetadataToDeleteToHeaders implements Binder {
> + private final Function<String, String> FN;
> + private final Function<String, String> FN2;
Duly noted. Names will be changed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251266
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Everett Toews <no...@github.com>.
Reopened #17.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Zack Shoylev <no...@github.com>.
>
> - HttpResponse listResponse = HttpResponse.builder().statusCode(204)
> - .addHeader("X-Account-Container-Count", "3")
> - .addHeader("X-Account-Bytes-Used", "323479").build();
> + try {
> + AccountApi accountApi = api.getAccountApiForRegion("DFW");
> +
> + assertEquals(accountApi.get(), mockAccount());
> +
> + assertEquals(server.takeRequest().getRequestLine(), "POST /tokens HTTP/1.1");
> + assertEquals(server.takeRequest().getRequestLine(), "HEAD /v1/MossoCloudFS_5bcf396e-39dd-45ff-93a1-712b9aba90a9/ HTTP/1.1");
> +
> + } finally {
Shouldn't this apply to more code above? (the try block)
Too bad we can't use java 7 constructs.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5431998
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> + * @see {@link Account}
> + * @see {@link Metadata}
> + */
> +@Singleton
> +public class BindAccountMetadataToDeleteToHeaders implements Binder {
> + private final Function<String, String> FN;
> + private final Function<String, String> FN2;
> +
> + public BindAccountMetadataToDeleteToHeaders() {
> + FN = new Function<String, String>() {
> +
> + @Override
> + public String apply(String element) {
> + String inLowercase = element.toLowerCase();
> +
> + return (!inLowercase.startsWith(ACCOUNT_METADATA_PREFIX))
Will convert to lowercase before hand.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251275
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> +import org.jclouds.rest.Binder;
> +import org.jclouds.util.Maps2;
> +
> +import com.google.common.base.Function;
> +import com.google.common.collect.Multimaps;
> +
> +/**
> + * Binds the Account Metadata to the HTTP request headers.
> + *
> + * @see {@link Binder}
> + * @see {@link Account}
> + * @see {@link Metadata}
> + */
> +@Singleton
> +public class BindAccountMetadataToHeaders implements Binder {
> + private final Function<String, String> FN;
Changing name accordingly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251436
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Everett Toews <no...@github.com>.
Closing and reopening to trigger build. CloudBees isn't looking happy right now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-21190984
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> + return metadata;
> + }
> +
> + /**
> + * Static utility method to convert a Python timestamp to a Java {@link Date}. For example,
> + * the timestamp returned from the API is a String in the form "1368738007.90714".
> + * This method handles the necessary conversion.
> + *
> + * @param inTimestamp the timestamp to convert.
> + *
> + * @return the converted {@link Date}
> + *
> + * @see Python's <code>time.time()</code>
> + */
> + public static Date convertTimestamp(String inTimestamp) {
> + checkNotNull(inTimestamp);
Changing signature.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251501
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + * associated with them.
> + */
> +public class SwiftResource implements Comparable<SwiftResource> {
> +
> + public static Builder<?> builder() {
> + return new ConcreteBuilder();
> + }
> +
> + public Builder<?> toBuilder() {
> + return new ConcreteBuilder().fromSwiftResource(this);
> + }
> +
> + public abstract static class Builder<T extends Builder<T>> {
> + protected abstract T self();
> +
> + protected String transactionId;
If the transaction ID must be supplied to avoid an NPE later, either add a non-null default or make it a constructor parameter of the Builder?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383513
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> +public interface SwiftConstants {
> +
> + /**
> + * The maximum number of objects that can be returned per request.
> + */
> + int MAX_OBJECTS = 10000;
> +
> + /**
> + * The maximum length of a container name is 256 (bytes).
> + */
> + int MAX_LENGTH_CONTAINER_NAME = 256;
> +
> + /**
> + * The maximum length of an object name is 1024 (bytes).
> + */
> + int MAX_LENGTH_OBJECT_NAME = 1024;
"LENGTH_X_NAME" sounds like number of characters to me. If this is a _memory size_ limit, perhaps call it `MAX_OBJECT_NAME_SIZE_BYTES`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383747
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import com.google.common.base.Function;
> +import com.google.common.collect.Maps;
> +import com.google.common.collect.Multimaps;
> +
> +/**
> + * Binds the Account Metadata to delete to the HTTP request headers.
> + *
> + * @see {@link Binder}
> + * @see {@link Account}
> + * @see {@link Metadata}
> + */
> +@Singleton
> +public class BindAccountMetadataToDeleteToHeaders implements Binder {
> + private final Function<String, String> FN;
> + private final Function<String, String> FN2;
Could you pick variable names that explain a bit what they are?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5279310
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> + }
> +
> + @Override
> + public String toString() {
> + return "prefix: " + ACCOUNT_METADATA_PREFIX;
> + }
> + };
> + }
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof Map<?,?>, "this binder is only valid for Maps!");
> + checkNotNull(request, "request");
> +
> + Map<String, String> userMetadata = Maps2.transformKeys((Map<String, String>) input, FN);
What specifically do you want renamed here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6252158
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> */
> @Test(groups = "unit", testName = "AccountApiExpectTest")
> public class AccountApiExpectTest extends BaseSwiftApiExpectTest {
>
> - public void testGetAccountMetadataWhenResponseIs2xx() throws Exception {
> + /**
> + * Tests the {@link AccountApi#get()} method.
> + *
> + * @throws Exception if an exception occurs.
> + */
> + public void testGetAccount() throws Exception {
> +
> + MockWebServer server = new MockWebServer();
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251946
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Gaul <no...@github.com>.
> + * @see {@link Binder}
> + * @see {@link Account}
> + */
> +@Singleton
> +public class BindAccountMetadataToHeaders implements Binder {
> +
> + private final Function<String, String> transformMetadataKeysFN;
> +
> + public BindAccountMetadataToHeaders() {
> +
> + transformMetadataKeysFN = new Function<String, String>() {
> +
> + @Override
> + public String apply(String element) {
> + String inLowercase = element.toLowerCase();
> + String prefixLowercase = ACCOUNT_METADATA_PREFIX.toLowerCase();
Eagerly calling this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6380582
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Adrian Cole <no...@github.com>.
> + * associated with them.
> + */
> +public class SwiftResource implements Comparable<SwiftResource> {
> +
> + public static Builder<?> builder() {
> + return new ConcreteBuilder();
> + }
> +
> + public Builder<?> toBuilder() {
> + return new ConcreteBuilder().fromSwiftResource(this);
> + }
> +
> + public abstract static class Builder<T extends Builder<T>> {
> + protected abstract T self();
> +
> + protected String transactionId;
things like transactionId and timestamp are clutter, and we don't include them in any other jclouds api. replacement pull request will remove the hierarchical complexity this base class added.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6632441
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + return "";
> + }
> + };
> + }
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(input instanceof Map<?,?>, "this binder is only valid for Maps!");
> + checkNotNull(request, "request");
> +
> + // Transform the map to filter only the prefixed values
> + Map<String, String> userMetadata = Maps2.transformKeys((Map<String, String>) input, transformHeaders);
> +
> + // Transform the values to empty
> + Map<String, String> deletedMetadata = Maps.transformValues(userMetadata, transformValues);
Do we really think this is easier to read and understand than:
```
// Transform the values to empty
for (String key : userMetadata.keySet()) {
userMetadata.put(key, "");
}
```
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383315
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + /**
> + * The maximum length of HTTP requests is 5 (gigabytes).
> + */
> + int MAX_LENGTH_HTTP_REQUEST = 5;
> +
> + /**
> + * For an integer value n, limits the number of results to at
> + * most n values.
> + */
> + String LIMIT = "limit";
> +
> + /**
> + * Given a string value x, return object names greater in value than
> + * the specified marker.
> + */
> + String MARKER = "marker";
Could you explain how the value of this constant relates to the doc comment? Sorry, confused here...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383683
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + * @param metadata the Account {@link Metadata} to create or update.
> + *
> + * @see <a href="http://docs.openstack.org/api/openstack-object-storage/1.0/content/create-update-account-metadata.html">
> + * Create or Update Account Metadata API</a>
> + *
> + * @return <code>true</code> if the Account Metadata was successfully created or updated, false if not.
> + */
> + @Named("account:createOrUpdateMetadata")
> + @Consumes
> + @POST
> + @Fallback(FalseOnNotFoundOr404.class)
> + @Path("/")
> + boolean createOrUpdateMetadata(@BinderParam(BindAccountMetadataToHeaders.class) Map<String, String> metadata);
> +
> + /**
> + * Deletes Account metadata.
Use `{@link Account}`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383619
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> @Path("/")
> Account get();
>
> + /**
> + * Gets the Account metadata.
Use `{@link Account}`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383599
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + public static final String STORAGE_URL = "X-Storage-Url";
> +
> + /** Transaction ID*/
> + public static final String TRANSACTION_ID = "X-Trans-Id";
> +
> + /** Timestamp */
> + public static final String TIMESTAMP = "X-Timestamp";
> +
> + /**
> + * You can specify <code>X-Newest: true</code> on GETs and HEADs to indicate you want
> + * Swift to query all back-end copies and return the newest version retrieved.
> + */
> + public static final String NEWEST = "X-Newest";
> +
> + /** Account Metadata Prefix */
> + public static final String ACCOUNT_METADATA_PREFIX = "X-Account-Meta-";
Curious why we're doing lowercase comparison in the functions then: would it make sense to make this lowercase too?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383783
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + @Override
> + public String toString() {
> + return "prefix: " + ACCOUNT_METADATA_PREFIX;
> + }
> + };
> + }
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof Map<?,?>, "this binder is only valid for Maps!");
> + checkNotNull(request, "request");
> +
> + Map<String, String> userMetadata = Maps2.transformKeys((Map<String, String>) input, FN);
See comment above
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5397939
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Everett Toews <no...@github.com>.
Closed #17.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Adrian Cole <no...@github.com>.
agree that `s/assertTrue(myMap.size() >= 0)/assertFalse(myMap.isEmpty())/g`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-25107195
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> public int hashCode() {
> - return Objects.hashCode(getContainerCount(), getBytesUsed());
> + return Objects.hashCode(getContainerCount(), getObjectCount(), getBytesUsed(), getMetadata());
`timestamp` not relevant for hashCode/equals?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383405
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + header.getValue());
> + }
> +
> + return metadata;
> + }
> +
> + /**
> + * Static utility method to convert a Python timestamp to a Java {@link Date}. For example,
> + * the timestamp returned from the API is a String in the form "1368738007.90714".
> + * This method handles the necessary conversion.
> + *
> + * @param inTimestamp the timestamp to convert.
> + *
> + * @return the converted {@link Date}
> + *
> + * @see Python's <code>time.time()</code>
Add an href?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5398069
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + * This class represents a generic model for Metadata.
> + *
> + * Requests made to the various metadata APIs, will utilize this generic metadata.
> + *
> + * <p/>Prefixed headers, such as {@link SwiftHeaders#ACCOUNT_META_PREFIX},
> + * {@link SwiftHeaders#CONTAINER_META_PREFIX}, and {@link SwiftHeaders#ACCOUNT_META_PREFIX}
> + * will stripped via one of the parsing classes in {@link org.jclouds.openstack.swift.functions}
> + * when metadata is requested from the API.
> + *
> + * <p/>The Metadata related API calls, <code>createOrUpdateMetadata(Metadata)</code> and
> + * <code>deleteMetadata(Metadata)</code> will bind the prefixes to the values using one of the
> + * binders in {@link org.jclouds.openstack.swift.v1.binders}.
> + *
> + * @see {@link AccountApi}
> + */
> +public class Metadata extends ForwardingMap<String, String> {
Is this class extended to be overridden? If not, why a ForwardingMap here that does not do anything except call the underlying map? Why not just use a `Map<String, String>`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5398054
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #212](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/212/) SUCCESS
This pull request looks good
[(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-labs-openstack/pull/17#issuecomment-21191446
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> +
> + /**
> + * The maximum length of an object name is 1024 (bytes).
> + */
> + int MAX_LENGTH_OBJECT_NAME = 1024;
> +
> + /**
> + * The maximum length of HTTP requests is 5 (gigabytes).
> + */
> + int MAX_LENGTH_HTTP_REQUEST = 5;
> +
> + /**
> + * For an integer value n, limits the number of results to at
> + * most n values.
> + */
> + String LIMIT = "limit";
Could you explain how the value of this constant relates to the doc comment? Sorry, confused here...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383678
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> IMHO we should stop writing expect tests and switch 100% to MWS tests, especially for blobstores.
Good to know! One to remember for the next round of tests...
Would it make sense somehow hooking MWS in "underneath" expect tests, or do we simply deprecate that and go with MWS from here..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-25217324
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Gaul <no...@github.com>.
> + int MAX_OBJECTS = 10000;
> +
> + /**
> + * The maximum length of a container name is 256 (bytes).
> + */
> + int MAX_LENGTH_CONTAINER_NAME = 256;
> +
> + /**
> + * The maximum length of an object name is 1024 (bytes).
> + */
> + int MAX_LENGTH_OBJECT_NAME = 1024;
> +
> + /**
> + * The maximum length of HTTP requests is 5 (gigabytes).
> + */
> + int MAX_LENGTH_HTTP_REQUEST = 5;
Should this be 5L * 1024 * 1024 * 1024?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6380909
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + return metadata;
> + }
> +
> + /**
> + * Static utility method to convert a Python timestamp to a Java {@link Date}. For example,
> + * the timestamp returned from the API is a String in the form "1368738007.90714".
> + * This method handles the necessary conversion.
> + *
> + * @param inTimestamp the timestamp to convert.
> + *
> + * @return the converted {@link Date}
> + *
> + * @see Python's <code>time.time()</code>
> + */
> + public static Date convertTimestamp(String inTimestamp) {
> + checkNotNull(inTimestamp);
`checkNotNull(inTimestamp, "inTimestamp");`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5398066
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> +
> + return (inLowercase.startsWith(ACCOUNT_METADATA_PREFIX))
> + ? inLowercase : ACCOUNT_METADATA_PREFIX + inLowercase;
> + }
> +
> + @Override
> + public String toString() {
> + return "prefix: " + ACCOUNT_METADATA_PREFIX;
> + }
> + };
> + }
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof Map<?,?>, "this binder is only valid for Maps!");
I think this was actually meant to refer to the following comment:
> Remove one of the checks, since null instanceof X == false:
>
> checkArgument(input instanceof Map<?,?>, "input must be a non-null java.util.Map");
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6253996
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + }
> + };
> +
> + FN2 = new Function<String, String>() {
> +
> + @Override
> + public String apply(String element) {
> + return "";
> + }
> + };
> + }
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof Map<?,?>, "this binder is only valid for Maps!");
Remove one of the checks, since `null instanceof X == false`:
```
checkArgument(input instanceof Map<?,?>, "input must be a non-null java.util.Map");
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5397891
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Everett Toews <no...@github.com>.
@jdaggett Where are we at with this PR?
@andrewgaul Do you need to give this a final review?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-24511656
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> +
> +import com.google.common.base.Function;
> +import com.google.common.collect.Maps;
> +import com.google.common.collect.Multimaps;
> +
> +/**
> + * Binds the Account Metadata to delete to the HTTP request headers.
> + *
> + * @see {@link Binder}
> + * @see {@link Account}
> + * @see {@link Metadata}
> + */
> +@Singleton
> +public class BindAccountMetadataToDeleteToHeaders implements Binder {
> +
> + private final Function<String, String> transformHeaders;
Since we know what the transform will be doing, how about "prefixAndToLowerCase", or something that is less opaque than the current name?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383237
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Gaul <no...@github.com>.
Mention JCLOUDS-73 in the commit message to tag the issue. Please address nits as well.
@etoews I do not see any blocking issues for this commit, however, I do not understand the overall strategy and benefits of reimplementation. @jdaggett Could you add a description of the planned work to the JIRA issue as well as some pointers to how I would test this against a live Cloud Files instance (I have credentials)? Excited to move forward with Swift improvements!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-24523056
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> +
> + protected ToStringHelper string() {
> + return Objects.toStringHelper(this)
> + .add("transactionId", transactionId).add("timestamp", timestamp).add("metadata", metadata);
> + }
> +
> + @Override
> + public int compareTo(SwiftResource that) {
> + if (that == null)
> + return 1;
> + if (this == that)
> + return 0;
> + return this.getTransactionId().compareTo(that.getTransactionId());
> + }
> +
> +}
Grrr formatter...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251994
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + * <code>time.time()</code> format (in seconds). It will be converted to a proper
> + * Java {@link Date}
> + *
> + * @param from the {@link HttpResponse} containing the headers
> + *
> + * @return the {@link Account} object
> + */
> + public Account apply(HttpResponse from) {
> +
> + Multimap<String, String> metaMap =
> + filterPrefixedHeaders(from.getHeaders(), ACCOUNT_METADATA_PREFIX);
> +
> + Map<String, String> m = extractMetadata(metaMap, ACCOUNT_METADATA_PREFIX);
> +
> + return Account.builder()
> + .transactionId(from.getFirstHeaderOrNull(TRANSACTION_ID))
I thought `null` was not a valid value for `transactionId`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383647
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> /**
> - * @author Adrian Cole
> + * This class is responsible for testing the Account API methods.
> + *
> + * @see {@link AccountApi}
> + *
> + * @author Jeremy Daggett
> */
> @Test(groups = "unit", testName = "AccountApiExpectTest")
> public class AccountApiExpectTest extends BaseSwiftApiExpectTest {
Any reason for not using the "standard" jclouds `requestsSendResponses
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383866
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Adrian Cole <no...@github.com>.
I'm going to polish this commit in another pull request and take care of the issues noted. thx @jdaggett for the start!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-25249965
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
In general, would be curious to know why we're using MockWebServer over `requestsSendResponses` here when we're using the latter in pretty much all other new expect tests at present.
For all the `assertTrue` assertions, please add a debugging-friendly message such as
```
assertTrue(foobar.size() > 2, "Expected more than 2 items but got: " + foobar)
```
Also, not sure what the purpose of the various `assertTrue(myMap.size() >= 0)` checks is - how can maps have a negative size?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-24531140
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Zack Shoylev <no...@github.com>.
> */
> + @Named("account:get")
> + @Consumes
I think @Consumes(MediaType.APPLICATION_JSON) is needed on most methods or the call might get rejected (during live testing)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5431854
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Gaul <no...@github.com>.
I found the JIRA issue https://issues.apache.org/jira/browse/JCLOUDS-73. Can you add JCLOUDS-73 to the commit message so that we tag this in the issue?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-21363775
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> +
> + return (inLowercase.startsWith(ACCOUNT_METADATA_PREFIX))
> + ? inLowercase : ACCOUNT_METADATA_PREFIX + inLowercase;
> + }
> +
> + @Override
> + public String toString() {
> + return "prefix: " + ACCOUNT_METADATA_PREFIX;
> + }
> + };
> + }
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof Map<?,?>, "this binder is only valid for Maps!");
What specifically do you want renamed here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6252155
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> + header.getValue());
> + }
> +
> + return metadata;
> + }
> +
> + /**
> + * Static utility method to convert a Python timestamp to a Java {@link Date}. For example,
> + * the timestamp returned from the API is a String in the form "1368738007.90714".
> + * This method handles the necessary conversion.
> + *
> + * @param inTimestamp the timestamp to convert.
> + *
> + * @return the converted {@link Date}
> + *
> + * @see Python's <code>time.time()</code>
Adding an href
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251509
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
>
> - HttpResponse listResponse = HttpResponse.builder().statusCode(204)
> - .addHeader("X-Account-Container-Count", "3")
> - .addHeader("X-Account-Bytes-Used", "323479").build();
> + try {
> + AccountApi accountApi = api.getAccountApiForRegion("DFW");
> +
> + assertEquals(accountApi.get(), mockAccount());
> +
> + assertEquals(server.takeRequest().getRequestLine(), "POST /tokens HTTP/1.1");
> + assertEquals(server.takeRequest().getRequestLine(), "HEAD /v1/MossoCloudFS_5bcf396e-39dd-45ff-93a1-712b9aba90a9/ HTTP/1.1");
> +
> + } finally {
Moving the try up in the methods. As I learn more about MockWebServer this code may be refactored
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251940
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Everett Toews <no...@github.com>.
@jdaggett Will you have a chance to address these comments sometime soon?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-23103570
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> assertTrue(account.getBytesUsed() >= 0);
> }
> }
>
> + /**
> + * Tests the {@link AccountApi#getMetadata()} method.
> + *
> + * @throws Exception if there is an exception.
> + */
> + public void testGetAccountMetadata() throws Exception {
> + for (String regionId : api.getConfiguredRegions()) {
> + AccountApi accountApi = api.getAccountApiForRegion(regionId);
> + Map<String, String> metadata = accountApi.getMetadata();
> +
> + assertNotNull(metadata);
> + assertTrue(metadata.size() >= 0);
How could a map have negative size?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6384004
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> +import org.jclouds.rest.Binder;
> +import org.jclouds.util.Maps2;
> +
> +import com.google.common.base.Function;
> +import com.google.common.collect.Multimaps;
> +
> +/**
> + * Binds the Account Metadata to the HTTP request headers.
> + *
> + * @see {@link Binder}
> + * @see {@link Account}
> + */
> +@Singleton
> +public class BindAccountMetadataToHeaders implements Binder {
> +
> + private final Function<String, String> transformMetadataKeysFN;
Again, please rename to specify what this function actually _does_
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383344
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + * associated with them.
> + */
> +public class SwiftResource implements Comparable<SwiftResource> {
> +
> + public static Builder<?> builder() {
> + return new ConcreteBuilder();
> + }
> +
> + public Builder<?> toBuilder() {
> + return new ConcreteBuilder().fromSwiftResource(this);
> + }
> +
> + public abstract static class Builder<T extends Builder<T>> {
> + protected abstract T self();
> +
> + protected String transactionId;
Even better ;-) Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6635087
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Zack Shoylev <no...@github.com>.
> */
> @Test(groups = "unit", testName = "AccountApiExpectTest")
> public class AccountApiExpectTest extends BaseSwiftApiExpectTest {
>
> - public void testGetAccountMetadataWhenResponseIs2xx() throws Exception {
> + /**
> + * Tests the {@link AccountApi#get()} method.
> + *
> + * @throws Exception if an exception occurs.
> + */
> + public void testGetAccount() throws Exception {
> +
> + MockWebServer server = new MockWebServer();
Nice
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5431942
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #20](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/20/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-21191548
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + * Gets the Account metadata.
> + *
> + * @return the {@link Metadata} without any prefixes.
> + *
> + * @see <a href="http://docs.openstack.org/api/openstack-object-storage/1.0/content/retrieve-account-metadata.html">
> + * Get Account Metadata API</a>
> + */
> + @Named("account:getMetadata")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @HEAD
> + @ResponseParser(ParseAccountMetadataFromHeaders.class)
> + @Path("/")
> + Map<String, String> getMetadata();
> +
> + /**
> + * Creates or updates the Account metadata.
Use `{@link Account}`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383605
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> +/**
> + * Constants used in Swift implementations.
> + *
> + * @author Jeremy Daggett
> + */
> +public interface SwiftConstants {
> +
> + /**
> + * The maximum number of objects that can be returned per request.
> + */
> + int MAX_OBJECTS = 10000;
> +
> + /**
> + * The maximum length of a container name is 256 (bytes).
> + */
> + int MAX_LENGTH_CONTAINER_NAME = 256;
"length of name" sounds like number of characters to me. If this is a _memory size_ limit, perhaps call it `MAX_CONTAINER_NAME_SIZE_BYTES`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383726
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> + }
> +
> + @Override
> + public boolean equals(Object object) {
> + if (this == object) {
> + return true;
> + }
> + if (object instanceof Account) {
> + final Account other = Account.class.cast(object);
> + return equal(getContainerCount(), other.getContainerCount())
> + && equal(getObjectCount(), other.getObjectCount())
> + && equal(getBytesUsed(), other.getBytesUsed())
> + && equal(getMetadata(), other.getMetadata());
> + } else {
> + return false;
> + }
Noted.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251495
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + @Override
> + public boolean equals(Object object) {
> + if (this == object) {
> + return true;
> + }
> + if (object instanceof Account) {
> + final Account other = Account.class.cast(object);
> + return equal(getContainerCount(), other.getContainerCount())
> + && equal(getObjectCount(), other.getObjectCount())
> + && equal(getBytesUsed(), other.getBytesUsed())
> + && equal(getMetadata(), other.getMetadata());
> + } else {
> + return false;
> + }
Rewrite this to use the "equals pattern" in e.g. [RegionAndName](https://github.com/jclouds/jclouds/blob/master/apis/ec2/src/main/java/org/jclouds/ec2/compute/domain/RegionAndName.java#L49)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5397996
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + FN2 = new Function<String, String>() {
> +
> + @Override
> + public String apply(String element) {
> + return "";
> + }
> + };
> + }
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof Map<?,?>, "this binder is only valid for Maps!");
> + checkNotNull(request, "request");
> +
> + Map<String, String> userMetadata = Maps2.transformKeys((Map<String, String>) input, FN);
Why the need for the cast? `Maps2.<String, String, String>transformKeys(input, FN)`? But is Java not able to figure that out itself?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5397918
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
@everett-toews Yes! @andrewgaul and I met this morning to discuss BlobStore futures and Swift, so I will get some comments in here and push the necessary changes. Thx.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-23121246
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + */
> +@Singleton
> +public class BindAccountMetadataToDeleteToHeaders implements Binder {
> +
> + private final Function<String, String> transformHeaders;
> + private final Function<String, String> transformValues;
> +
> + public BindAccountMetadataToDeleteToHeaders() {
> +
> + transformHeaders = new Function<String, String>() {
> +
> + @Override
> + public String apply(String element) {
> + String inLowercase = element.toLowerCase();
> + return (!inLowercase.startsWith(ACCOUNT_METADATA_PREFIX))
> + ? ACCOUNT_METADATA_PREFIX.toLowerCase() + inLowercase : inLowercase;
If `ACCOUNT_METADATA_PREFIX` isn't in lowercase already, declare a local constant or variable which represents it and use that here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383221
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Adrian Cole <no...@github.com>.
IMHO we should stop writing expect tests and switch 100% to MWS tests, especially for blobstores. Expect tests have gaps in that they do not test what is sent over the wire. This burnt us several times in S3 (expect headers, etc).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-25107145
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Ignasi Barrera <no...@github.com>.
> */
> + @Named("account:get")
> + @Consumes
I'm not familiar with the Swift API, but do the methods send an Accept header with `*/*` or should we better send "application/json" or a concrete media type (to make sure the right parser is used to parse the response body)?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5279584
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> + /** Container Sync To */
> + public static final String CONTAINER_SYNC_TO = "X-Container-Sync-To";
> +
> + /** Swift CORS Container Headers */
> + public static final String ACCESS_CONTROL_ALLOW_HEADERS = "X-Container-Meta-Access-Control-Allow-Headers";
> + public static final String ACCESS_CONTROL_ALLOW_ORIGIN = "X-Container-Meta-Access-Control-Allow-Origin";
> + public static final String ACCESS_CONTROL_EXPOSE_HEADERS = "X-Container-Meta-Access-Control-Expose-Headers";
> + public static final String ACCESS_CONTROL_MAX_AGE = "X-Container-Meta-Access-Control-Max-Age";
> +
> + /**
> + * This mode is normally only active for anonymous requests.
> + * To use it with authenticated requests, set the header <code>X-Web-Mode: true</code> on the request.
> + */
> + public static final String WEB_MODE = "X-Web-Mode";
> +
> + /** Static Web Headers */
Should be a `/*...*/` and not a Javadoc comment
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383813
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Gaul <no...@github.com>.
> @@ -115,6 +137,22 @@ public String toString() {
> }
>
> protected ToStringHelper string() {
> - return toStringHelper("").add("containerCount", getContainerCount()).add("bytesUsed", getBytesUsed());
> + return super.string()
> + .add("containerCount", getContainerCount())
> + .add("objectCount", getObjectCount())
> + .add("bytesUsed", getBytesUsed());
> + }
> +
> + @Override
> + public boolean equals(Object object) {
> + if (this == object) return true;
> + if (object == null || getClass() != object.getClass()) return false;
Call if (!(object instanceof Account)).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6380771
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> +import com.google.common.base.Function;
> +import com.google.common.collect.Maps;
> +import com.google.common.collect.Multimaps;
> +
> +/**
> + * Binds the Account Metadata to delete to the HTTP request headers.
> + *
> + * @see {@link Binder}
> + * @see {@link Account}
> + * @see {@link Metadata}
> + */
> +@Singleton
> +public class BindAccountMetadataToDeleteToHeaders implements Binder {
> +
> + private final Function<String, String> transformHeaders;
> + private final Function<String, String> transformValues;
Same here. We know that this is actually "setValuesToEmpty" or something like that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6383253
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Zack Shoylev <no...@github.com>.
> @@ -46,7 +48,7 @@ protected Properties setupProperties() {
> contextProperties.setProperty("jclouds.keystone.credential-type", "passwordCredentials");
> return contextProperties;
> }
> -
> +
whitespace nit
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5431907
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Phillips <no...@github.com>.
> protected long bytesUsed;
> -
> +
Unneeded?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r5397945
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Gaul <no...@github.com>.
@jadggett Looks OK but can you expand the commit message to explain the context of what this commit does? Also does it reference any JIRA issues?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-21363644
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Andrew Gaul <no...@github.com>.
> @@ -115,6 +137,22 @@ public String toString() {
> }
>
> protected ToStringHelper string() {
> - return toStringHelper("").add("containerCount", getContainerCount()).add("bytesUsed", getBytesUsed());
> + return super.string()
> + .add("containerCount", getContainerCount())
> + .add("objectCount", getObjectCount())
> + .add("bytesUsed", getBytesUsed());
> + }
> +
> + @Override
> + public boolean equals(Object object) {
> + if (this == object) return true;
> + if (object == null || getClass() != object.getClass()) return false;
> + Account that = Account.class.cast(object);
Use operator cast instead: (Account) object.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6380702
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
Contributes to https://issues.apache.org/jira/browse/JCLOUDS-73
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17#issuecomment-21186769
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Jeremy Daggett <no...@github.com>.
> + }
> + };
> +
> + FN2 = new Function<String, String>() {
> +
> + @Override
> + public String apply(String element) {
> + return "";
> + }
> + };
> + }
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof Map<?,?>, "this binder is only valid for Maps!");
Will remove the checkNotNull(...) method.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6251407
Re: [jclouds-labs-openstack] New Swift Account API, along with
models, and expect/live tests (#17)
Posted by Adrian Cole <no...@github.com>.
> +
> + return metadata;
> + }
> +
> + /**
> + * Static utility method to convert a Python timestamp to a Java {@link Date}. For example,
> + * the timestamp returned from the API is a String in the form "1368738007.90714".
> + * This method handles the necessary conversion.
> + *
> + * @param inTimestamp the timestamp to convert.
> + *
> + * @return the converted {@link Date}
> + *
> + * @see Python's <a href="http://docs.python.org/2/library/time.html"><code>time.time()</code></a> method
> + */
> + public static Date convertTimestamp(String inTimestamp) {
if these are internal details, fine to be private static. otherwise, bump out like ^^ said
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/17/files#r6579567