From 121783ac3b346abafb5d568be794cde1c941a47c Mon Sep 17 00:00:00 2001 From: Marco Ziccardi Date: Fri, 22 Apr 2016 21:29:07 +0200 Subject: [PATCH] Make DNS batch get zone/changerequest return null for NOT_FOUND (#953) --- .../google/cloud/BaseServiceException.java | 17 ++++++- .../java/com/google/cloud/dns/DnsBatch.java | 44 ++++++++++++----- .../com/google/cloud/dns/spi/RpcBatch.java | 2 +- .../com/google/cloud/dns/DnsBatchTest.java | 49 ++++++++++++++++++- 4 files changed, 94 insertions(+), 18 deletions(-) diff --git a/gcloud-java-core/src/main/java/com/google/cloud/BaseServiceException.java b/gcloud-java-core/src/main/java/com/google/cloud/BaseServiceException.java index ad2d6bf07144..7cc55fd7a19b 100644 --- a/gcloud-java-core/src/main/java/com/google/cloud/BaseServiceException.java +++ b/gcloud-java-core/src/main/java/com/google/cloud/BaseServiceException.java @@ -135,8 +135,21 @@ public BaseServiceException(IOException exception, boolean idempotent) { this.debugInfo = debugInfo; } - public BaseServiceException(GoogleJsonError error, boolean idempotent) { - this(error.getCode(), error.getMessage(), reason(error), idempotent); + public BaseServiceException(GoogleJsonError googleJsonError, boolean idempotent) { + super(googleJsonError.getMessage()); + Error error = new Error(googleJsonError.getCode(), reason(googleJsonError)); + this.code = error.code; + this.reason = error.reason; + this.retryable = isRetryable(idempotent, error); + if (this.reason != null) { + GoogleJsonError.ErrorInfo errorInfo = googleJsonError.getErrors().get(0); + this.location = errorInfo.getLocation(); + this.debugInfo = (String) errorInfo.get("debugInfo"); + } else { + this.location = null; + this.debugInfo = null; + } + this.idempotent = idempotent; } public BaseServiceException(int code, String message, String reason, boolean idempotent) { diff --git a/gcloud-java-dns/src/main/java/com/google/cloud/dns/DnsBatch.java b/gcloud-java-dns/src/main/java/com/google/cloud/dns/DnsBatch.java index d81fc27f3a1a..8037b6b28ce2 100644 --- a/gcloud-java-dns/src/main/java/com/google/cloud/dns/DnsBatch.java +++ b/gcloud-java-dns/src/main/java/com/google/cloud/dns/DnsBatch.java @@ -91,7 +91,7 @@ public DnsBatchResult> listZones(Dns.ZoneListOption... options) { public DnsBatchResult createZone(ZoneInfo zone, Dns.ZoneOption... options) { DnsBatchResult result = new DnsBatchResult<>(); // todo this can cause misleading report of a failure, intended to be fixed within #924 - RpcBatch.Callback callback = createZoneCallback(this.options, result, true); + RpcBatch.Callback callback = createZoneCallback(this.options, result, false, true); Map optionMap = DnsImpl.optionMap(options); batch.addCreateZone(zone.toPb(), callback, optionMap); return result; @@ -118,7 +118,7 @@ public DnsBatchResult deleteZone(String zoneName) { */ public DnsBatchResult getZone(String zoneName, Dns.ZoneOption... options) { DnsBatchResult result = new DnsBatchResult<>(); - RpcBatch.Callback callback = createZoneCallback(this.options, result, true); + RpcBatch.Callback callback = createZoneCallback(this.options, result, true, true); Map optionMap = DnsImpl.optionMap(options); batch.addGetZone(zoneName, callback, optionMap); return result; @@ -186,7 +186,7 @@ public DnsBatchResult> listChangeRequests(String zoneName, public DnsBatchResult getChangeRequest(String zoneName, String changeRequestId, Dns.ChangeRequestOption... options) { DnsBatchResult result = new DnsBatchResult<>(); - RpcBatch.Callback callback = createChangeRequestCallback(zoneName, result, true); + RpcBatch.Callback callback = createChangeRequestCallback(zoneName, result, true, true); Map optionMap = DnsImpl.optionMap(options); batch.addGetChangeRequest(zoneName, changeRequestId, callback, optionMap); return result; @@ -197,20 +197,21 @@ public DnsBatchResult getChangeRequest(String zoneName, String ch * {@code zoneName} to this batch. The {@code options} can be used to restrict the fields returned * in the same way as for {@link Dns#applyChangeRequest(String, ChangeRequestInfo, * Dns.ChangeRequestOption...)}. Calling {@link DnsBatchResult#get()} on the return value yields - * the created {@link ChangeRequest} if successful, {@code null} if the change request does not - * exist, or throws a {@link DnsException} if the operation failed or the zone does not exist. + * the created {@link ChangeRequest} if successful or throws a {@link DnsException} if the + * operation failed or the zone does not exist. */ public DnsBatchResult applyChangeRequest(String zoneName, ChangeRequestInfo changeRequest, Dns.ChangeRequestOption... options) { DnsBatchResult result = new DnsBatchResult<>(); - RpcBatch.Callback callback = createChangeRequestCallback(zoneName, result, false); + RpcBatch.Callback callback = + createChangeRequestCallback(zoneName, result, false, false); Map optionMap = DnsImpl.optionMap(options); batch.addApplyChangeRequest(zoneName, changeRequest.toPb(), callback, optionMap); return result; } /** - * Submits this batch for processing using a single HTTP request. + * Submits this batch for processing using a single RPC request. */ public void submit() { batch.submit(); @@ -259,7 +260,7 @@ public void onFailure(GoogleJsonError googleJsonError) { * A joint callback for both "get zone" and "create zone" operations. */ private RpcBatch.Callback createZoneCallback(final DnsOptions serviceOptions, - final DnsBatchResult result, final boolean idempotent) { + final DnsBatchResult result, final boolean nullForNotFound, final boolean idempotent) { return new RpcBatch.Callback() { @Override public void onSuccess(ManagedZone response) { @@ -268,7 +269,12 @@ public void onSuccess(ManagedZone response) { @Override public void onFailure(GoogleJsonError googleJsonError) { - result.error(new DnsException(googleJsonError, idempotent)); + DnsException serviceException = new DnsException(googleJsonError, idempotent); + if (nullForNotFound && serviceException.code() == HTTP_NOT_FOUND) { + result.success(null); + } else { + result.error(serviceException); + } } }; } @@ -337,17 +343,29 @@ public void onFailure(GoogleJsonError googleJsonError) { * A joint callback for both "get change request" and "create change request" operations. */ private RpcBatch.Callback createChangeRequestCallback(final String zoneName, - final DnsBatchResult result, final boolean idempotent) { + final DnsBatchResult result, final boolean nullForNotFound, + final boolean idempotent) { return new RpcBatch.Callback() { @Override public void onSuccess(Change response) { - result.success(response == null ? null : ChangeRequest.fromPb(options.service(), - zoneName, response)); + result.success(response == null ? null : ChangeRequest.fromPb(options.service(), zoneName, + response)); } @Override public void onFailure(GoogleJsonError googleJsonError) { - result.error(new DnsException(googleJsonError, idempotent)); + DnsException serviceException = new DnsException(googleJsonError, idempotent); + if (serviceException.code() == HTTP_NOT_FOUND) { + if ("entity.parameters.changeId".equals(serviceException.location()) + || (serviceException.getMessage() != null + && serviceException.getMessage().contains("parameters.changeId"))) { + // the change id was not found, but the zone exists + result.success(null); + return; + } + // the zone does not exist, so throw an exception + } + result.error(serviceException); } }; } diff --git a/gcloud-java-dns/src/main/java/com/google/cloud/dns/spi/RpcBatch.java b/gcloud-java-dns/src/main/java/com/google/cloud/dns/spi/RpcBatch.java index 154ffa4a3c99..6fa3cfb70718 100644 --- a/gcloud-java-dns/src/main/java/com/google/cloud/dns/spi/RpcBatch.java +++ b/gcloud-java-dns/src/main/java/com/google/cloud/dns/spi/RpcBatch.java @@ -110,7 +110,7 @@ void addApplyChangeRequest(String zoneName, Change change, Callback call Map options); /** - * Submits a batch of requests for processing using a single HTTP request to Cloud DNS. + * Submits a batch of requests for processing using a single RPC request to Cloud DNS. */ void submit(); } diff --git a/gcloud-java-dns/src/test/java/com/google/cloud/dns/DnsBatchTest.java b/gcloud-java-dns/src/test/java/com/google/cloud/dns/DnsBatchTest.java index bd743a3babb6..746a65ab7155 100644 --- a/gcloud-java-dns/src/test/java/com/google/cloud/dns/DnsBatchTest.java +++ b/gcloud-java-dns/src/test/java/com/google/cloud/dns/DnsBatchTest.java @@ -19,6 +19,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -218,7 +219,9 @@ public void testCreateZone() { } // testing error here, success is tested with options RpcBatch.Callback capturedCallback = callback.getValue(); - capturedCallback.onFailure(GOOGLE_JSON_ERROR); + GoogleJsonError error = new GoogleJsonError(); + error.setCode(404); + capturedCallback.onFailure(error); try { batchResult.get(); fail("Should throw a DnsException on error."); @@ -279,6 +282,23 @@ public void testGetZone() { } } + @Test + public void testGetZoneNotFound() { + EasyMock.reset(batchMock); + Capture> callback = Capture.newInstance(); + Capture> capturedOptions = Capture.newInstance(); + batchMock.addGetZone(EasyMock.eq(ZONE_NAME), EasyMock.capture(callback), + EasyMock.capture(capturedOptions)); + EasyMock.replay(batchMock); + DnsBatchResult batchResult = dnsBatch.getZone(ZONE_NAME); + assertEquals(0, capturedOptions.getValue().size()); + GoogleJsonError error = new GoogleJsonError(); + error.setCode(404); + RpcBatch.Callback capturedCallback = callback.getValue(); + capturedCallback.onFailure(error); + assertNull(batchResult.get()); + } + @Test public void testGetZoneWithOptions() { EasyMock.reset(dns, batchMock, optionsMock); @@ -556,6 +576,29 @@ public void testGetChangeRequest() { } } + @Test + public void testGetChangeRequestNotFound() { + EasyMock.reset(batchMock); + Capture> callback = Capture.newInstance(); + Capture> capturedOptions = Capture.newInstance(); + batchMock.addGetChangeRequest(EasyMock.eq(ZONE_NAME), + EasyMock.eq(CHANGE_REQUEST_COMPLETE.generatedId()), EasyMock.capture(callback), + EasyMock.capture(capturedOptions)); + EasyMock.replay(batchMock); + DnsBatchResult batchResult = dnsBatch.getChangeRequest(ZONE_NAME, + CHANGE_REQUEST_COMPLETE.generatedId()); + assertEquals(0, capturedOptions.getValue().size()); + RpcBatch.Callback capturedCallback = callback.getValue(); + GoogleJsonError error = new GoogleJsonError(); + GoogleJsonError.ErrorInfo errorInfo = new GoogleJsonError.ErrorInfo(); + errorInfo.setReason("reason"); + errorInfo.setLocation("entity.parameters.changeId"); + error.setCode(404); + error.setErrors(ImmutableList.of(errorInfo)); + capturedCallback.onFailure(error); + assertNull(batchResult.get()); + } + @Test public void testGetChangeRequestWithOptions() { EasyMock.reset(dns, batchMock, optionsMock); @@ -599,7 +642,9 @@ public void testApplyChangeRequest() { } // testing error here, success is tested with options RpcBatch.Callback capturedCallback = callback.getValue(); - capturedCallback.onFailure(GOOGLE_JSON_ERROR); + GoogleJsonError error = new GoogleJsonError(); + error.setCode(404); + capturedCallback.onFailure(error); try { batchResult.get(); fail("Should throw a DnsException on error.");