nacx requested changes on this pull request.
Thanks @gatj98!
This looks very good, and it is great that you figured out how to implement the
pagination (not easy at all) and that you implemented the mock tests
accordingly.
Good job!
> +
+import com.google.common.base.Supplier;
+import org.jclouds.http.HttpException;
+import org.jclouds.http.HttpRequest;
+import org.jclouds.http.HttpRequestFilter;
+import org.jclouds.location.Zone;
+
+import javax.inject.Inject;
+import java.util.Set;
+
+/**
+ * Adds set of Datacenter IDs as set in jclouds.zones JVM property.
+ */
+public class DatacenterIdListDatacentersFilter implements HttpRequestFilter {
+ @Inject
+ @Zone
Locations in jclouds are defined in a hierarchy such as: Provider (unique;
DimensionData) > Region > Zone.
Are we going to model the locations as zones, and a Datacenter will be one
zone? In that case, what is going to be a region? Otherwise change the
annotation to `@Region` and adopt that if it is more appropriate (just asking).
> +import org.jclouds.dimensiondata.cloudcontrol.DimensionDataCloudControlApi;
+import org.jclouds.dimensiondata.cloudcontrol.domain.Datacenter;
+import org.jclouds.dimensiondata.cloudcontrol.domain.Datacenters;
+import org.jclouds.dimensiondata.cloudcontrol.options.PaginationOptions;
+import org.jclouds.http.functions.ParseJson;
+import org.jclouds.json.Json;
+
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import java.util.List;
+
+@Singleton
+public class ParseDatacenters extends ParseJson<Datacenters> {
+
+ @Inject
+ public ParseDatacenters(Json json) {
Remove the `public` modifier from all injection constructors.
Also, move all these parser classes that support the pagination iterables as
inner classes of the API interface. It's the preferred approach now, to limit
their visibility.
> + public static class ToPagedIterable extends
> ArgsToPagedIterable<OperatingSystem, ToPagedIterable> {
+
+ private DimensionDataCloudControlApi api;
+
+ @Inject
+ public ToPagedIterable(DimensionDataCloudControlApi api) {
+ this.api = api;
+ }
+
+ @Override
+ protected Function<Object, IterableWithMarker<OperatingSystem>>
markerToNextForArgs(List<Object> args) {
+ return new Function<Object, IterableWithMarker<OperatingSystem>>()
{
+ @Override
+ public IterableWithMarker<OperatingSystem> apply(Object input)
{
+ PaginationOptions paginationOptions =
PaginationOptions.class.cast(input);
+ return
api.getInfrastructureApi().listOperatingSystems(getArgs(request).get(0).toString(),
paginationOptions);
No need for `getArgs(request)`; the `args` parameter in the method is already
that; just use it.
> + public static class ToPagedIterable extends
> ArgsToPagedIterable<OperatingSystem, ToPagedIterable> {
+
+ private DimensionDataCloudControlApi api;
+
+ @Inject
+ public ToPagedIterable(DimensionDataCloudControlApi api) {
+ this.api = api;
+ }
+
+ @Override
+ protected Function<Object, IterableWithMarker<OperatingSystem>>
markerToNextForArgs(List<Object> args) {
+ return new Function<Object, IterableWithMarker<OperatingSystem>>()
{
+ @Override
+ public IterableWithMarker<OperatingSystem> apply(Object input)
{
+ PaginationOptions paginationOptions =
PaginationOptions.class.cast(input);
+ return
api.getInfrastructureApi().listOperatingSystems(getArgs(request).get(0).toString(),
paginationOptions);
BTW, since in most cases, pagination methods take one argument (we've found it
common in many APIs) jclouds provides the convenience class
`Arg0ToPagedIterable`. It is the same but instead of getting the list of args,
you just get one optional arg parameter (the first one passed to the original
method). Perhaps you could consider extending that class if it is more
convenient, but not a big deal :)
> +
+import org.jclouds.dimensiondata.cloudcontrol.domain.Datacenter;
+import org.jclouds.dimensiondata.cloudcontrol.domain.OperatingSystem;
+import
org.jclouds.dimensiondata.cloudcontrol.internal.BaseDimensionDataCloudControlApiLiveTest;
+import org.testng.annotations.Test;
+
+import java.util.List;
+
+import static org.testng.Assert.assertNotNull;
+
+@Test(groups = "live", testName = "InfrastructureApiLiveTest", singleThreaded
= true)
+public class InfrastructureApiLiveTest extends
BaseDimensionDataCloudControlApiLiveTest {
+
+ @Test
+ public void testListDatacenters() {
+ List<Datacenter> datacenters =
api().listDatacenters().concat().toList();
Better not call `toList()`. That will copy all elements into a list, losing the
benefits of lazy loading each page. Not a big deal in the tests, but let's use
pagination properly (this also serves as example for users).
> + PaginatedCollection<Datacenter> datacenters =
> api.getInfrastructureApi().listDatacenters(paginationOptions);
+
+ assertEquals(250, datacenters.getPageSize());
+ assertEquals(2, datacenters.getTotalCount());
+ assertEquals(2, datacenters.getPageCount());
+ assertEquals(1, datacenters.getPageNumber());
+
+ Uris.UriBuilder uriBuilder = Uris
+
.uriBuilder("/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/infrastructure/datacenter");
+ uriBuilder.addQuery(paginationOptions.buildQueryParameters());
+ assertSent(HttpMethod.GET, uriBuilder.toString());
+ }
+
+ public void testListDatacenters404() {
+ server.enqueue(response404());
+
assertTrue(api.getInfrastructureApi().listDatacenters().concat().isEmpty());
Also verify the sent request and that there is only on request generated.
> +
+ assertEquals(250, operatingSystems.getPageSize());
+ assertEquals(33, operatingSystems.getTotalCount());
+ assertEquals(33, operatingSystems.getPageCount());
+ assertEquals(1, operatingSystems.getPageNumber());
+
+ Uris.UriBuilder uriBuilder = Uris
+
.uriBuilder("/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/infrastructure/operatingSystem");
+ uriBuilder.addQuery("datacenterId", "NA9");
+ uriBuilder.addQuery(paginationOptions.buildQueryParameters());
+ assertSent(HttpMethod.GET, uriBuilder.toString());
+ }
+
+ public void testListOperatingSystems404() {
+ server.enqueue(response404());
+
assertTrue(api.getInfrastructureApi().listOperatingSystems("NA9").concat().isEmpty());
Also verify the generated request.
> +/**
+ * Mock tests for the {@link InfrastructureApi} class.
+ */
+@Test(groups = "unit", testName = "InfrastructureApiMockTest", singleThreaded
= true)
+public class InfrastructureApiMockTest extends
BaseAccountAwareCloudControlMockTest {
+
+ public void testListDatacenters() throws Exception {
+ server.enqueue(jsonResponse("/datacenters.json"));
+ Iterable<Datacenter> datacenters =
api.getInfrastructureApi().listDatacenters().concat();
+
+ assertEquals(size(datacenters), 2); // Force the PagedIterable to advance
+ assertEquals(server.getRequestCount(), 2);
+
+ Uris.UriBuilder uriBuilder = Uris
+
.uriBuilder("/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/infrastructure/datacenter");
+ Set<String> zones =
ctx.utils().injector().getInstance(ZoneIdsSupplier.class).get();
Is there a value configured there for this? Let's make sure (perhaps in the
base class) to configure some zones so this test properly verifies the
behaviour.
> + * 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.dimensiondata.cloudcontrol.options;
+
+import com.google.common.collect.Multimap;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.util.Collection;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+@Test(groups = "unit", testName = "PaginationOptionsTest", singleThreaded =
true)
No need for this to be single-threaded. Most single-threaded tests are due to
the fact that mockwebserver starts the server in a before method. Tests that
don't use MWS don't need to be single-threaded in general.
--
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/377#pullrequestreview-29018949
