nacx requested changes on this pull request.
Many thanks @jawnclarke!
I've added several comments. The PR looks good, but let's keep it simple to set
the right foundation so we can easily add more APIs and build stuff on top of
this without having to revisit it.
Figuring out the base stuff is the difficult part, and things will move faster
once this is merged.
> + @Override
+ public Builder toBuilder() {
+ return new Builder().fromApiMetadata(this);
+ }
+
+ public DimensionDataCloudControlApiMetadata() {
+ this(new Builder());
+ }
+
+ protected DimensionDataCloudControlApiMetadata(Builder builder) {
+ super(builder);
+ }
+
+ public static Properties defaultProperties() {
+ Properties properties = BaseHttpApiMetadata.defaultProperties();
+ properties.setProperty(Constants.PROPERTY_CONNECTION_TIMEOUT,
"1200000"); // 15 minutes
Can we avoid setting this? The connection timeout configures the time to get a
connection opened, not the time required to read all the data. It looks
excessive. Let's better remove this default and let users configure it as
needed.
> + properties.setProperty(Constants.PROPERTY_CONNECTION_TIMEOUT,
> "1200000"); // 15 minutes
+ return properties;
+ }
+
+ public static class Builder extends
BaseHttpApiMetadata.Builder<DimensionDataCloudControlApi, Builder> {
+
+ protected Builder() {
+ id("dimensiondata-cloudcontrol").name("DimensionData CloudControl
API").identityName("user name")
+ .credentialName("user password")
+
.documentation(URI.create("http://www.dimensiondata.com/en-US/Solutions/Cloud";))
+
.defaultEndpoint("https://api-REGION.dimensiondata.com/";).version("2.4")
+
.defaultProperties(DimensionDataCloudControlApiMetadata.defaultProperties())
+ .view(typeToken(ComputeServiceContext.class)).defaultModules(
+ ImmutableSet.<Class<? extends
Module>>builder().add(DimensionDataCloudControlHttpApiModule.class)
+ .add(DimensionDataCloudControlParserModule.class)
+
.add(DimensionDataComputeServiceContextModule.class).build());
For now let's remove the ComputeServiceAdapter module, the implementation and
all other transformation functions. They are not implemented and return `null`
and placeholders, and provide no value and just add complexity to the code and
the PR. Let's remove all that stuff and start with a clean API to set a good
foundation for the provider.
That will help and allow further PRs to be easier and faster.
> +
+ @Override
+ protected void bindErrorHandlers() {
+ //TODO Implement Method
+ }
+
+ @Override
+ protected void bindRetryHandlers() {
+ //TODO Implement Method
+ }
+
+ @Override
+ protected void installLocations() {
+ install(new LocationModule());
+
bind(ImplicitLocationSupplier.class).to(OnlyLocationOrFirstZone.class).in(Scopes.SINGLETON);
+ }
Don't override this yet. Let's remove it for now and we'll add it back (if it
turns needed) when implementing the compute service adapter.
> +import org.jclouds.location.suppliers.implicit.OnlyLocationOrFirstZone;
+import org.jclouds.rest.ConfiguresHttpApi;
+import org.jclouds.rest.config.HttpApiModule;
+
+@ConfiguresHttpApi
+public class DimensionDataCloudControlHttpApiModule extends
HttpApiModule<DimensionDataCloudControlApi> {
+
+ @Override
+ protected void bindErrorHandlers() {
+ //TODO Implement Method
+ }
+
+ @Override
+ protected void bindRetryHandlers() {
+ //TODO Implement Method
+ }
You can remove this, as jclouds provides sane default retry handlers.
> +
+import com.google.inject.Scopes;
+import org.jclouds.dimensiondata.cloudcontrol.DimensionDataCloudControlApi;
+import org.jclouds.location.config.LocationModule;
+import org.jclouds.location.suppliers.ImplicitLocationSupplier;
+import org.jclouds.location.suppliers.implicit.OnlyLocationOrFirstZone;
+import org.jclouds.rest.ConfiguresHttpApi;
+import org.jclouds.rest.config.HttpApiModule;
+
+@ConfiguresHttpApi
+public class DimensionDataCloudControlHttpApiModule extends
HttpApiModule<DimensionDataCloudControlApi> {
+
+ @Override
+ protected void bindErrorHandlers() {
+ //TODO Implement Method
+ }
Remove this method if you're not defining any custom error handler.
> + * limitations under the License.
+ */
+package org.jclouds.dimensiondata.cloudcontrol.config;
+
+import com.google.inject.AbstractModule;
+import com.google.inject.name.Names;
+import org.jclouds.Constants;
+import org.jclouds.json.config.GsonModule;
+
+import java.util.Collections;
+
+public class DimensionDataCloudControlParserModule extends AbstractModule {
+
+ @Override
+ protected void configure() {
+ Names.bindProperties(binder(),
Collections.singletonMap(Constants.PROPERTY_PRETTY_PRINT_PAYLOADS, "true"));
There is no need to bind this. It is already bound to the context and users
will be able to configure this property when creating it. Let's remove this
line.
>
- Account() {
+ @Nullable
+ public abstract AccountPhoneNumber phone();
+
+ public abstract AccountOrganization organization();
+
+ @SerializedNames({ "userName", "fullName", "firstName", "lastName",
"emailAddress", "roles", "phone", "department",
+ "customDefined1", "customDefined2", "organization", "state" })
+ public static Account create(String userName, String fullName, String
firstName, String lastName,
+ String emailAddress, List<RoleType> roles, AccountPhoneNumber phone,
String department, String customDefined1,
+ String customDefined2, AccountOrganization organization, String
state) {
+ return
builder().userName(userName).fullName(fullName).firstName(firstName).lastName(lastName)
+ .emailAddress(emailAddress)
+ .roles(roles == null ? ImmutableList.<RoleType>of() :
ImmutableList.copyOf(roles)).phone(phone)
Just pass the list here as it comes from the parameter, and move the
immutability logic to the builder following [this
pattern](https://github.com/jclouds/jclouds-labs/blob/master/dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/Backup.java#L53-L62).
>
- Account() {
Why has this been removed? Providing a non-visible default constructor is a
best-practice to enforce using the builders. Let's add the constructors back.
>
- Account() {
+ @Nullable
+ public abstract AccountPhoneNumber phone();
+
+ public abstract AccountOrganization organization();
+
+ @SerializedNames({ "userName", "fullName", "firstName", "lastName",
"emailAddress", "roles", "phone", "department",
+ "customDefined1", "customDefined2", "organization", "state" })
jclouds makes some assumptions when serializing and deserializing. The order of
the parameters in this annotation **must be the same** than the order in which
the variables are declared in the class. Please, reorder accordingly.
> +import org.jclouds.http.HttpRequestFilter;
+
+import java.util.List;
+
+/**
+ * Accepts requests and modifies the endpoint path so that it is injected with
the organisation id.
+ * Handles both oec and caas based URLs.
+ */
+@Singleton
+public class OrganisationIdFilter implements HttpRequestFilter {
+
+ private static final int ORGANIZATION_ID_INDEX = 3;
+ private final Supplier<String> organisationIdSupplier;
+
+ @Inject
+ public OrganisationIdFilter(@Memoized Supplier<String>
organisationIdSupplier) {
Remove the `public` modifier. It is a best-practice in Guice to make all
injection constructors visible just to the default scope.
> +
+ // to have the compute service adapter override default locations
+ install(new
LocationsFromComputeServiceAdapterModule<ServerWithExternalIp, BaseImage,
BaseImage, Datacenter>() {
+ });
+
+ super.configure();
+ }
+
+ @Provides
+ @Singleton
+ @Memoized
+ public final Supplier<String>
getOrganisationIdForAccount(AtomicReference<AuthorizationException>
authException,
+ @Named(PROPERTY_SESSION_INTERVAL) long seconds,
DimensionDataCloudControlApi api) {
+ return MemoizedRetryOnTimeOutButNotOnAuthorizationExceptionSupplier
+ .create(authException, new OrganisationIdForAccount(api), seconds,
TimeUnit.SECONDS);
+ }
Better move this supplier to the HttpApi module, since it is generic for the
provider APIs, not specifically about the compute abstraction.
> +import org.jclouds.logging.Logger;
+
+import javax.annotation.Resource;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+@Singleton
+public class OrganisationIdForAccount implements Supplier<String> {
+ @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
+
+ private final AccountApi api;
+
+ @Inject
+ public OrganisationIdForAccount(DimensionDataCloudControlApi api) {
Remove the `public` modifier.
> +import com.google.inject.name.Named;
+import org.jclouds.compute.reference.ComputeServiceConstants;
+import org.jclouds.dimensiondata.cloudcontrol.DimensionDataCloudControlApi;
+import org.jclouds.dimensiondata.cloudcontrol.domain.Account;
+import org.jclouds.dimensiondata.cloudcontrol.features.AccountApi;
+import org.jclouds.logging.Logger;
+
+import javax.annotation.Resource;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+@Singleton
+public class OrganisationIdForAccount implements Supplier<String> {
+ @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
better remove if unused.
> +
+ private static final int ORGANIZATION_ID_INDEX = 3;
+ private final Supplier<String> organisationIdSupplier;
+
+ @Inject
+ public OrganisationIdFilter(@Memoized Supplier<String>
organisationIdSupplier) {
+ this.organisationIdSupplier = organisationIdSupplier;
+ }
+
+ @Override
+ public HttpRequest filter(HttpRequest request) throws HttpException {
+ return
request.toBuilder().replacePath(injectOrganisationId(request.getEndpoint().getPath())).build();
+ }
+
+ @VisibleForTesting
+ public String injectOrganisationId(String path) {
Don't make it public if it is just visible for testing. Remove the public
modifier and leave it in default scope.
> + assertNotNull(account);
+ assertEquals("devlab1-online-vendor-devuser1", account.userName());
+ assertEquals("Donal Lunny", account.fullName());
+ assertEquals("Donal", account.firstName());
+ assertEquals("Lunny", account.lastName());
+ assertEquals("[email protected]",
account.emailAddress());
+ assertThat(account.roles().contains(Account.RoleType.create("storage")));
+ assertThat(account.roles().contains(Account.RoleType.create("primary
administrator")));
+ assertEquals("1", account.phone().countryCode());
+ assertEquals("1234567", account.phone().number());
+ assertEquals("6ac1e746-b1ea-4da5-a24e-caf1a978789d",
account.organization().id());
+ assertEquals("Eng Customer of DevLab1-homed online sign-up Vendor",
account.organization().name());
+ assertEquals("DevLab1Geo1", account.organization().homeGeoName());
+ assertEquals("apidevlab1.opsourcecloud.net",
account.organization().homeGeoApiHost());
+ assertEquals("DEVLAB1GEO1", account.organization().homeGeoId());
+ assertEquals("NORMAL", account.state());
Values seem to be of a particular user. Change them for `notNull` asserts or
something that can be run by anyone.
> + * 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.dimensiondata.cloudcontrol.features;
+
+import org.jclouds.dimensiondata.cloudcontrol.domain.Account;
+import
org.jclouds.dimensiondata.cloudcontrol.internal.BaseDimensionDataCloudControlApiLiveTest;
+import org.testng.annotations.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.AssertJUnit.assertEquals;
+
+@Test(groups = "live", testName = "AccountApiLiveTest", singleThreaded = true)
Looks like it does not need to be single-threaded.
> +import org.testng.annotations.Test;
+
+import java.io.IOException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.AssertJUnit.assertEquals;
+
+@Test(groups = "unit", testName = "AccountApiMockTest", singleThreaded = true)
+public class AccountApiMockTest extends BaseDimensionDataCloudControlMockTest {
+
+ @BeforeMethod
+ @Override
+ public void start() throws IOException {
+ super.start();
+ }
No need to override this.
> + assertEquals("Donal", account.firstName());
+ assertEquals("Lunny", account.lastName());
+ assertEquals("[email protected]",
account.emailAddress());
+ assertThat(account.roles().contains(Account.RoleType.create("storage")));
+ assertThat(account.roles().contains(Account.RoleType.create("primary
administrator")));
+ assertEquals("1", account.phone().countryCode());
+ assertEquals("1234567", account.phone().number());
+ assertEquals("Marketing", account.department());
+ assertEquals("custom defined 1", account.customDefined1());
+ assertEquals("custom defined 2", account.customDefined2());
+ assertEquals("6ac1e746-b1ea-4da5-a24e-caf1a978789d",
account.organization().id());
+ assertEquals("Eng Customer of DevLab1-homed online sign-up Vendor",
account.organization().name());
+ assertEquals("DevLab1Geo1", account.organization().homeGeoName());
+ assertEquals("apidevlab1.opsourcecloud.net",
account.organization().homeGeoApiHost());
+ assertEquals("DEVLAB1GEO1", account.organization().homeGeoId());
+ assertEquals("NORMAL", account.state());
There is no need to verify the response deserialization in mock tests, so you
can remove all these asserts. We just want to make sure the generated requests
are the expected ones.
That method uses basic authentication (via its filter), so let's modify the
`assertSent` method in the base mock test class to verify that the
authentication header is sent too.
> + assertThat(account.roles().contains(Account.RoleType.create("primary
> administrator")));
+ assertEquals("1", account.phone().countryCode());
+ assertEquals("1234567", account.phone().number());
+ assertEquals("Marketing", account.department());
+ assertEquals("custom defined 1", account.customDefined1());
+ assertEquals("custom defined 2", account.customDefined2());
+ assertEquals("6ac1e746-b1ea-4da5-a24e-caf1a978789d",
account.organization().id());
+ assertEquals("Eng Customer of DevLab1-homed online sign-up Vendor",
account.organization().name());
+ assertEquals("DevLab1Geo1", account.organization().homeGeoName());
+ assertEquals("apidevlab1.opsourcecloud.net",
account.organization().homeGeoApiHost());
+ assertEquals("DEVLAB1GEO1", account.organization().homeGeoId());
+ assertEquals("NORMAL", account.state());
+
+ assertSent(server, "GET", "/caas/2.4/user/myUser");
+ }
+
Add another test method that simulates a 404 response, to verify that the
method returns `null`.
Since you are defining an explicit `@Fallback` in the api method, we'd better
verify it behaves as expected on the affected responses.
> + * limitations under the License.
+ */
+package org.jclouds.dimensiondata.cloudcontrol.filters;
+
+import com.google.common.base.Supplier;
+import
org.jclouds.dimensiondata.cloudcontrol.compute.suppliers.OrganisationIdForAccount;
+import
org.jclouds.dimensiondata.cloudcontrol.internal.BaseDimensionDataCloudControlMockTest;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+
+import static org.testng.Assert.assertEquals;
+
+@Test(groups = "unit", testName = "OrganisationIdFilterTest", singleThreaded =
true)
+public class OrganisationIdFilterTest extends
BaseDimensionDataCloudControlMockTest {
There is no need for this test to be single-threaded nor to extend the base
mock test class. You don't need to actually start a mock web server to test
this class.
> +import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+
+import static org.testng.Assert.assertEquals;
+
+@Test(groups = "unit", testName = "OrganisationIdFilterTest", singleThreaded =
true)
+public class OrganisationIdFilterTest extends
BaseDimensionDataCloudControlMockTest {
+
+ @BeforeMethod
+ @Override
+ public void start() throws IOException {
+ super.start();
+ server.enqueue(xmlResponse("/account.json"));
+ }
Also remove this.
> +
+import static org.testng.Assert.assertEquals;
+
+@Test(groups = "unit", testName = "OrganisationIdFilterTest", singleThreaded =
true)
+public class OrganisationIdFilterTest extends
BaseDimensionDataCloudControlMockTest {
+
+ @BeforeMethod
+ @Override
+ public void start() throws IOException {
+ super.start();
+ server.enqueue(xmlResponse("/account.json"));
+ }
+
+ @Test
+ public void testCaasUrl() {
+ Supplier<String> orgIdSupplier = new OrganisationIdForAccount(api);
You can just return a mock supplier for this test:
`Suppliers.ofInstance("6ac1e746-b1ea-4da5-a24e-caf1a978789d/")`.
This way you don't need the overhead of starting a mock server, creating the
context, etc.
> + public void testCaasUrl() {
+ Supplier<String> orgIdSupplier = new OrganisationIdForAccount(api);
+ String expectedPath =
"/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/0896551e-4fe3-4450-a627-ad5548e7e83a?clone=trevor-test2&desc=trevor-description2";
+ String updatedPath = new
OrganisationIdFilter(orgIdSupplier).injectOrganisationId(
+
"/caas/2.4/server/0896551e-4fe3-4450-a627-ad5548e7e83a?clone=trevor-test2&desc=trevor-description2");
+ assertEquals(updatedPath, expectedPath);
+ }
+
+ @Test
+ public void testDontAddIfAlreadyPresent() {
+ Supplier<String> orgIdSupplier = new OrganisationIdForAccount(api);
+ String expectedPath =
"/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/0896551e-4fe3-4450-a627-ad5548e7e83a";
+ String updatedPath = new
OrganisationIdFilter(orgIdSupplier).injectOrganisationId(
+
"/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/0896551e-4fe3-4450-a627-ad5548e7e83a");
+ assertEquals(updatedPath, expectedPath);
+ }
Also add a test to verify the behavior when the path segments are < 3 ?
> +@Test(groups = "live")
+public class BaseDimensionDataCloudControlApiLiveTest extends
BaseApiLiveTest<DimensionDataCloudControlApi> {
+
+ public BaseDimensionDataCloudControlApiLiveTest() {
+ provider = "dimensiondata-cloudcontrol";
+ }
+
+ @Override
+ protected ApiMetadata createApiMetadata() {
+ return new DimensionDataCloudControlApiMetadata();
+
+ }
+
+ @Override
+ protected Iterable<Module> setupModules() {
+ return ImmutableSet.<Module>of(getLoggingModule(), new
SshjSshClientModule());
The SSH module is only needed for the compute service tests, so you could
remove this override method.
> + protected ApiMetadata createApiMetadata() {
+ return new DimensionDataCloudControlApiMetadata();
+
+ }
+
+ @Override
+ protected Iterable<Module> setupModules() {
+ return ImmutableSet.<Module>of(getLoggingModule(), new
SshjSshClientModule());
+ }
+
+ @Override
+ protected Properties setupProperties() {
+ Properties overrides = super.setupProperties();
+ overrides.setProperty("jclouds.ssh.retry-auth", "true");
+ return overrides;
+ }
SSH config is only needed for compute tests, so you should be able to remove
this method too.
> +
+ protected Properties overrides() {
+ return new Properties();
+ }
+
+ protected String url(String path) {
+ return server.getUrl(path).toString();
+ }
+
+ protected MockResponse jsonResponse(String resource) {
+ return new MockResponse().addHeader("Content-Type",
"application/json").setBody(stringFromResource(resource));
+ }
+
+ protected MockResponse xmlResponse(String resource) {
+ return new MockResponse().addHeader("Content-Type",
"application/xml").setBody(stringFromResource(resource));
+ }
Remove this? Looks like we're just consuming a json api.
> + return new MockResponse().addHeader("Content-Type",
> "application/json").setBody(stringFromResource(resource));
+ }
+
+ protected MockResponse xmlResponse(String resource) {
+ return new MockResponse().addHeader("Content-Type",
"application/xml").setBody(stringFromResource(resource));
+ }
+
+ protected MockResponse responseUnexpectedError() {
+ return new MockResponse().setResponseCode(400).setStatus("HTTP/1.1 400
Bad Request")
+ .setBody("content:
[{\"operation\":\"OPERATION\",\"responseCode\":\"UNEXPECTED_ERROR\"}]");
+ }
+
+ protected MockResponse responseResourceNotFound() {
+ return new MockResponse().setResponseCode(400).setStatus("HTTP/1.1 400
Bad Request")
+ .setBody("content:
[{\"operation\":\"OPERATION\",\"responseCode\":\"RESOURCE_NOT_FOUND\"}]");
+ }
Hmmm, upon a 400 response, is this what the DimensionData API returns? Do
clients need to actually read the response body to really know the cause of the
failure?
In any case, it looks like we need a custom error handler to propagate the
right exception upon failed requests. jclouds by default throws
`HttpResponseExceptions`, but there are other exceptions that better represent
the different response codes:
* IllegalArgumentException - for 400 Bad Request or 409 Conflict.
* ResourceNotFoundException - for 404 Not Found.
* AuthorizationException - for 403 Unauthorized.
Those can be handled by creating a `DimensionDataCloudControlErrorHandler` (you
can take [the Packet
one](https://github.com/jclouds/jclouds-labs/blob/master/packet/src/main/java/org/jclouds/packet/handlers/PacketErrorHandler.java)
as an example) and declaring it in the http api module (as in
[here](https://github.com/jclouds/jclouds-labs/blob/master/packet/src/main/java/org/jclouds/packet/config/PacketHttpApiModule.java#L48-L53)).
--
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/375#pullrequestreview-28841227
