Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

improve error handling and logging #1311

Merged
merged 7 commits into from
Aug 23, 2024
Merged

improve error handling and logging #1311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
08cede7
#1306 replace anonymous lambdas (Supplier) by named classes (ValueRes…
asolntsev Jul 21, 2024
d7e9ffb
#1306 add readable toString() methods to some objects
asolntsev Jul 21, 2024
8e6ffc3
#1306 throw exception instead of silently logging it
asolntsev Jul 21, 2024
9df62a1
#1306 Remove unneeded logging level configuration in BaseFakerTest.be…
asolntsev Jul 21, 2024
19e5cd1
#1306 setup FINE logging in tests
asolntsev Jul 21, 2024
808d796
#1306 add debug logs for FakerIntegrationTest
asolntsev Jul 21, 2024
5a81bbf
#1306 Remove unneeded hack in BaseFakerTest
asolntsev Jul 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/main/java/net/datafaker/internal/helper/FakerIDN.java
View file
Open in desktop
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
5 changes: 5 additions & 0 deletions src/main/java/net/datafaker/providers/base/AbstractProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,9 @@ protected final <G> List<G> loadGenerators(Class<G> generatorClass) {
.map(ServiceLoader.Provider::get)
.toList();
}

@Override
public String toString() {
return "%s(%s)@%s".formatted(getClass().getSimpleName(), faker, Integer.toHexString(hashCode()));
}
}
2 changes: 1 addition & 1 deletion src/main/java/net/datafaker/providers/base/Address.java
View file
Open in desktop
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
5 changes: 5 additions & 0 deletions src/main/java/net/datafaker/providers/base/BaseFaker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,4 +400,9 @@ public final <B extends ProviderRegistration> B getFaker() {
public static Method getMethod(AbstractProvider<?> ap, String methodName) {
return ap == null ? null : ObjectMethods.getMethodByName(ap, methodName);
}

@Override
public String toString() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this the default toString?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost. Except that it's shorter due to getSimpleName instead of getName.
I would like to make it even more shorter, but it doesn't have any human-readable fields...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but why do you need it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In debug logs.
To make output of such logs a bit better readable (than the default):

    LOG.fine(() -> "resExp(%s [%s]) current: %s, root: %s, context: %s, regExpContext: %s -> res: %s".formatted(directive, Arrays.toString(args), current, root, context, regExpContext, res));

return getClass().getSimpleName() + "@" + Integer.toHexString(hashCode());
}
}
3 changes: 1 addition & 2 deletions src/main/java/net/datafaker/providers/base/Image.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,7 @@
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);

Check warning on line 161 in src/main/java/net/datafaker/providers/base/Image.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/net/datafaker/providers/base/Image.java#L161 

Added line #L161 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why you'd want to throw an exception here.

Copy link
Collaborator Author

@asolntsev asolntsev Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's much better to clearly show the problem instead of hiding it.

  • When you return null, it almost always causes NullPointerException in the next step. And user will not understand what happened. For example, in DF own test:
        assertThat(faker.image().base64TIFF()).startsWith("data:image/tiff;base64,");
  • If user wants to protect, he will need to write IF:
String image = faker.image().base64TIFF();
if (image == null) {
  // WHAT SHOULD I DO HERE?..
  throw new RuntimeException("WTF, the image is null, and I have no idea why.");
}

Here is more about "Throw Early, Catch Late" Principle

Copy link
Contributor

@bodiam bodiam Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is writing that if(null) check different from:

try {
   String image = faker.image().base64TIFF();
} catch(Exception e) {
  throw new RuntimeException("WTF, the image generation failed");
}

I don't mind throwing exceptions early if the code is in my control, but this is a library, and how would a user know this code can throw an exception?

The implicit contract in DF is that none of the methods will ever throw an exception,this changes that approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It differs a lot.

  • User doesn't need to check if image is null.
  • User doesn't need to pollute his code with unneeded IFs and try/catches.
  • User will see a clear error message explaining WHY the image could not be generated.

The implicit contract in DF is that none of the methods will ever throw an exception

It sounds very strange to me.
Nobody can ever guarantee that his code never throws any exceptions.

Anyway, in this case, such an exception actually cannot happen. We need to catch IOException only because java interfaces like InputStream declare throws IOException. But in this case we operate with ByteArrayOutputStream which never throws IOException.

}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is ideal, I believe the code can throw exceptions now sometimes, and I don't think we should.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also had a doubt, but no, this code doesn't fail.

This is a test that tries through all possible values:

    @Test
    void allPossibleAddresses() {
        for (int first = 2; first < 256; first++) {
            for (int second = 2; second < 256; second++) {
                for (int third = 2; third < 256; third++) {
                    for (int fourth = 2; fourth < 256; fourth++) {
                        InetAddress inetAddress = inet4Address((byte) first, (byte) second, (byte) third, (byte) fourth);
                        assertThat(inetAddress).isNotNull();
                    }
                }
            }
        }
    }

}

/**
* 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, I don't think this is correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above: it's correct :)

}

/**
* @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 @@
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 @@
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 @@
* @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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above. :)

}

/**
* <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 @@
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 @@
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);

Check warning on line 538 in src/main/java/net/datafaker/providers/base/Internet.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/net/datafaker/providers/base/Internet.java#L537-L538 

Added lines #L537 - L538 were not covered by tests
}
}

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);

Check warning on line 546 in src/main/java/net/datafaker/providers/base/Internet.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/net/datafaker/providers/base/Internet.java#L545-L546 

Added lines #L545 - L546 were not covered by tests
}
}
}
2 changes: 0 additions & 2 deletions src/main/java/net/datafaker/providers/base/Twitter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import java.util.ArrayList;
import java.util.Date;
import java.util.UUID;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
Expand Down Expand Up @@ -58,7 +57,6 @@ public Date createdTime(boolean forward, Date base, Date constraints) {
*/
public String twitterId(int expectedLength) {
if (expectedLength <= 6 || expectedLength >= 25) {
LOGGER.setLevel(Level.WARNING);
LOGGER.warning("expectedLength <= 6 may easily cause twitter id collision. And expectedLength >= 25" +
" can be easily out of bound.");
}
Expand Down
10 changes: 2 additions & 8 deletions src/main/java/net/datafaker/service/FakeValues.java
View file
Open in desktop
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 @@
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);

Check warning on line 74 in src/main/java/net/datafaker/service/FakeValues.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/net/datafaker/service/FakeValues.java#L74 

Added line #L74 was not covered by tests
}
return null;
}

private Map<String, Object> loadValues() {
Expand All @@ -102,13 +98,11 @@
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);

Check warning on line 105 in src/main/java/net/datafaker/service/FakeValues.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/net/datafaker/service/FakeValues.java#L105 

Added line #L105 was not covered by tests
}
if (result != null) {
return result;
Expand Down
Loading