From b241d8cf091855b3c6918d91f95ebca7528ab626 Mon Sep 17 00:00:00 2001 From: Thilo Schwarz Date: Sun, 11 Jan 2026 18:28:22 +0100 Subject: [PATCH] issue #9 - Refactor `CfClientTest`, `CfDnsClient`, and `CfBasicHttpClient`: improve error handling, enable record filtering, refine empty result behavior, and update tests for clarity and robustness. --- .../codes/thischwa/cf/CfBasicHttpClient.java | 15 +++- .../java/codes/thischwa/cf/CfDnsClient.java | 51 ++++++------ .../java/codes/thischwa/cf/CfClientTest.java | 78 +++++++++++-------- 3 files changed, 89 insertions(+), 55 deletions(-) diff --git a/src/main/java/codes/thischwa/cf/CfBasicHttpClient.java b/src/main/java/codes/thischwa/cf/CfBasicHttpClient.java index 8b260c8..a2ed0c3 100644 --- a/src/main/java/codes/thischwa/cf/CfBasicHttpClient.java +++ b/src/main/java/codes/thischwa/cf/CfBasicHttpClient.java @@ -71,8 +71,19 @@ abstract class CfBasicHttpClient { T respObj = objectMapper.readValue(result.responseBody, responseType); if (!respObj.getResponseResultInfo().isSuccess()) { log.error("API error."); - respObj.getResponseResultInfo().getErrors().forEach(e -> log.error(" - {}", e.toString())); - throw new CloudflareApiException("API error."); + StringBuilder errorMessage = new StringBuilder("API error"); + if (!respObj.getResponseResultInfo().getErrors().isEmpty()) { + errorMessage.append(": "); + respObj.getResponseResultInfo().getErrors().forEach(e -> { + log.error(" - {}", e.toString()); + errorMessage.append(e).append("; "); + }); + // Remove trailing "; " + if (errorMessage.toString().endsWith("; ")) { + errorMessage.setLength(errorMessage.length() - 2); + } + } + throw new CloudflareApiException(errorMessage.toString()); } logUri = request.getRequestUri(); if (result.statusCode >= 200 && result.statusCode < 300) { diff --git a/src/main/java/codes/thischwa/cf/CfDnsClient.java b/src/main/java/codes/thischwa/cf/CfDnsClient.java index bb39306..3e78488 100644 --- a/src/main/java/codes/thischwa/cf/CfDnsClient.java +++ b/src/main/java/codes/thischwa/cf/CfDnsClient.java @@ -13,6 +13,7 @@ import codes.thischwa.cf.model.RecordType; import codes.thischwa.cf.model.ZoneEntity; import codes.thischwa.cf.model.ZoneMultipleResponse; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -51,6 +52,8 @@ public class CfDnsClient extends CfBasicHttpClient { private final ResponseValidator responseValidator; + private final boolean emptyResultThrowsException; + /** * Constructs a new instance of {@code CfDnsClient}. * @@ -107,6 +110,7 @@ public class CfDnsClient extends CfBasicHttpClient { String authKey) { super(baseUrl, authEmail, authKey); this.responseValidator = new ResponseValidator(emptyResultThrowsException); + this.emptyResultThrowsException = emptyResultThrowsException; } private static String buildFqdn(ZoneEntity zone, String sld) { @@ -216,15 +220,12 @@ public class CfDnsClient extends CfBasicHttpClient { */ public List recordList(ZoneEntity zone, String sld, @Nullable RecordType... types) throws CloudflareApiException { - String fqdn = buildFqdn(zone, sld); - String endpoint = buildEndpointWithTypeFilters(CfRequest.RECORD_INFO_NAME.buildPath(zone.getId(), fqdn), types); - RecordMultipleResponse resp = getRequest(endpoint, RecordMultipleResponse.class); - checkResponse(resp, false); - List recs = resp.getResult(); - recs.forEach(rec -> rec.setZoneId(zone.getId())); - return recs; + PagingRequest pagingRequest = PagingRequest.defaultPaging(); + List recs = recordList(zone, sld, pagingRequest); + return filterAndSetZoneRecords(zone, types, recs); } + /** * Retrieves all record entities for a specific second-level domain (SLD) within a given DNS * zone using the provided paging request parameters. @@ -256,23 +257,11 @@ public class CfDnsClient extends CfBasicHttpClient { */ public List recordList(ZoneEntity zone, RecordType... types) throws CloudflareApiException { - String endpoint = buildEndpointWithTypeFilters(CfRequest.RECORD_LIST.buildPath(zone.getId()), types); + String endpoint = CfRequest.RECORD_LIST.buildPath(zone.getId()); RecordMultipleResponse resp = getRequest(endpoint, RecordMultipleResponse.class); checkResponse(resp, false); - return resp.getResult(); - } - - private String buildEndpointWithTypeFilters(String baseEndpoint, @Nullable RecordType... types) { - if (types == null || types.length == 0) { - return baseEndpoint; - } - StringBuilder endpoint = new StringBuilder(baseEndpoint); - String separator = baseEndpoint.contains("?") ? "&" : "?"; - for (RecordType type : types) { - endpoint.append(separator).append("type=").append(type); - separator = "&"; - } - return endpoint.toString(); + List recs = resp.getResult(); + return filterAndSetZoneRecords(zone, types, recs); } /** @@ -459,6 +448,24 @@ public class CfDnsClient extends CfBasicHttpClient { return result; } + private List filterAndSetZoneRecords(ZoneEntity zone, @Nullable RecordType[] types, List recs) + throws CloudflareNotFoundException { + List filtered; + if (types != null && types.length > 0) { + filtered = recs.stream().filter(rec -> Arrays.asList(types).contains(RecordType.valueOf(rec.getType()))).collect(Collectors.toList()); + } else { + filtered = new ArrayList<>(recs); + } + filtered.forEach(rec -> rec.setZoneId(zone.getId())); + + // special exception for an empty result, normally it's done in the RecordValidator + if (filtered.isEmpty() && emptyResultThrowsException) { + throw new CloudflareNotFoundException("No records exist after filtering zone: " + zone.getName()); + } + return filtered; + } + + private List cleanRecordsForPostOrPut(List records) { List cleaned = new ArrayList<>(); records.forEach( diff --git a/src/test/java/codes/thischwa/cf/CfClientTest.java b/src/test/java/codes/thischwa/cf/CfClientTest.java index 6e426f3..70b5c28 100644 --- a/src/test/java/codes/thischwa/cf/CfClientTest.java +++ b/src/test/java/codes/thischwa/cf/CfClientTest.java @@ -5,6 +5,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; import static org.junit.jupiter.api.Assumptions.assumeTrue; import codes.thischwa.cf.model.BatchEntry; @@ -47,30 +48,49 @@ public class CfClientTest { @Test void testAddHost() throws Exception { ZoneEntity zone = client.zoneGet(ZONE_STR); - client.recordDeleteTypeIfExists(zone, SLD_STR, RecordType.A, RecordType.AAAA); - RecordEntity record = RecordEntity.build(SLD_STR, RecordType.A, TTL, "127.0.0.1"); - RecordEntity createdRecord = client.recordCreate(zone, record); - assertNotNull(createdRecord.getId()); - assertEquals(SLD_STR, createdRecord.getSld()); - assertEquals(RecordType.A.getType(), createdRecord.getType()); - assertEquals(TTL, createdRecord.getTtl()); - assertEquals("127.0.0.1", createdRecord.getContent()); - assertNotNull(createdRecord.getCreatedOn()); - client.recordDeleteTypeIfExists(zone, SLD_STR, RecordType.A); - assertThrows(CloudflareNotFoundException.class, - () -> client.recordList(zone, SLD_STR, RecordType.A)); + try { + // clean-up + client.recordDeleteTypeIfExists(zone, SLD_STR, RecordType.A, RecordType.AAAA); - record = RecordEntity.build(SLD_STR + "." + ZONE_STR, RecordType.A, TTL, "127.1.0.1"); - createdRecord = client.recordCreate(zone, record); - assertNotNull(createdRecord.getId()); - assertEquals(SLD_STR, createdRecord.getSld()); - assertEquals(RecordType.A.getType(), createdRecord.getType()); - assertEquals(TTL, createdRecord.getTtl()); - assertEquals("127.1.0.1", createdRecord.getContent()); - assertNotNull(createdRecord.getCreatedOn()); + RecordEntity record = RecordEntity.build(SLD_STR, RecordType.A, TTL, "127.0.0.1"); + RecordEntity createdRecord = client.recordCreate(zone, record); + assertNotNull(createdRecord.getId()); + assertEquals(SLD_STR, createdRecord.getSld()); + assertEquals(RecordType.A.getType(), createdRecord.getType()); + assertEquals(TTL, createdRecord.getTtl()); + assertEquals("127.0.0.1", createdRecord.getContent()); + assertNotNull(createdRecord.getCreatedOn()); - client.recordDeleteTypeIfExists(zone, SLD_STR, RecordType.A); + List records = client.recordList(zone, SLD_STR, RecordType.A); + assertEquals(1, records.size()); + RecordEntity fetchedRecord = records.get(0); + assertEquals(createdRecord.getId(), fetchedRecord.getId()); + assertEquals(createdRecord.getContent(), fetchedRecord.getContent()); + assertEquals(createdRecord.getType(), fetchedRecord.getType()); + client.recordDelete(zone, createdRecord); + + // test A and AAAA records for the same SLD + RecordEntity recordA = RecordEntity.build(SLD_STR, RecordType.A, TTL, "127.0.0.2"); + RecordEntity recordAAAA = RecordEntity.build(SLD_STR, RecordType.AAAA, TTL, "2001:db8::1"); + RecordEntity createdRecordA = client.recordCreate(zone, recordA); + RecordEntity createdRecordAAAA = client.recordCreate(zone, recordAAAA); + assertNotNull(createdRecordA.getId()); + assertNotNull(createdRecordAAAA.getId()); + assertEquals(SLD_STR, createdRecordA.getSld()); + assertEquals(SLD_STR, createdRecordAAAA.getSld()); + assertEquals(RecordType.A.getType(), createdRecordA.getType()); + assertEquals(RecordType.AAAA.getType(), createdRecordAAAA.getType()); + + client.recordDeleteTypeIfExists(zone, SLD_STR, RecordType.A, RecordType.AAAA); + assertThrows(CloudflareNotFoundException.class, + () -> client.recordList(zone, SLD_STR, RecordType.A, RecordType.AAAA)); + } finally { + // cleanup in case of failures during test + try { + client.recordDeleteTypeIfExists(zone, SLD_STR, RecordType.A, RecordType.AAAA); + } catch (Exception e) { /* ignore */ } + } } @Test @@ -82,14 +102,6 @@ public class CfClientTest { () -> client.recordList(zList.get(0), "not-existing")); } - @Test - void testEmptyResultThrowsException() throws Exception { - List zList = client.zoneList(); - CfDnsClient client = new CfDnsClient(true, API_EMAIL, API_KEY); - assertThrows(CloudflareNotFoundException.class, - () -> client.recordList(zList.get(0), "not-existing")); - } - @Test void testDns() throws Exception { // starting point: already existing zone 'mein-d-ns.de' @@ -121,7 +133,7 @@ public class CfClientTest { createdRe1 = client.recordCreate(z, RecordEntity.build(domain, RecordType.A, TTL, "130.0.0.3")); assertNotNull(createdRe1.getId()); - assertEquals(randomSld+ "." + ZONE_STR, createdRe1.getName()); + assertEquals(randomSld + "." + ZONE_STR, createdRe1.getName()); assertEquals(randomSld, createdRe1.getSld()); assertEquals(RecordType.A.getType(), createdRe1.getType()); assertEquals(z.getId(), createdRe1.getZoneId()); @@ -156,7 +168,7 @@ public class CfClientTest { } else if (Objects.equals(re.getType(), RecordType.AAAA.getType())) { assertEquals("2a0a:4cc0:c0:2e4::1", re.getContent()); } else { - throw new IllegalStateException("Unexpected record type: " + re.getType()); + fail(String.format("Unexpected record type: %s", re.getType())); } } @@ -165,12 +177,16 @@ public class CfClientTest { assertTrue(fullList.size() >= 2); assertTrue(fullList.stream().anyMatch(re -> re.getId().equals(createdRe1.getId()))); assertTrue(fullList.stream().anyMatch(re -> re.getId().equals(createdRe2.getId()))); + assertTrue( + fullList.stream().allMatch(re -> re.getType().equals(RecordType.A.getType()) || re.getType().equals(RecordType.AAAA.getType()))); // test recordList with types without SLD List aList = client.recordList(z, RecordType.A); assertFalse(aList.isEmpty()); + assertTrue(aList.size() >= 1); assertTrue(aList.stream().anyMatch(re -> re.getId().equals(createdRe1.getId()))); assertTrue(aList.stream().noneMatch(re -> re.getId().equals(createdRe2.getId()))); + assertTrue(aList.stream().allMatch(re -> re.getType().equals(RecordType.A.getType()))); // test fluent api list List fluentList = client.zone(ZONE_STR).list(RecordType.A);