Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[Ignasi Barrera]

Closed #319.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319#event-805473797

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[Ignasi Barrera]

Pushed to master as 
[4e397202](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/4e397202).
 Thanks @alibazlamit!

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319#issuecomment-250173385

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[alibazlamit]

Squashed. Thanks.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319#issuecomment-250150406

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[Ignasi Barrera]

Mind squashing the commits so I can cleanly merge it?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319#issuecomment-250109508

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[Ignasi Barrera]

nacx commented on this pull request.



> +import com.google.inject.TypeLiteral;
+import java.io.ByteArrayInputStream;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import java.util.zip.ZipInputStream;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import org.apache.jclouds.oneandone.rest.domain.VPNConfig;
+import org.jclouds.http.functions.ParseJson;
+import org.jclouds.json.Json;
+
+@Singleton
+public class VPNConfigParser extends ParseJson {
+
+   @Inject
+   public VPNConfigParser(Json json) {

Perfect. Thanks!

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[Ignasi Barrera]

nacx approved this pull request.





-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319#pullrequestreview-1891528

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[alibazlamit]

alibazlamit commented on this pull request.



> +import com.google.inject.TypeLiteral;
+import java.io.ByteArrayInputStream;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import java.util.zip.ZipInputStream;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import org.apache.jclouds.oneandone.rest.domain.VPNConfig;
+import org.jclouds.http.functions.ParseJson;
+import org.jclouds.json.Json;
+
+@Singleton
+public class VPNConfigParser extends ParseJson {
+
+   @Inject
+   public VPNConfigParser(Json json) {

I tried removing the parser class that create an error it could not parse the 
JSON correctly for a reason so i decided to leave it like this, i have removed 
the public modifier and pushed the change. Thanks

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[alibazlamit]

@alibazlamit pushed 1 commit.

426a288  remove public constructor


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319/files/4331e099037b24edea158bfa3d20228c9c39035d..426a288b57f1de9a554568766ebd966399ab0d39

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[Ignasi Barrera]

nacx commented on this pull request.



> +import com.google.inject.TypeLiteral;
+import java.io.ByteArrayInputStream;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import java.util.zip.ZipInputStream;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import org.apache.jclouds.oneandone.rest.domain.VPNConfig;
+import org.jclouds.http.functions.ParseJson;
+import org.jclouds.json.Json;
+
+@Singleton
+public class VPNConfigParser extends ParseJson {
+
+   @Inject
+   public VPNConfigParser(Json json) {

The visibility of this constructor still needs to be reduced by removing the 
public modifier (see 
[this](https://github.com/google/guice/wiki/KeepConstructorsHidden)). Or you 
can completely remove the parser class and bring back the `@SelectJson` 
annotation in the api method.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[alibazlamit]

@alibazlamit pushed 1 commit.

4331e09  Updated with changes


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319/files/debbc9b5bc3c2d072ee55d9f184d3c59ed778fb8..4331e099037b24edea158bfa3d20228c9c39035d

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[alibazlamit]

alibazlamit commented on this pull request.



> +   public static class ToZipStream implements Function ZipInputStream> {
+
+  @Inject
+  public ToZipStream() {
+ super();
+  }
+
+  @Override
+  public ZipInputStream apply(VPNConfig input) {
+ try {
+byte[] decoded = base64().decode(input.content());
+ZipInputStream zipStream = new ZipInputStream(new 
ByteArrayInputStream(decoded));
+return zipStream;
+ } catch (Exception ex) {
+
Logger.getLogger(VPNConfigParser.class.getName()).log(Level.SEVERE, null, ex);
+return null;

Removed the try catch, if the API fails to return the config the fallback with 
work as needed and return a null object, no need for a try catch in there, if 
any other error should be returned from the API the error handler will do its 
job.

Changes pushed

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[Ignasi Barrera]

nacx requested changes on this pull request.

Done reviewing. Thanks @alibazlamit!

> @@ -62,9 +65,10 @@
@Named("vpn:configurations:get")
@GET
@Path("/{vpnId}/configuration_file")
-   @SelectJson("config_zip_file")
+   @ResponseParser(VPNConfigParser.class)

Is this parser class needed? You could leave the `@SelectJson` annotation and 
just create the transformation function as a top level class.

> +import com.google.inject.TypeLiteral;
+import java.io.ByteArrayInputStream;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import java.util.zip.ZipInputStream;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import org.apache.jclouds.oneandone.rest.domain.VPNConfig;
+import org.jclouds.http.functions.ParseJson;
+import org.jclouds.json.Json;
+
+@Singleton
+public class VPNConfigParser extends ParseJson {
+
+   @Inject
+   public VPNConfigParser(Json json) {

Remove the `public`modifier if this parser class remains.

> +import org.jclouds.json.Json;
+
+@Singleton
+public class VPNConfigParser extends ParseJson {
+
+   @Inject
+   public VPNConfigParser(Json json) {
+  super(json, TypeLiteral.get(VPNConfig.class));
+   }
+
+   public static class ToZipStream implements Function {
+
+  @Inject
+  public ToZipStream() {
+ super();
+  }

Remove this constructor?

> +
+   public static class ToZipStream implements Function {
+
+  @Inject
+  public ToZipStream() {
+ super();
+  }
+
+  @Override
+  public ZipInputStream apply(VPNConfig input) {
+ try {
+byte[] decoded = base64().decode(input.content());
+ZipInputStream zipStream = new ZipInputStream(new 
ByteArrayInputStream(decoded));
+return zipStream;
+ } catch (Exception ex) {
+
Logger.getLogger(VPNConfigParser.class.getName()).log(Level.SEVERE, null, ex);

Don't use java logging. jclouds provides a logging abstraction that allows 
users to configure their preferred logging framework. In order to get the right 
log, just declare the following class variable of type 
`org.jclouds.logging.Logger`:

```java
@Resource
protected Logger logger = Logger.NULL;
```

This will configure a null logger by default, but use the one configured by 
users providing a logging module when creating the jclouds context.

> +   public static class ToZipStream implements Function ZipInputStream> {
+
+  @Inject
+  public ToZipStream() {
+ super();
+  }
+
+  @Override
+  public ZipInputStream apply(VPNConfig input) {
+ try {
+byte[] decoded = base64().decode(input.content());
+ZipInputStream zipStream = new ZipInputStream(new 
ByteArrayInputStream(decoded));
+return zipStream;
+ } catch (Exception ex) {
+
Logger.getLogger(VPNConfigParser.class.getName()).log(Level.SEVERE, null, ex);
+return null;

Instead of returning `null` on *any* failure, are there situations where it 
makes sense returning null? Can we return null just in those cases, and 
propagate the exception otherwise?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319#pullrequestreview-1608407

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[alibazlamit]

alibazlamit commented on this pull request.



> +   @GET
+   @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+   List getList(GenericQueryOptions options);
+
+   @Named("vpn:get")
+   @GET
+   @Path("/{vpnId}")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   Vpn get(@PathParam("vpnId") String vpnId);
+
+   @Named("vpn:configurations:get")
+   @GET
+   @Path("/{vpnId}/configuration_file")
+   @SelectJson("config_zip_file")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   String getConfiguration(@PathParam("vpnId") String vpnId);

@nacx Added the function could you please check the PR, Thanks.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[alibazlamit]

@alibazlamit pushed 1 commit.

debbc9b  Fixed eol


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319/files/cdc081ebd87932804176473980ef7faf59076b4a..debbc9b5bc3c2d072ee55d9f184d3c59ed778fb8

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[alibazlamit]

@alibazlamit pushed 1 commit.

cdc081e  Updated Changes.


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319/files/4f275a8ce0ed1e4738587b3ed6a9e410cf70a259..cdc081ebd87932804176473980ef7faf59076b4a

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[alibazlamit]

alibazlamit commented on this pull request.



> +   @GET
+   @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+   List getList(GenericQueryOptions options);
+
+   @Named("vpn:get")
+   @GET
+   @Path("/{vpnId}")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   Vpn get(@PathParam("vpnId") String vpnId);
+
+   @Named("vpn:configurations:get")
+   @GET
+   @Path("/{vpnId}/configuration_file")
+   @SelectJson("config_zip_file")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   String getConfiguration(@PathParam("vpnId") String vpnId);

OK ill add it


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[Ignasi Barrera]

nacx commented on this pull request.



> +   @GET
+   @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+   List getList(GenericQueryOptions options);
+
+   @Named("vpn:get")
+   @GET
+   @Path("/{vpnId}")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   Vpn get(@PathParam("vpnId") String vpnId);
+
+   @Named("vpn:configurations:get")
+   @GET
+   @Path("/{vpnId}/configuration_file")
+   @SelectJson("config_zip_file")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   String getConfiguration(@PathParam("vpnId") String vpnId);

Well, I know it is unnecessary, but if we already know we are returning a zip 
file, why not returning a proper stream?

I mean, we know it is not *a regular string*, the same way that we return 
objects instead of raw json. Responses are not simple strings and we try to 
return useful objects. Also many users that consume apis look first at the 
method signatures and in the worst case they read the docs :) Returning a 
ZipInputStream here would serve two purposes: better document what this method 
does, and provide a better interface for users.

Adding that should be trivial, as the function should only ned to decode the 
string and create a stream for it, not a big deal we'll struggle to maintain.


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[alibazlamit]

alibazlamit commented on this pull request.



> +   @GET
+   @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+   List getList(GenericQueryOptions options);
+
+   @Named("vpn:get")
+   @GET
+   @Path("/{vpnId}")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   Vpn get(@PathParam("vpnId") String vpnId);
+
+   @Named("vpn:configurations:get")
+   @GET
+   @Path("/{vpnId}/configuration_file")
+   @SelectJson("config_zip_file")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   String getConfiguration(@PathParam("vpnId") String vpnId);

@nacx I think its an unnecessary step,users will have to still convert the 
ZipInputStream to a file and choose a path,i would leave the whole to who ever 
wants to use that method,on the other hand if you think its must be there ill 
implement that.


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[alibazlamit]

@alibazlamit pushed 1 commit.

4f275a8  Review changes applied


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319/files/057a2df15be24f6d76029394e39493c08738639e..4f275a8ce0ed1e4738587b3ed6a9e410cf70a259

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[Ignasi Barrera]

nacx requested changes on this pull request.

Thanks @alibazlamit!

> +import org.jclouds.rest.annotations.Fallback;
+import org.jclouds.rest.annotations.MapBinder;
+import org.jclouds.rest.annotations.RequestFilters;
+import org.jclouds.rest.annotations.SelectJson;
+import org.jclouds.rest.binders.BindToJsonPayload;
+
+@Path("/vpns")
+@Produces("application/json")
+@Consumes("application/json")
+@RequestFilters(AuthenticateRequest.class)
+public interface VpnApi {
+
+   @Named("vpn:list")
+   @GET
+   @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+   List getList();

Rename to `list`.

> +  assertNotNull(resultWithQuery);
+  assertFalse(resultWithQuery.isEmpty());
+  Assert.assertTrue(resultWithQuery.size() > 0);
+   }
+
+   @Test
+   public void testGet() {
+  Vpn result = vpnApi().get(currentVpn.id());
+
+  assertNotNull(result);
+  assertEquals(result.id(), currentVpn.id());
+   }
+
+   @Test
+   public void testGetConfiguration() throws InterruptedException {
+  Thread.sleep(15000);

Is this really needed? Is there a more deterministic way of waiting?

> @@ -37,7 +37,7 @@
   
 
   
-2.0.0-SNAPSHOT
+2.0.0-SNAPSHOT  

[nit] Discard this change.

> +   @GET
+   @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+   List getList(GenericQueryOptions options);
+
+   @Named("vpn:get")
+   @GET
+   @Path("/{vpnId}")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   Vpn get(@PathParam("vpnId") String vpnId);
+
+   @Named("vpn:configurations:get")
+   @GET
+   @Path("/{vpnId}/configuration_file")
+   @SelectJson("config_zip_file")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   String getConfiguration(@PathParam("vpnId") String vpnId);

Interesting, so this method returns a base64 encoded zip file containing the 
VPN files. Would it make sense to return a `ZipInputStream`, so users can 
easily consume and store the returned files?

You could create a `Function` that takes that base64 
string and returns a proper ZipInputStream, and then annotate this method with 
`@Transform(YourFunction.class)`, so jclouds automatically transforms the 
response.

WDYT?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319#pullrequestreview-1293800

Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)

[alibazlamit]

@alibazlamit pushed 1 commit.

89ae677  removed checkstyle skip


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/319/files/8ca167c97b501d2fe28b6a3a34fbdc0df96dbf7f..89ae677061d0aae07945036e187487e3964f8d42