Fix divider in rotated access log for `DefaultAccessLogReceiver` by Pankraz76 · Pull Request #48398 · quarkusio/quarkus

Pankraz76 · 2025-06-15T18:59:43Z

Issue #48342 fix: No divider between base file name and timestamp in the name of rotated access log

tc:

before:
zero

after:

100% (1/1)	100% (11/11)	92% (63/68)	68% (26/38)

�

100% (1/1)	100% (12/12)	98% (71/72)	73% (31/42)

�

50% (1/2)	36% (12/33)	67% (93/138)	60% (48/80)

�

quarkus-bot · 2025-06-15T19:00:01Z

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

title should preferably start with an uppercase character (if it makes sense!)

_{This message is automatically generated by a bot.}

Pankraz76 · 2025-06-15T19:01:53Z

+    private String fixLogBaseName(String logBaseName) {
+        if(logBaseName.endsWith("."))
+        {
+        return logBaseName.substring(0,logBaseName.length()-1);


would this workaround the issue?

@zhangjiangqige
@dmlloyd

We would need a test that proves this

test should fail with old impl. will double check.

testRotatedFileNameFormatWithDot

testMultipleDotsInBaseName

fail with old impl.

removing breaking .dot suffix passes the test with old impl.

To me it seems to be covered.

never the less this feels just like a workaround.

having the prefix might be resulting from somewhere we should actually fix it there.

Imho the issue is still existing, but not prior anymore temporary solution in place.

geoand

Apart from the fact that I don't think the PR fixes the original issue (see relevant comment in the test), I am too keen on seeing a bunch of (AI generated?) tests for each and every method

geoand · 2025-06-17T09:33:33Z

+        assertTrue(Files.list(tempDir)
+                .anyMatch(p -> p.getFileName().toString().matches("test-rotate\\d{4}-\\d{2}-\\d{2}\\.log")));


This indicates to me that the issue is not fixed.

will check again. thanks.

yes looks off.

Expected :"test-rotate2025-06-17.log"
Actual :"test-rotate.2025-06-17.log"

assuming actual is correct.

Pankraz76 · 2025-06-17T09:45:00Z

Apart from the fact that I don't think the PR fixes the original issue (see relevant comment in the test), I am too keen on seeing a bunch of (AI generated?) tests for each and every method

Yes, it's generated and corrected by me.

Having one test with even only one assert is the ideal, so the design would end up the same.

Here it's only about the numbers, as they don't lie—the branch coverage is incremented from zero to more than 50%. So it's covered, providing the security.

The only issue could be redundant tests, which is indeed something to look into afterward.

Except for this, every generated test covering something is better than having nothing, in my opinion.

It's no problem to increment from the AI skeleton.

new tests are good, but I don't think the pR description is accurate

Increase test coverage on MavenProject apache/maven#2462 (review)

geoand · 2025-06-17T10:12:50Z

The only issue could be redundant tests

Unfortunately that's only not exactly true - every piece of code is a maintenance burden for those called upon to maintain it (which only becomes more difficult when the code is not written by those people)

Pankraz76 · 2025-06-17T10:30:43Z

+        receiver.rotate();
+
+        assertThat(Files.list(tempDir)
+                .filter(p -> p.getFileName().toString().matches("test-multi-rotate.\\d{4}-\\d{2}-\\d{2}\\.log"))


Suggested change

.filter(p -> p.getFileName().toString().matches("test-multi-rotate.\\d{4}-\\d{2}-\\d{2}\\.log"))

.filter(p -> p.getFileName().toString().matches("test-multi-rotate\\d{4}-\\d{2}-\\d{2}\\.log"))

this will fail.

Pankraz76 · 2025-06-17T12:07:18Z

maintenance burden

yes nothing comes without cost of carry. I have adjusted completely.

Pankraz76 · 2025-06-18T09:50:39Z

maintenance burden

this is my motivation to reduce burden. im willing to help streamline development.

current setup comes short as chosen plugins insufficient. Considering starts on github its 5k vs 300. Just by following these numbers and seeing the failed effort align imports and prevent unused. Its might tells that the decision made is not justifiable anymore.

Pankraz76 · 2025-06-18T14:09:36Z


    private long changeOverPoint;
    private String currentDateString;
-    private boolean forceLogRotation;


could prevent this with

test coverage of 100% like done in checkstyle.

mutation testing, right?

Pankraz76 · 2025-06-18T14:52:37Z

100% and found our bug so could merge and the fix bug.

wrong branch buts its very much the same:

#48449

Pankraz76 · 2025-06-19T10:58:49Z

1K LOC of tests nearly up to 100%.

im here to serve you.

maxandersen · 2025-06-19T21:11:03Z

1K LOC of tests nearly up to 100%.

I'm not following what you mean ?

Pankraz76 · 2025-06-20T06:47:04Z

1_000 lines of code for testing.

Nearly up to 100% test coverage, from zero before. Im wondering what JBoss understands under the term Professional.

I have different interpretation of this term.

Pankraz76 · 2025-06-20T06:48:51Z

Drop unused members of DefaultAccessLogReceiver #48449

first need enabler as 50% of class makes no sense or is simple dead.

Pankraz76 · 2025-06-20T09:07:32Z

closing as impossible for me to fix without removal of technical depth prior. Affected class has 50% dead code-

maintenance burden #48471

Pankraz76 · 2025-06-20T11:38:26Z

@iddeepak consider this a real benefit and learning experience in all manners.

Feel free to take over and improve quarkus, as the world relies on this high performing AC.

Pankraz76 · 2025-06-21T11:38:36Z

+        assertThat(Files.list(tempDir))
+                .extracting(Path::getFileName)
+                .extracting(Path::toString)
+                .containsExactlyInAnyOrder(


this the real deal test:

java.lang.AssertionError: Expecting actual: ["test-empty.log", "test-empty.2025-06-21-2.log", "test-empty.2025-06-21-3.log", "test-empty.2025-06-21-1.log", "test-empty2025-06-21.log", "test-empty.2025-06-21.log"] to contain exactly in any order: ["test-empty.log", "test-empty.2025-06-21.log", "test-empty.2025-06-21-1.log", "test-empty.2025-06-21-2.log", "test-empty.2025-06-21-3.log", "test-empty.2025-06-21-4.log"] elements not found: ["test-empty.2025-06-21-4.log"] and elements not expected: ["test-empty2025-06-21.log"] at io.quarkus.vertx.http.runtime.DefaultAccessLogReceiverTest.testRotationWithEmptyLogFile(DefaultAccessLogReceiverTest.java:780) at java.base/java.lang.reflect.Method.invoke(Method.java:569) at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

Pankraz76 · 2025-06-21T11:45:52Z

each test is dedicated to one concern, therefore should be simple to digest.

Let me know if its too much quality assurance resulting in test coverage. Idk whats the DoD or what quality gate to pass.

gsmet · 2025-07-07T16:38:08Z

I'm closing this one in favor of #48814 .

Thanks for the effort, I made you a co-author in the other PR.

quarkus-bot Bot added area/reactive-sql-clients area/vertx labels Jun 15, 2025

Pankraz76 commented Jun 15, 2025

View reviewed changes

Comment thread ...x-http/runtime/src/test/java/io/quarkus/vertx/http/runtime/DefaultAccessLogReceiverTest.java Outdated

Pankraz76 marked this pull request as ready for review June 15, 2025 19:12

This was referenced Jun 16, 2025

fix missing alphabetical order with sortpom-maven-plugin #48382

Closed

Issue #48342 chore: simplify DefaultAccessLogReceiver #48402

Closed

Pankraz76 changed the title ~~[48342] fix: No divider between base file name and timestamp in the name of rotated access log~~ Issue quarkusio#48342 fix: No divider between base file name and timestamp in the name of rotated access log Jun 16, 2025

Pankraz76 changed the title ~~Issue quarkusio#48342 fix: No divider between base file name and timestamp in the name of rotated access log~~ Issue #48342 fix: No divider between base file name and timestamp in the name of rotated access log Jun 16, 2025

Pankraz76 requested a review from geoand June 17, 2025 09:06

geoand added area/logging and removed area/vertx area/reactive-sql-clients labels Jun 17, 2025

geoand reviewed Jun 17, 2025

View reviewed changes

quarkus-bot Bot added the area/vertx label Jun 17, 2025

Pankraz76 commented Jun 17, 2025

View reviewed changes

Comment thread .../src/main/java/io/quarkus/vertx/http/runtime/filters/accesslog/DefaultAccessLogReceiver.java

Pankraz76 commented Jun 17, 2025

View reviewed changes

Pankraz76 requested a review from geoand June 17, 2025 10:31

Pankraz76 mentioned this pull request Jun 18, 2025

Drop unused members of DefaultAccessLogReceiver #48449

Closed

Pankraz76 changed the title ~~Issue #48342 fix: No divider between base file name and timestamp in the name of rotated access log~~ Issue #48342 fix DefaultAccessLogReceiver: No divider between base file name and timestamp in the name of rotated access log Jun 18, 2025

Pankraz76 changed the title ~~Issue #48342 fix DefaultAccessLogReceiver: No divider between base file name and timestamp in the name of rotated access log~~ Fix divider in rotated access log for DefaultAccessLogReceiver Jun 18, 2025

Pankraz76 commented Jun 18, 2025

View reviewed changes

Comment thread .../src/main/java/io/quarkus/vertx/http/runtime/filters/accesslog/DefaultAccessLogReceiver.java

Pankraz76 commented Jun 18, 2025

View reviewed changes

Pankraz76 mentioned this pull request Jun 19, 2025

integrate Eclipse Formatter Profile into Maven Spotless Plugin #48047

Closed

Pankraz76 mentioned this pull request Jun 19, 2025

Drop unused members of VertxHttpProcessor #48473

Closed

Pankraz76 closed this Jun 20, 2025

quarkus-bot Bot added the triage/invalid This doesn't seem right label Jun 20, 2025

This was referenced Jun 20, 2025

No divider between base file name and timestamp in the name of rotated access log #48342

Closed

Fix #5770: [java] UnnecessaryLocalBeforeBranchRule pmd/pmd#5835

Closed

Pankraz76 mentioned this pull request Jun 21, 2025

Migrate to Java 17 #47302

Closed

Pankraz76 reopened this Jun 21, 2025

quarkus-bot Bot removed the triage/invalid This doesn't seem right label Jun 21, 2025

Pankraz76 commented Jun 21, 2025

View reviewed changes

Fix divider in rotated access log for DefaultAccessLogReceiver

05596d7

gsmet closed this Jul 7, 2025

quarkus-bot Bot added the triage/invalid This doesn't seem right label Jul 7, 2025

		assertTrue(Files.list(tempDir)
		.anyMatch(p -> p.getFileName().toString().matches("test-rotate\\d{4}-\\d{2}-\\d{2}\\.log")));

	.filter(p -> p.getFileName().toString().matches("test-multi-rotate.\\d{4}-\\d{2}-\\d{2}\\.log"))
	.filter(p -> p.getFileName().toString().matches("test-multi-rotate\\d{4}-\\d{2}-\\d{2}\\.log"))

Uh oh!

Conversation

Pankraz76 commented Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quarkus-bot Bot commented Jun 15, 2025

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!

Uh oh!

geoand left a comment

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!

Pankraz76 commented Jun 17, 2025

Uh oh!

geoand commented Jun 17, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 commented Jun 17, 2025

Uh oh!

Pankraz76 commented Jun 18, 2025

Uh oh!

Uh oh!

Pankraz76 Jun 18, 2025 • 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!

Pankraz76 commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pankraz76 commented Jun 19, 2025

Uh oh!

maxandersen commented Jun 19, 2025

Uh oh!

Pankraz76 commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pankraz76 commented Jun 20, 2025

Uh oh!

Pankraz76 commented Jun 20, 2025

Uh oh!

Pankraz76 commented Jun 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 commented Jun 21, 2025

Uh oh!

gsmet commented Jul 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Pankraz76 commented Jun 15, 2025 •

edited

Loading

Pankraz76 Jun 18, 2025 •

edited

Loading

Pankraz76 commented Jun 18, 2025 •

edited

Loading

Pankraz76 commented Jun 20, 2025 •

edited

Loading