Skip to content

Improve configuration error handling of HttpAppender #3011

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.

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

Closed
ppkarwasz opened this issue Sep 24, 2024 · 5 comments · Fixed by #3438
Closed

Improve configuration error handling of HttpAppender #3011

ppkarwasz opened this issue Sep 24, 2024 · 5 comments · Fixed by #3438
Labels
appenders Affects one or more Appender plugins bug Incorrect, unexpected, or unintended behavior of existing code waiting-for-user More information is needed from the user

Comments

@ppkarwasz
Copy link
Contributor

ppkarwasz commented Sep 24, 2024

As reported in this SO question if the HTTP appender does not have a configured layout, it fails with an NPE.

The correct behavior should be:

  • issue a status logger message at level ERROR,
  • return null.
@ppkarwasz ppkarwasz added the bug Incorrect, unexpected, or unintended behavior of existing code label Sep 24, 2024
@vy vy added the appenders Affects one or more Appender plugins label Sep 26, 2024
@Suvrat1629
Copy link
Contributor

@ppkarwasz
I have made the necessary changes in the HttpAppender.java file but i don't know how and what changes are required in the HttpAppenderTest.java. I did try adding some test cases but i got the following error on testing

Image
sorry for my lack of understanding about testing but is it necessary to write test cases to check the (layout == null) behaviour

@ppkarwasz
Copy link
Contributor Author

What you want to test is the behavior of the Builder class. Since HttpAppenderTest is a heavy test that starts up an HTTP server, I would rather create a new HttpAppenderBuilderTest to test the builder.

The builder needs to test that the required properties (name, url and layout) are present, before creating the appender. So, for example, you could test that:

HttpAppender.newBuilder()
    .setName("HTTP")
    .setUrl(new URL("https://localhost"))
    .build();

returns null and logs an ERROR status logger message. To capture the status logger messages for a test case, we have an @UsingStatusListener annotation that injects a ListStatusListener into the test parameters. The @UsingStatusListener annotation is for example used here.

@Test
@UsingStatusListener
void testNoAnsiNonEmpty(final ListStatusListener listener) {
final String[] options = {"%-5level: %msg", PatternParser.DISABLE_ANSI + "=true"};
final HighlightConverter converter = HighlightConverter.newInstance(null, options);
assertNotNull(converter);
assertThat(listener.findStatusData(Level.WARN)).isEmpty();
final LogEvent event = Log4jLogEvent.newBuilder()
.setLevel(Level.INFO)
.setLoggerName("a.b.c")
.setMessage(new SimpleMessage("message in a bottle"))
.build();
final StringBuilder buffer = new StringBuilder();
converter.format(event, buffer);
assertEquals("INFO : message in a bottle", buffer.toString());
}

@Suvrat1629
Copy link
Contributor

I did write the test cases but i am facing a weird problem, my HttpAppenderBuilderTest.java is not able to find the required dependencies is there some xml file or something that i have to write? HttpAppenderBuilderTest.java is not even able to find the dependencies present in the HttpAppenderTest.java. Almost none of the import statements are working. Could you pleasae guide me with what i could possibly do to fix this

@vy
Copy link
Member

vy commented Jan 16, 2025

@Suvrat1629, I presume you mean you cannot build and/or run tests in your IDE. If you use IntelliJ IDEA, install everything first (i.e. ./mvnw install -DskipTests) and then use Reload All Maven Projects Incrementally action – it is the reload button in the Maven side window. For further details, see the development instructions.

@Suvrat1629
Copy link
Contributor

@vy
Thanks for the response,
I will follow the steps you have given me and try to get back to this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appenders Affects one or more Appender plugins bug Incorrect, unexpected, or unintended behavior of existing code waiting-for-user More information is needed from the user
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants