Skip to content

Commit f967598

Browse files
claudeclaude
committed
Fix critical code quality issues and strengthen test coverage
- Replace deprecated CAMEL_CASE_TO_LOWER_CASE_WITH_UNDERSCORES with SNAKE_CASE - Fix ExecutionException unwrapping in CachingInternalClient to preserve root cause - Specify UTF-8 charset explicitly in ApiKeyRequestInterceptor - Throw IllegalArgumentException instead of returning null from getFields() with empty fields - Scope feign-httpclient to test (production code only uses feign.Client) - Scope lombok to provided (compile-time annotation processor only) - Remove redundant bulkAsArray() method from IpdataService - Add Javadoc to IpdataService public API - Remove package-private setters from IpdataModel (Jackson uses field access) - Fix testSingleFields to use fixture IP instead of hardcoded 8.8.8.8 - Strengthen testError to assert RemoteIpdataException with 401 status - Add EdgeCaseTest with negative tests for empty fields and exception unwrapping https://claude.ai/code/session_01KxvyXRVVZaLrgTZshvsZY6
1 parent 7473634 commit f967598

File tree

10 files changed

+145
-34
lines changed

10 files changed

+145
-34
lines changed

‎pom.xml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
<groupId>io.github.openfeign</groupId>
5959
<artifactId>feign-httpclient</artifactId>
6060
<version>${version.client.feign}</version>
61+
<scope>test</scope>
6162
</dependency>
6263
<dependency>
6364
<groupId>com.google.guava</groupId>
@@ -79,6 +80,7 @@
7980
<groupId>org.projectlombok</groupId>
8081
<artifactId>lombok</artifactId>
8182
<version>1.18.38</version>
83+
<scope>provided</scope>
8284
</dependency>
8385
<dependency>
8486
<groupId>org.hamcrest</groupId>

‎src/main/java/io/ipdata/client/model/IpdataModel.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,12 @@
11
package io.ipdata.client.model;
22

33
import com.fasterxml.jackson.annotation.JsonProperty;
4-
import lombok.AccessLevel;
54
import lombok.Getter;
6-
import lombok.Setter;
75
import lombok.ToString;
86
import lombok.experimental.Accessors;
97

108
import java.util.List;
119

12-
@Setter(AccessLevel.PACKAGE)
1310
@ToString
1411
@Getter
1512
@Accessors(fluent = true)

‎src/main/java/io/ipdata/client/service/ApiKeyRequestInterceptor.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import lombok.extern.slf4j.Slf4j;
88

99
import java.io.InputStreamReader;
10+
import java.nio.charset.StandardCharsets;
1011

1112
@RequiredArgsConstructor
1213
@Slf4j
@@ -22,7 +23,7 @@ class ApiKeyRequestInterceptor implements RequestInterceptor {
2223
String version;
2324
try {
2425
version = CharStreams.toString(new InputStreamReader(ApiKeyRequestInterceptor.class
25-
.getResourceAsStream("/io/ipdata/client/VERSION"))).replaceAll("\\n", "");
26+
.getResourceAsStream("/io/ipdata/client/VERSION"), StandardCharsets.UTF_8)).replaceAll("\\n", "");
2627
version = String.format("io.ipdata.client.java.%s", version);
2728
} catch (Exception e) {
2829
log.error(e.getMessage(), e);

‎src/main/java/io/ipdata/client/service/CachingInternalClient.java‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,20 @@ class CachingInternalClient implements IpdataInternalClient, IpdataInternalSingl
2929
private final LoadingCache<String, Currency> currencyCache;
3030
private final LoadingCache<String, ThreatModel> threatCache;
3131

32+
private static IpdataException unwrap(ExecutionException e) throws IpdataException {
33+
Throwable cause = e.getCause();
34+
if (cause instanceof IpdataException) {
35+
throw (IpdataException) cause;
36+
}
37+
throw new IpdataException(cause != null ? cause.getMessage() : e.getMessage(), cause != null ? cause : e);
38+
}
39+
3240
@Override
3341
public IpdataModel getFields(String ip, String fields) throws IpdataException {
3442
try {
3543
return fieldsCache.get(HashPair.of(ip, fields));
3644
} catch (ExecutionException e) {
37-
throw new IpdataException(e.getMessage(), e);
45+
throw unwrap(e);
3846
}
3947
}
4048

@@ -43,7 +51,7 @@ public AsnModel asn(String ip) throws IpdataException {
4351
try {
4452
return asnCache.get(ip);
4553
} catch (ExecutionException e) {
46-
throw new IpdataException(e.getMessage(), e);
54+
throw unwrap(e);
4755
}
4856
}
4957

@@ -52,7 +60,7 @@ public TimeZone timeZone(String ip) throws IpdataException {
5260
try {
5361
return tzCache.get(ip);
5462
} catch (ExecutionException e) {
55-
throw new IpdataException(e.getMessage(), e);
63+
throw unwrap(e);
5664
}
5765
}
5866

@@ -61,7 +69,7 @@ public Currency currency(String ip) throws IpdataException {
6169
try {
6270
return currencyCache.get(ip);
6371
} catch (ExecutionException e) {
64-
throw new IpdataException(e.getMessage(), e);
72+
throw unwrap(e);
6573
}
6674
}
6775

@@ -70,7 +78,7 @@ public ThreatModel threat(String ip) throws IpdataException {
7078
try {
7179
return threatCache.get(ip);
7280
} catch (ExecutionException e) {
73-
throw new IpdataException(e.getMessage(), e);
81+
throw unwrap(e);
7482
}
7583
}
7684

@@ -79,7 +87,7 @@ public IpdataModel ipdata(String ip) throws IpdataException {
7987
try {
8088
return ipdataCache.get(ip);
8189
} catch (ExecutionException e) {
82-
throw new IpdataException(e.getMessage(), e);
90+
throw unwrap(e);
8391
}
8492
}
8593

‎src/main/java/io/ipdata/client/service/IpdataService.java‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,48 @@
55

66
import java.util.List;
77

8+
/**
9+
* Primary interface for accessing the ipdata.co API.
10+
* <p>
11+
* Provides methods for looking up geolocation, threat intelligence, and other
12+
* metadata for IP addresses. Supports single IP lookups, bulk lookups, and
13+
* selective field retrieval.
14+
* <p>
15+
* Also exposes single-field accessors (e.g. {@code getCountryName}, {@code getCity})
16+
* inherited from {@link IpdataInternalSingleFieldClient}.
17+
*
18+
* @see io.ipdata.client.Ipdata#builder()
19+
*/
820
public interface IpdataService extends IpdataInternalSingleFieldClient {
921

22+
/**
23+
* Retrieves the full IP data model for the given IP address.
24+
*
25+
* @param ip an IPv4 or IPv6 address
26+
* @return the full geolocation and metadata for the IP
27+
* @throws IpdataException if the API call fails
28+
*/
1029
IpdataModel ipdata(String ip) throws IpdataException;
1130

31+
/**
32+
* Retrieves IP data for multiple IP addresses in a single request.
33+
*
34+
* @param ips list of IPv4 or IPv6 addresses
35+
* @return a list of IP data models, one per input address
36+
* @throws IpdataException if the API call fails
37+
*/
1238
List<IpdataModel> bulk(List<String> ips) throws IpdataException;
1339

14-
IpdataModel[] bulkAsArray(List<String> ips) throws IpdataException;
15-
40+
/**
41+
* Retrieves only the specified fields for the given IP address.
42+
* <p>
43+
* Fields are sorted before querying to maximize cache hit rates when caching is enabled.
44+
*
45+
* @param ip an IPv4 or IPv6 address
46+
* @param fields one or more fields to retrieve (e.g. {@code IpdataField.ASN}, {@code IpdataField.CURRENCY})
47+
* @return a partial IP data model containing only the requested fields
48+
* @throws IpdataException if the API call fails
49+
* @throws IllegalArgumentException if no fields are specified
50+
*/
1651
IpdataModel getFields(String ip, IpdataField<?>... fields) throws IpdataException;
1752
}

‎src/main/java/io/ipdata/client/service/IpdataServiceBuilder.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818

1919
import java.net.URL;
2020

21-
import static com.fasterxml.jackson.databind.PropertyNamingStrategy.CAMEL_CASE_TO_LOWER_CASE_WITH_UNDERSCORES;
21+
import static com.fasterxml.jackson.databind.PropertyNamingStrategies.SNAKE_CASE;
2222

2323
@RequiredArgsConstructor(staticName = "of")
2424
public class IpdataServiceBuilder {
@@ -31,7 +31,7 @@ public class IpdataServiceBuilder {
3131

3232
public IpdataService build() {
3333
final ObjectMapper mapper = new ObjectMapper();
34-
mapper.setPropertyNamingStrategy(CAMEL_CASE_TO_LOWER_CASE_WITH_UNDERSCORES);
34+
mapper.setPropertyNamingStrategy(SNAKE_CASE);
3535
mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
3636
mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
3737
mapper.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY);

‎src/main/java/io/ipdata/client/service/IpdataServiceSupport.java‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,15 +30,10 @@ private IpdataInternalSingleFieldClient getApi() {
3030
return singleFieldClient;
3131
}
3232

33-
@Override
34-
public IpdataModel[] bulkAsArray(List<String> ips) throws IpdataException {
35-
return bulk(ips).toArray(new IpdataModel[0]);
36-
}
37-
3833
@Override
3934
public IpdataModel getFields(String ip, IpdataField<?>... fields) throws IpdataException {
4035
if (fields.length == 0) {
41-
return null;
36+
throw new IllegalArgumentException("At least one field must be specified");
4237
}
4338
//sorting here to improve the likelihood of a cache hit, otherwise a permutation of the same
4439
//array would result into a different cache key, and thus a cache miss

‎src/test/java/io/ipdata/client/EdgeCaseTest.java‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
package io.ipdata.client;
2+
3+
import io.ipdata.client.error.IpdataException;
4+
import io.ipdata.client.error.RemoteIpdataException;
5+
import io.ipdata.client.model.IpdataModel;
6+
import io.ipdata.client.service.IpdataService;
7+
import lombok.SneakyThrows;
8+
import org.junit.Assert;
9+
import org.junit.Test;
10+
11+
import static io.ipdata.client.service.IpdataField.ASN;
12+
import static io.ipdata.client.service.IpdataField.CURRENCY;
13+
14+
public class EdgeCaseTest {
15+
16+
private static final TestContext TEST_CONTEXT = new TestContext(MockIpdataServer.API_KEY, MockIpdataServer.getInstance().getUrl());
17+
18+
@Test(expected = IllegalArgumentException.class)
19+
@SneakyThrows
20+
public void testGetFieldsWithNoFieldsThrows() {
21+
IpdataService service = TEST_CONTEXT.ipdataService();
22+
service.getFields("8.8.8.8");
23+
}
24+
25+
@Test(expected = IllegalArgumentException.class)
26+
@SneakyThrows
27+
public void testGetFieldsWithNoFieldsThrowsCaching() {
28+
IpdataService service = TEST_CONTEXT.cachingIpdataService();
29+
service.getFields("8.8.8.8");
30+
}
31+
32+
@Test
33+
@SneakyThrows
34+
public void testInvalidKeyReturnsRemoteException() {
35+
IpdataService service = Ipdata.builder()
36+
.url(TEST_CONTEXT.url())
37+
.key("INVALID_KEY")
38+
.noCache()
39+
.get();
40+
try {
41+
service.ipdata("8.8.8.8");
42+
Assert.fail("Expected RemoteIpdataException");
43+
} catch (RemoteIpdataException e) {
44+
Assert.assertEquals(401, e.getStatus());
45+
Assert.assertNotNull(e.getMessage());
46+
}
47+
}
48+
49+
@Test
50+
@SneakyThrows
51+
public void testCachedInvalidKeyUnwrapsException() {
52+
IpdataService service = Ipdata.builder()
53+
.url(TEST_CONTEXT.url())
54+
.key("INVALID_KEY")
55+
.withDefaultCache()
56+
.get();
57+
try {
58+
service.ipdata("8.8.8.8");
59+
Assert.fail("Expected RemoteIpdataException");
60+
} catch (RemoteIpdataException e) {
61+
Assert.assertEquals(401, e.getStatus());
62+
}
63+
}
64+
65+
@Test
66+
@SneakyThrows
67+
public void testGetFieldsReturnsSelectedFields() {
68+
IpdataService service = TEST_CONTEXT.ipdataService();
69+
IpdataModel model = service.getFields("8.8.8.8", ASN, CURRENCY);
70+
Assert.assertNotNull(model.asn());
71+
Assert.assertNotNull(model.currency());
72+
}
73+
}

‎src/test/java/io/ipdata/client/FullModelTest.java‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,15 @@
11
package io.ipdata.client;
22

3-
import feign.httpclient.ApacheHttpClient;
43
import io.ipdata.client.error.IpdataException;
4+
import io.ipdata.client.error.RemoteIpdataException;
55
import io.ipdata.client.model.IpdataModel;
66
import io.ipdata.client.service.IpdataService;
77
import lombok.SneakyThrows;
8-
import org.apache.http.impl.client.HttpClientBuilder;
98
import org.junit.Assert;
109
import org.junit.Test;
1110
import org.junit.runner.RunWith;
1211
import org.junit.runners.Parameterized;
1312

14-
import java.util.concurrent.TimeUnit;
15-
1613

1714
@RunWith(Parameterized.class)
1815
public class FullModelTest {
@@ -44,21 +41,24 @@ public void testFullResponse() {
4441
public void testSingleFields() {
4542
IpdataService ipdataService = fixture.service();
4643
String field = ipdataService.getCountryName(fixture.target());
47-
String expected = TEST_CONTEXT.get("/8.8.8.8/country_name", null);
48-
Assert.assertEquals(field, expected);
44+
String expected = TEST_CONTEXT.get("/" + fixture.target() + "/country_name", null);
45+
Assert.assertEquals(expected, field);
4946
}
5047

5148

5249
@SneakyThrows
53-
@Test(expected = IpdataException.class)
50+
@Test
5451
public void testError() {
5552
IpdataService serviceWithInvalidKey = Ipdata.builder().url(TEST_CONTEXT.url())
5653
.key("THIS_IS_AN_INVALID_KEY")
57-
.withDefaultCache()
58-
.feignClient(new ApacheHttpClient(HttpClientBuilder.create()
59-
.setConnectionTimeToLive(10, TimeUnit.SECONDS)
60-
.build())).get();
61-
serviceWithInvalidKey.ipdata(fixture.target());
54+
.noCache()
55+
.get();
56+
try {
57+
serviceWithInvalidKey.ipdata(fixture.target());
58+
Assert.fail("Expected RemoteIpdataException");
59+
} catch (RemoteIpdataException e) {
60+
Assert.assertEquals(401, e.getStatus());
61+
}
6262
}
6363

6464
@Parameterized.Parameters

‎src/test/java/io/ipdata/client/TestContext.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
import java.net.URL;
2424
import java.util.Map;
2525

26-
import static com.fasterxml.jackson.databind.PropertyNamingStrategy.CAMEL_CASE_TO_LOWER_CASE_WITH_UNDERSCORES;
26+
import static com.fasterxml.jackson.databind.PropertyNamingStrategies.SNAKE_CASE;
2727
import static java.lang.System.getenv;
2828
import static java.util.Arrays.asList;
2929
import static net.javacrumbs.jsonunit.JsonAssert.assertJsonEquals;
@@ -51,7 +51,7 @@ public TestContext(String key, String url) {
5151
this.key = key;
5252
this.url = new URL(url);
5353
mapper = new ObjectMapper();
54-
mapper.setPropertyNamingStrategy(CAMEL_CASE_TO_LOWER_CASE_WITH_UNDERSCORES);
54+
mapper.setPropertyNamingStrategy(SNAKE_CASE);
5555
mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, true);
5656
mapper.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY);
5757
httpClient = HttpClientBuilder.create().build();

0 commit comments

Comments
 (0)