Skip to content

Commit

Permalink
#1306 throw exception instead of silently logging it
Browse files Browse the repository at this point in the history
... especially with FINE level (== DEBUG).

This pattern causes flaky failures that are very hard to debug.
If something went wrong, it's always safer to throw it with a clear description of what happened.

See "Throw early, catch late" principle.
  • Loading branch information
asolntsev committed Jul 21, 2024
1 parent b8071b5 commit 0773642
Show file tree
Hide file tree
Showing 14 changed files with 101 additions and 141 deletions.
4 changes: 2 additions & 2 deletions src/main/java/net/datafaker/internal/helper/FakerIDN.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@ public class FakerIDN {
public static String toASCII(String in) {
try {
return IDN.toASCII(in);
} catch (IllegalArgumentException e) {
} catch (IllegalArgumentException ignore) {
// let's continue with the character by character encoding hack.
}
final StringBuilder asciiResult = new StringBuilder();
for (int i = 0; i < in.length(); i++) {
try {
asciiResult.append(IDN.toASCII(in.substring(i, i + 1)));
} catch (Exception ignored) {
} catch (IllegalArgumentException ignored) {
}
}
if (asciiResult.isEmpty()) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/net/datafaker/providers/base/Address.java
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public String zipCodeByState(String stateAbbr) {
}

public String countyByZipCode(String postCode) {
return resolve("address.county_by_postcode." + postCode, () -> "County are not configured for postcode " + postCode);
return resolve("address.county_by_postcode." + postCode, () -> "County is not configured for postcode " + postCode);
}

public String streetSuffix() {
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/net/datafaker/providers/base/Image.java
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,7 @@ private String generateBase64RasterImage(ImageType imageType, int width, int hei
byte[] imageBytes = baos.toByteArray();
return "data:" + imageType.mimeType + ";base64," + Base64.getEncoder().encodeToString(imageBytes);
} catch (IOException e) {
e.printStackTrace();
return null;
throw new RuntimeException("Failed to generate image %s of size %sx%s".formatted(imageType, width, height), e);
}
}

Expand Down
60 changes: 28 additions & 32 deletions src/main/java/net/datafaker/providers/base/Internet.java
Original file line number Diff line number Diff line change
Expand Up @@ -246,41 +246,29 @@ public String macAddress() {
* @return a correctly formatted IPv4 address.
*/
public String ipV4Address() {
try {
return getIpV4Address().getHostAddress();
} catch (UnknownHostException e) {
return "127.0.0.1";
}
return getIpV4Address().getHostAddress();
}

/**
* returns an IPv4 address.
*
* @return an IPv4 address.
*/
public InetAddress getIpV4Address() throws UnknownHostException {
return Inet4Address.getByAddress(new byte[]{
(byte) (faker.random().nextInt(254) + 2),
(byte) (faker.random().nextInt(254) + 2),
(byte) (faker.random().nextInt(254) + 2),
(byte) (faker.random().nextInt(254) + 2)});
public InetAddress getIpV4Address() {
return inet4Address((byte) (faker.random().nextInt(254) + 2), (byte) (faker.random().nextInt(254) + 2), (byte) (faker.random().nextInt(254) + 2), (byte) (faker.random().nextInt(254) + 2));
}

/**
* @return a valid private IPV4 address in dot notation
*/
public String privateIpV4Address() {
try {
return getPrivateIpV4Address().getHostAddress();
} catch (UnknownHostException e) {
return "127.0.0.1";
}
return getPrivateIpV4Address().getHostAddress();
}

/**
* @return a private IPV4 address
*/
public InetAddress getPrivateIpV4Address() throws UnknownHostException {
public InetAddress getPrivateIpV4Address() {
final Byte[] PRIVATE_FIRST_OCTET = {10, 127, (byte) 169, (byte) 192, (byte) 172};
final Byte[] PRIVATE_SECOND_OCTET_172 = {16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31};

Expand All @@ -295,24 +283,20 @@ public InetAddress getPrivateIpV4Address() throws UnknownHostException {
case (byte) 192 -> second = (byte) 168;
case (byte) 169 -> second = (byte) 254;
}
return Inet4Address.getByAddress(new byte[]{first, second, third, fourth});
return inet4Address(first, second, third, fourth);
}

/**
* @return a valid public IPV4 address in dot notation
*/
public String publicIpV4Address() {
try {
return getPublicIpV4Address().getHostAddress();
} catch (UnknownHostException e) {
return "127.0.0.1";
}
return getPublicIpV4Address().getHostAddress();
}

/**
* @return a valid public IPV4 address
*/
public InetAddress getPublicIpV4Address() throws UnknownHostException {
public InetAddress getPublicIpV4Address() {
final RandomService r = faker.random();

final byte[] PRIVATE_FIRST_OCTET = {10, 127, (byte) 169, (byte) 192, (byte) 172};
Expand All @@ -325,7 +309,7 @@ public InetAddress getPublicIpV4Address() throws UnknownHostException {
while (Arrays.binarySearch(PRIVATE_FIRST_OCTET, first) > 0) {
first = (byte) r.nextInt(256);
}
return Inet4Address.getByAddress(new byte[]{first, second, third, fourth});
return inet4Address(first, second, third, fourth);
}

/**
Expand All @@ -343,19 +327,15 @@ public String ipV4Cidr() {
* @return a correctly formatted IPv6 address.
*/
public String ipV6Address() {
try {
return getIpV6Address().getHostAddress();
} catch (UnknownHostException e) {
return "0:0:0:0:0:0:0:1";
}
return getIpV6Address().getHostAddress();
}

/**
* <p>Returns an IPv6 address in hh:hh:hh:hh:hh:hh:hh:hh format.</p>
*
* @return a IPV6 address.
*/
public InetAddress getIpV6Address() throws UnknownHostException {
public InetAddress getIpV6Address() {
final RandomService random = faker.random();
final char[] res = new char[4 * 8 + 7];
for (int i = 0; i < 8; i++) {
Expand All @@ -366,7 +346,7 @@ public InetAddress getIpV6Address() throws UnknownHostException {
char[] hex = random.hex(4, false).toCharArray();
System.arraycopy(hex, 0, res, i + offset, hex.length);
}
return Inet6Address.getByName(String.valueOf(res));
return inet6Address(String.valueOf(res));
}

/**
Expand Down Expand Up @@ -550,4 +530,20 @@ public String toString() {
return browserName;
}
}

private static InetAddress inet4Address(byte first, byte second, byte third, byte fourth) {
try {
return Inet4Address.getByAddress(new byte[]{first, second, third, fourth});
} catch (UnknownHostException e) {
throw new RuntimeException("Failed to create Inet4Address from %s %s %s %s".formatted(first, second, third, fourth), e);
}
}

private static InetAddress inet6Address(String host) {
try {
return Inet6Address.getByName(host);
} catch (UnknownHostException e) {
throw new RuntimeException("Failed to create Inet6Address from host '%s'".formatted(host), e);
}
}
}
10 changes: 2 additions & 8 deletions src/main/java/net/datafaker/service/FakeValues.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,8 @@
import java.util.Set;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.logging.Level;
import java.util.logging.Logger;

public class FakeValues implements FakeValuesInterface {
private static final Logger LOG = Logger.getLogger("faker");
private static final Map<FakeValuesContext, FakeValues> FAKE_VALUES_MAP = new HashMap<>();
private final FakeValuesContext fakeValuesContext;
private volatile Map<String, Object> values;
Expand Down Expand Up @@ -74,9 +71,8 @@ private Map<String, Object> loadFromUrl() {
try (InputStream stream = url.openStream()) {
return readFromStream(stream);
} catch (IOException e) {
LOG.log(Level.SEVERE, "Exception: ", e);
throw new RuntimeException("Failed to read fake values from %s".formatted(url), e);
}
return null;
}

private Map<String, Object> loadValues() {
Expand All @@ -102,13 +98,11 @@ private Map<String, Object> loadValues() {
try (InputStream stream2 = getClass().getClassLoader().getResourceAsStream(path)) {
result = readFromStream(stream2);
enrichMapWithJavaNames(result);
} catch (Exception e) {
LOG.log(Level.SEVERE, "Exception: ", e);
}
}

} catch (IOException e) {
LOG.log(Level.SEVERE, "Exception: ", e);
throw new RuntimeException("Failed to read fake values from %s".formatted(path), e);
}
if (result != null) {
return result;
Expand Down
37 changes: 19 additions & 18 deletions src/main/java/net/datafaker/service/FakeValuesService.java
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
import com.github.curiousoddman.rgxgen.RgxGen;

import static java.util.Objects.requireNonNull;
import static java.util.logging.Level.FINE;
import static java.util.logging.Level.SEVERE;
import static net.datafaker.transformations.Field.field;

public class FakeValuesService {
Expand Down Expand Up @@ -947,22 +949,15 @@ private ValueResolver resolveFromMethodOn(Object obj, String directive, String[]
if (obj == null) {
return null;
}
try {
final MethodAndCoercedArgs accessor = retrieveMethodAccessor(obj, directive, args);
return (accessor == null)
? NULL_VALUE
: new MethodAndCoercedArgsResolver(accessor, obj);
} catch (Exception e) {
LOG.log(Level.FINE, "Can't call " + directive + " on " + obj, e);
return NULL_VALUE;
}
final MethodAndCoercedArgs accessor = retrieveMethodAccessor(obj, directive, args);
return accessor == null ? NULL_VALUE : new MethodAndCoercedArgsResolver(accessor, obj);
}

/**
* Accepts a {@link BaseFaker} instance and a name.firstName style 'key' which is resolved to the return value of:
* {@link BaseFaker#name()}'s {@link Name#firstName()} method.
*
* @throws RuntimeException if there's a problem invoking the method or it doesn't exist.
* @throws RuntimeException if there's a problem invoking the method, or it doesn't exist.
*/
private ValueResolver resolveFakerObjectAndMethod(ProviderRegistration faker, String key, int dotIndex, String[] args) {
final String[] classAndMethod;
Expand All @@ -985,10 +980,11 @@ private ValueResolver resolveFakerObjectAndMethod(ProviderRegistration faker, St
if (accessor == null) {
return NULL_VALUE;
}

return new MethodAndCoercedArgsResolver(accessor, objectWithMethodToInvoke);
} catch (Exception e) {
LOG.log(Level.SEVERE, e.getMessage(), e);
return NULL_VALUE;
} catch (InvocationTargetException | IllegalAccessException e) {
throw new RuntimeException("Failed to resolve faker object and method for %s (dotIndex=%s, args=%s)"
.formatted(key, dotIndex, Arrays.toString(args)), e);
}
}

Expand Down Expand Up @@ -1020,7 +1016,7 @@ private MethodAndCoercedArgs retrieveMethodAccessor(Object object, String method
*/
private MethodAndCoercedArgs accessor(Class<?> clazz, String name, String[] args) {
final String finalName = name;
LOG.log(Level.FINE, () -> "Find accessor named " + finalName + " on " + clazz.getSimpleName() + " with args " + Arrays.toString(args));
LOG.fine(() -> "Find accessor named " + finalName + " on " + clazz.getSimpleName() + " with args " + Arrays.toString(args));
name = removeUnderscoreChars(name);
final Collection<Method> methods;
if (CLASS_2_METHODS_CACHE.containsKey(clazz)) {
Expand Down Expand Up @@ -1155,7 +1151,9 @@ private Object[] coerceArguments(Method accessor, String[] args) {
}
coerced[i] = coercedArgument;
} catch (Exception e) {
LOG.fine("Unable to coerce " + args[i] + " to " + toType.getSimpleName() + " via " + toType.getSimpleName() + "(String) constructor.");
Throwable cause = unwrap(e);
Level level = cause instanceof IllegalArgumentException || cause instanceof NoSuchMethodException ? FINE : SEVERE;
LOG.log(level, "Unable to coerce " + args[i] + " to " + toType.getSimpleName() + " via " + toType.getSimpleName() + "(String) constructor.", e);
return null;
}
}
Expand Down Expand Up @@ -1249,10 +1247,13 @@ public Object resolve() {
private static Object invokeAndToString(MethodAndCoercedArgs accessor, Object objectWithMethodToInvoke) {
try {
return accessor.invoke(objectWithMethodToInvoke);
} catch (Exception e) {
LOG.log(Level.FINE, e.getMessage(), e);
return null;
} catch (InvocationTargetException | IllegalAccessException e) {
throw new RuntimeException("Failed to invoke %s on %s".formatted(accessor, objectWithMethodToInvoke), unwrap(e));
}
}
}

private static Throwable unwrap(Throwable e) {
return e instanceof InvocationTargetException reflection ? unwrap(reflection.getTargetException()) : e;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@ public Object apply(Object input, Schema<Object, ?> schema) {
CLASS2CONSTRUCTOR.put(clazz, primaryConstructor);
}
result = primaryConstructor.newInstance();
} catch (InstantiationException | IllegalAccessException |
InvocationTargetException e) {
} catch (InstantiationException | IllegalAccessException | InvocationTargetException e) {
throw new RuntimeException(e);
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/test/java/net/datafaker/Issue759Test.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ public static void fakeSomeData(Faker faker) {
try {
String county = faker.address().countyByZipCode(zipCode);
assertThat(county).isNotEqualTo(zipCode);
} catch (Exception ignore) {
} catch (RuntimeException expected) {
assertThat(expected).hasMessageStartingWith("County is not configured for postcode " + zipCode);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

import java.time.LocalDate;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeParseException;
import java.util.Locale;
import java.util.regex.Pattern;

Expand All @@ -23,12 +22,10 @@ void testValidKoKrRrn() {

// Check if contains other character than digit
assertThat(rrn).matches(D_6_D_7);
try {
// Check date
LocalDate.parse(rrn.substring(0, 6), YYMMDD);
} catch (DateTimeParseException e) {
assertThat(e).isNull();
}
// Check date
LocalDate date = LocalDate.parse(rrn.substring(0, 6), YYMMDD);
int currentYear = LocalDate.now().getYear();
assertThat(date.getYear()).isBetween(currentYear - 80, currentYear + 80);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Set;
import java.util.stream.Stream;

import static java.lang.Thread.currentThread;
import static org.apache.commons.lang3.StringUtils.substringBefore;
import static org.assertj.core.api.Assertions.assertThat;
import static org.reflections.ReflectionUtils.getAllMethods;
Expand Down Expand Up @@ -112,7 +113,8 @@ private void testAllMethodsThatReturnStringsActuallyReturnStrings(AbstractProvid
try {
returnValue = method.invoke(provider);
} catch (Exception e) {
throw new RuntimeException("Test for method " + method + " and object " + provider + " was failed for locale " + locale, e);
throw new RuntimeException("Test for method %s and object %s was failed for locale %s [thread: %s]".formatted(
method, provider, locale, currentThread().getName()), e);
}
assertThat(returnValue).as("For method " + provider.getClass() + "#" + method.getName() + "value is '" + returnValue + "'").isInstanceOf(String.class);
final String returnValueAsString = (String) returnValue;
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/net/datafaker/providers/base/AddressTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class AddressTest extends BaseFakerTest<BaseFaker> {
private static final Condition<String> IS_A_NUMBER = new Condition<>(s -> {
try {
Double.valueOf(s);
} catch (NumberFormatException nfe) {
} catch (NumberFormatException ignore) {
return false;
}
return true;
Expand Down Expand Up @@ -184,7 +184,7 @@ void testCountyForWrongZipCode(String zipCode) {
final BaseFaker localFaker = new BaseFaker(new Locale("en", "US"));
assertThatThrownBy(() -> localFaker.address().countyByZipCode(zipCode))
.isInstanceOf(RuntimeException.class)
.hasMessage("County are not configured for postcode " + zipCode);
.hasMessage("County is not configured for postcode " + zipCode);
}

@Test
Expand Down
Loading

0 comments on commit 0773642

Please sign in to comment.