Skip to content

Commit

Permalink
Make DNS batch get zone/changerequest return null for NOT_FOUND (#953)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mziccard
mziccard committed Apr 22, 2016
1 parent 0a6c6f7 commit 121783a
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 18 deletions.
17 changes: 15 additions & 2 deletions gcloud-java-core/src/main/java/com/google/cloud/BaseServiceException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
44 changes: 31 additions & 13 deletions gcloud-java-dns/src/main/java/com/google/cloud/dns/DnsBatch.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ public DnsBatchResult<Page<Zone>> listZones(Dns.ZoneListOption... options) {
public DnsBatchResult<Zone> createZone(ZoneInfo zone, Dns.ZoneOption... options) {
DnsBatchResult<Zone> result = new DnsBatchResult<>();
// todo this can cause misleading report of a failure, intended to be fixed within #924
RpcBatch.Callback<ManagedZone> callback = createZoneCallback(this.options, result, true);
RpcBatch.Callback<ManagedZone> callback = createZoneCallback(this.options, result, false, true);
Map<DnsRpc.Option, ?> optionMap = DnsImpl.optionMap(options);
batch.addCreateZone(zone.toPb(), callback, optionMap);
return result;
Expand All @@ -118,7 +118,7 @@ public DnsBatchResult<Boolean> deleteZone(String zoneName) {
*/
public DnsBatchResult<Zone> getZone(String zoneName, Dns.ZoneOption... options) {
DnsBatchResult<Zone> result = new DnsBatchResult<>();
RpcBatch.Callback<ManagedZone> callback = createZoneCallback(this.options, result, true);
RpcBatch.Callback<ManagedZone> callback = createZoneCallback(this.options, result, true, true);
Map<DnsRpc.Option, ?> optionMap = DnsImpl.optionMap(options);
batch.addGetZone(zoneName, callback, optionMap);
return result;
Expand Down Expand Up @@ -186,7 +186,7 @@ public DnsBatchResult<Page<ChangeRequest>> listChangeRequests(String zoneName,
public DnsBatchResult<ChangeRequest> getChangeRequest(String zoneName, String changeRequestId,
Dns.ChangeRequestOption... options) {
DnsBatchResult<ChangeRequest> result = new DnsBatchResult<>();
RpcBatch.Callback<Change> callback = createChangeRequestCallback(zoneName, result, true);
RpcBatch.Callback<Change> callback = createChangeRequestCallback(zoneName, result, true, true);
Map<DnsRpc.Option, ?> optionMap = DnsImpl.optionMap(options);
batch.addGetChangeRequest(zoneName, changeRequestId, callback, optionMap);
return result;
Expand All @@ -197,20 +197,21 @@ public DnsBatchResult<ChangeRequest> 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<ChangeRequest> applyChangeRequest(String zoneName,
ChangeRequestInfo changeRequest, Dns.ChangeRequestOption... options) {
DnsBatchResult<ChangeRequest> result = new DnsBatchResult<>();
RpcBatch.Callback<Change> callback = createChangeRequestCallback(zoneName, result, false);
RpcBatch.Callback<Change> callback =
createChangeRequestCallback(zoneName, result, false, false);
Map<DnsRpc.Option, ?> 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();
Expand Down Expand Up @@ -259,7 +260,7 @@ public void onFailure(GoogleJsonError googleJsonError) {
* A joint callback for both "get zone" and "create zone" operations.
*/
private RpcBatch.Callback<ManagedZone> createZoneCallback(final DnsOptions serviceOptions,
final DnsBatchResult<Zone> result, final boolean idempotent) {
final DnsBatchResult<Zone> result, final boolean nullForNotFound, final boolean idempotent) {
return new RpcBatch.Callback<ManagedZone>() {
@Override
public void onSuccess(ManagedZone response) {
Expand All @@ -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);
}
}
};
}
Expand Down Expand Up @@ -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<Change> createChangeRequestCallback(final String zoneName,
final DnsBatchResult<ChangeRequest> result, final boolean idempotent) {
final DnsBatchResult<ChangeRequest> result, final boolean nullForNotFound,
final boolean idempotent) {
return new RpcBatch.Callback<Change>() {
@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);
}
};
}
Expand Down
2 changes: 1 addition & 1 deletion gcloud-java-dns/src/main/java/com/google/cloud/dns/spi/RpcBatch.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ void addApplyChangeRequest(String zoneName, Change change, Callback<Change> call
Map<DnsRpc.Option, ?> 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();
}
Expand Down
49 changes: 47 additions & 2 deletions gcloud-java-dns/src/test/java/com/google/cloud/dns/DnsBatchTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -218,7 +219,9 @@ public void testCreateZone() {
}
// testing error here, success is tested with options
RpcBatch.Callback<ManagedZone> 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.");
Expand Down Expand Up @@ -279,6 +282,23 @@ public void testGetZone() {
}
}

@Test
public void testGetZoneNotFound() {
EasyMock.reset(batchMock);
Capture<RpcBatch.Callback<ManagedZone>> callback = Capture.newInstance();
Capture<Map<DnsRpc.Option, Object>> capturedOptions = Capture.newInstance();
batchMock.addGetZone(EasyMock.eq(ZONE_NAME), EasyMock.capture(callback),
EasyMock.capture(capturedOptions));
EasyMock.replay(batchMock);
DnsBatchResult<Zone> batchResult = dnsBatch.getZone(ZONE_NAME);
assertEquals(0, capturedOptions.getValue().size());
GoogleJsonError error = new GoogleJsonError();
error.setCode(404);
RpcBatch.Callback<ManagedZone> capturedCallback = callback.getValue();
capturedCallback.onFailure(error);
assertNull(batchResult.get());
}

@Test
public void testGetZoneWithOptions() {
EasyMock.reset(dns, batchMock, optionsMock);
Expand Down Expand Up @@ -556,6 +576,29 @@ public void testGetChangeRequest() {
}
}

@Test
public void testGetChangeRequestNotFound() {
EasyMock.reset(batchMock);
Capture<RpcBatch.Callback<Change>> callback = Capture.newInstance();
Capture<Map<DnsRpc.Option, Object>> 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<ChangeRequest> batchResult = dnsBatch.getChangeRequest(ZONE_NAME,
CHANGE_REQUEST_COMPLETE.generatedId());
assertEquals(0, capturedOptions.getValue().size());
RpcBatch.Callback<Change> 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);
Expand Down Expand Up @@ -599,7 +642,9 @@ public void testApplyChangeRequest() {
}
// testing error here, success is tested with options
RpcBatch.Callback<Change> 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.");
Expand Down

0 comments on commit 121783a

Please sign in to comment.