nacx commented on this pull request.
Thanks @FileIOUtility!
> @@ -68,21 +68,19 @@
@GET
@Path("/server")
@ResponseParser(ParseServers.class)
- @Fallback(Fallbacks.EmptyIterableWithMarkerOnNotFoundOr404.class)
This is a behavior change since this method will now fail upon 4xx responses,
when previously it just returned an empty list. Consider adding the
corresponding fallback and keep returning empty lists.
> PaginatedCollection<Server> listServers(DatacenterIdListFilters
> datacenterIdListFilters);
@Named("server:list")
@GET
@Path("/server")
@Transform(ParseServers.ToPagedIterable.class)
@ResponseParser(ParseServers.class)
- @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
Same here
> @@ -66,9 +66,9 @@ public void handleError(HttpCommand command, HttpResponse
> response) {
exception = new AuthorizationException(message, exception);
break;
case 404:
- if (!command.getCurrentRequest().getMethod().equals("DELETE")) {
- exception = new ResourceNotFoundException(message, exception);
- }
+ // CloudControl uses error code 400 with RESOURCE_NOT_FOUND to
report missing assets
+ // 404 means malformed URI only
+ exception = new IllegalStateException(message, exception);
A malformed URL in a request is better represented as an
`IllegalArgumentException`. Illegal sates are more for 409 (Conflict) errors.
> + return null;
+ } else {
+ throw Throwables.propagate(t);
+ }
+ }
+ }
+
+ public static final class VoidOnResourceNotFound implements Fallback<Void> {
+ public Void createOrPropagate(Throwable t) {
+ if (t instanceof ResourceNotFoundException) {
+ return null;
+ } else {
+ throw Throwables.propagate(t);
+ }
+ }
+ }
You'd better copy the [default
behavior](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/Fallbacks.java#L132)
but changing the code to 400, to make sure you just capture the right
responses.
> @@ -121,10 +121,9 @@ public void testListServersWithDatacenterFiltering()
> throws Exception {
return
Uris.uriBuilder("/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/server");
}
- public void testListServers_404() throws Exception {
- server.enqueue(response404());
- assertTrue(serverApi().listServers().concat().isEmpty());
- assertSent(HttpMethod.GET, getListServerUriBuilder().toString());
+ public void testListServers_NoServersFound() {
+ server.enqueue(emptyListResponse("server"));
This test does not exercise the fallback, as it returns a 200 response. Change
it to return a 400 error code (you'll need the fallbacks for the empty list
mentioned previously).
> @@ -135,10 +134,10 @@ public void testGetServer() throws Exception {
assertNotNull(found.guest().vmTools());
}
- public void testGetServer_404() throws Exception {
- server.enqueue(response404());
- serverApi().getServer("12345");
- assertSent(GET,
"/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/server/12345");
+ public void testGetServer_NotFound() {
+ server.enqueue(responseResourceNotFound());
+ Server foundServer = serverApi().getServer("12345");
+ assertNull(foundServer, "should return null when Server was not found");
Same here. Get back the test that exercises the 400 fallback.
--
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/439#pullrequestreview-134404077
