improve error handling and logging by asolntsev · Pull Request #1311 · datafaker-net/datafaker

asolntsev · 2024-07-21T11:44:38Z

This PR doesn't fix #1306, but makes much easier debugging such problems.

what-the-diff · 2024-07-21T11:45:05Z

PR Summary

Type Modification of REGEXP2SUPPLIER_MAP field in FakeValuesService.java
The type of this field was upgraded from Map<RegExpContext, Supplier<?>> to Map<RegExpContext, ValueResolver>. This change helps in supporting advanced map operations without the need of external supplier.
Addition of New Inner Class SafeFetchResolver
An inner class SafeFetchResolver was introduced to help with code readability and organization. This should make maintaining the code easier in the future.
Method resolveExpression Refactoring
This method was optimized to utilize the newly created SafeFetchResolver and MethodResolver classes consequently enhancing performance and overall code sophistication.
Method resolveFromMethodOn Refactoring
The return type of resolveFromMethodOn method was adjusted from Supplier<Object> to ValueResolver, followed by refactoring of the method to use the MethodAndCoercedArgsResolver class. These modifications facilitate improved design architecture and cleaner code.
Addition of Nested Static Classes
Three new nested static classes - ConstantResolver, MethodResolver, and MethodAndCoercedArgsResolver were introduced, adding to the structure and robustness of the overall code. This brings added flexibility and allows the application to be more modular in nature.
Conversion of RegExpContext Class to Record
The transition of RegExpContext class to record simplifies code by automatically creating important methods like equals(), hashCode(), and toString(). This ensures more efficiency and consistency of operations.
Addition of ValueResolver Interface
This interface was implemented to facilitate easier value resolution in the code, providing a simplified way to manage value resolution logic.
Conversion of Classes to Records
Numerous classes, including SafeFetchResolver, ConstantResolver, MethodResolver, and MethodAndCoercedArgsResolver were converted to records for simplified handling and to streamline standard java operations, enhancing efficiency, readability, and maintainability.

This collection of changes overall leans towards refactoring the code for improved readability, maintainability, and performance.

codecov · 2024-07-21T11:48:39Z

Codecov Report

Attention: Patch coverage is 77.38095% with 19 lines in your changes missing coverage. Please review.

Project coverage is 91.97%. Comparing base (74e02a8) to head (5a81bbf).
Report is 8 commits behind head on main.

Files	Patch %	Lines
.../java/net/datafaker/service/FakeValuesService.java	81.66%	9 Missing and 2 partials ⚠️
...in/java/net/datafaker/providers/base/Internet.java	71.42%	4 Missing ⚠️
...rc/main/java/net/datafaker/service/FakeValues.java	0.00%	2 Missing ⚠️
.../main/java/net/datafaker/providers/base/Image.java	0.00%	1 Missing ⚠️
...tafaker/transformations/JavaObjectTransformer.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1311      +/-   ##
============================================
+ Coverage     91.82%   91.97%   +0.14%     
+ Complexity     3159     3152       -7     
============================================
  Files           316      316              
  Lines          6229     6232       +3     
  Branches        633      634       +1     
============================================
+ Hits           5720     5732      +12     
+ Misses          353      338      -15     
- Partials        156      162       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bodiam · 2024-07-21T23:50:21Z

src/main/java/net/datafaker/providers/base/BaseFaker.java

    }
+
+    @Override
+    public String toString() {


isn't this the default toString?

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...

but why do you need it?

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

bodiam · 2024-07-21T23:51:26Z

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

        } catch (IOException e) {
-            e.printStackTrace();
-            return null;
+            throw new RuntimeException("Failed to generate image %s of size %sx%s".formatted(imageType, width, height), e);


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

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

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.

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.

bodiam · 2024-07-21T23:52:11Z

src/main/java/net/datafaker/providers/base/Internet.java

-        } catch (UnknownHostException e) {
-            return "127.0.0.1";
-        }
+        return getIpV4Address().getHostAddress();


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

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

bodiam · 2024-07-21T23:52:30Z

src/main/java/net/datafaker/providers/base/Internet.java

-        } catch (UnknownHostException e) {
-            return "127.0.0.1";
-        }
+        return getPrivateIpV4Address().getHostAddress();


again, I don't think this is correct.

same as above: it's correct :)

bodiam · 2024-07-21T23:52:48Z

src/main/java/net/datafaker/providers/base/Internet.java

-        } catch (UnknownHostException e) {
-            return "0:0:0:0:0:0:0:1";
-        }
+        return getIpV6Address().getHostAddress();


same as above.

same as above. :)

asolntsev · 2024-08-22T19:25:47Z

@bodiam Let me summarize: I am pretty sure it's a good PR and should be merged. ;)

bodiam · 2024-08-22T22:59:03Z

Then feel free to merge it! :)

…olver) it allows to log what objects were returned when debugging flaky test failures.

... to make it easier to read these values in logs

... 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.

…fore() It was WTF moment when I realized that method BaseFakerTest.before() resets all logger levels to INFO. It took few hours to find out why my logging configuration doesn't work. :(

* by default, FINE logs are NOT written to console, BUT * if some test failed, all its FINE logs are written to console. Thus we can investigate flaky tests.

there is no need to initialize static Faker's vars in some specific way. They should stably work in any order.

asolntsev marked this pull request as draft July 21, 2024 11:44

asolntsev linked an issue Jul 21, 2024 that may be closed by this pull request

Unable to resolve expression during build #1306

Closed

asolntsev self-assigned this Jul 21, 2024

asolntsev force-pushed the chore/1306-improve-logging branch 2 times, most recently from 604b3b3 to 58aa282 Compare July 21, 2024 15:26

asolntsev marked this pull request as ready for review July 21, 2024 15:32

asolntsev mentioned this pull request Jul 21, 2024

Unable to resolve expression during build #1306

Closed

asolntsev changed the title ~~improve logging (useful for debugging flaky tests etc.)~~ improve error handling and logging Jul 21, 2024

bodiam reviewed Jul 21, 2024

View reviewed changes

asolntsev requested a review from bodiam July 22, 2024 09:33

asolntsev added 7 commits August 23, 2024 09:34

#1306 replace anonymous lambdas (Supplier) by named classes (ValueRes…

08cede7

…olver) it allows to log what objects were returned when debugging flaky test failures.

#1306 add readable toString() methods to some objects

d7e9ffb

... to make it easier to read these values in logs

#1306 Remove unneeded logging level configuration in BaseFakerTest.be…

9df62a1

…fore() It was WTF moment when I realized that method BaseFakerTest.before() resets all logger levels to INFO. It took few hours to find out why my logging configuration doesn't work. :(

#1306 setup FINE logging in tests

19e5cd1

* by default, FINE logs are NOT written to console, BUT * if some test failed, all its FINE logs are written to console. Thus we can investigate flaky tests.

#1306 add debug logs for FakerIntegrationTest

808d796

#1306 Remove unneeded hack in BaseFakerTest

5a81bbf

there is no need to initialize static Faker's vars in some specific way. They should stably work in any order.

asolntsev force-pushed the chore/1306-improve-logging branch from b01d682 to 5a81bbf Compare August 23, 2024 06:35

asolntsev merged commit 1f346c7 into main Aug 23, 2024

asolntsev deleted the chore/1306-improve-logging branch August 23, 2024 08:00

Conversation

asolntsev commented Jul 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

what-the-diff bot commented Jul 21, 2024

PR Summary

Uh oh!

codecov bot commented Jul 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asolntsev Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bodiam Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asolntsev commented Aug 22, 2024

Uh oh!

bodiam commented Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

asolntsev commented Jul 21, 2024 •

edited

Loading

codecov bot commented Jul 21, 2024 •

edited

Loading

asolntsev Jul 22, 2024 •

edited

Loading

bodiam Jul 22, 2024 •

edited

Loading