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

Console logging incorrectly uses Charset.defaultCharset() or UTF-8 #44353

Closed
wants to merge 1 commit into from
Closed

Console logging incorrectly uses Charset.defaultCharset() or UTF-8 #44353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 13 additions & 5 deletions ...t/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystemProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

package org.springframework.boot.logging;

import java.io.Console;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.function.BiConsumer;
import java.util.function.Function;

Expand Down Expand Up @@ -91,8 +91,12 @@ public LoggingSystemProperties(Environment environment, Function<String, String>
this.setter = (setter != null) ? setter : systemPropertySetter;
}

protected Console getConsole() {
return System.console();
}

protected Charset getDefaultCharset() {
return StandardCharsets.UTF_8;
return Charset.defaultCharset();
Copy link
Contributor Author

@nosan nosan Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This change will impact log4j2 CONSOLE_CHARSET and FILE_CHARSET.

Copy link
Contributor

@mhalbritter mhalbritter Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's fine that the log4j console output is changed, but I'm not sure about the file encoding. I'd prefer if the file stays utf-8. #43118 is about console logging only.

Copy link
Contributor Author

@nosan nosan Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The current situation

Log4j2 Logback
Console UTF-8 Charset.default()
File UTF-8 Charset.default()

The proposed PR aims to align the situation as follows:

Log4j2 Logback
Console Console.charset() or Charset.default() Console.charset() or Charset.default()
File Charset.default() Charset.default()

Alternatively, we could leave protected Charset getDefaultCharset() untouched, and this would result in

Log4j2 Logback
Console Console.charset() or UTF-8 Console.charset() or Charset.default()
File UTF-8 Charset.default()

I think it's fine that the log4j console output is changed, but I'm not sure about the file encoding. I'd prefer if the file stays utf-8.

this would result in

Log4j2 Logback
Console Console.charset() or Charset.default() Console.charset() or Charset.default()
File UTF-8 Charset.default()

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

The last table. I have some changes here, WDYT? https://github.com/mhalbritter/spring-boot/tree/pr/44353

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not retain UTF-8 for Log4j2 in this case?

Log4j2 Logback
Console Console.charset() or UTF-8 Console.charset() or Charset.default()
File UTF-8 Charset.default()

In that case, the logic will remain the same as the current one, except for scenarios
where the Console is available.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because Phil said here:

I think I'd be in favor of using System.console().charset() if we can and Charset.defaultCharset() if the Console is null. That would mostly align with Logback defaults. The JDK also appears to use Charset.defaultCharset() as a fallback (at least in JDK 17).

Copy link
Contributor Author

@nosan nosan Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've seen this but at the same time UTF-8 is the default one in Log4j2.

https://github.com/apache/logging-log4j2/blob/cab5454e201de95817600e79a26235c72e65d195/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java#L194

If it's alright with you, could we wait for @philwebb to provide clarification?

UPDATE:

I didn't notice this comment when writing mine.

I'm actually surprised that the logback file is not written in UTF-8. I'll talk to the team about that.

}

public final void apply() {
Expand All @@ -116,12 +120,11 @@ private PropertyResolver getPropertyResolver() {
}

protected void apply(LogFile logFile, PropertyResolver resolver) {
String defaultCharsetName = getDefaultCharset().name();
setSystemProperty(LoggingSystemProperty.APPLICATION_NAME, resolver);
setSystemProperty(LoggingSystemProperty.APPLICATION_GROUP, resolver);
setSystemProperty(LoggingSystemProperty.PID, new ApplicationPid().toString());
setSystemProperty(LoggingSystemProperty.CONSOLE_CHARSET, resolver, defaultCharsetName);
setSystemProperty(LoggingSystemProperty.FILE_CHARSET, resolver, defaultCharsetName);
setSystemProperty(LoggingSystemProperty.CONSOLE_CHARSET, resolver, getConsoleCharset().name());
setSystemProperty(LoggingSystemProperty.FILE_CHARSET, resolver, getDefaultCharset().name());
setSystemProperty(LoggingSystemProperty.CONSOLE_THRESHOLD, resolver, this::thresholdMapper);
setSystemProperty(LoggingSystemProperty.FILE_THRESHOLD, resolver, this::thresholdMapper);
setSystemProperty(LoggingSystemProperty.EXCEPTION_CONVERSION_WORD, resolver);
Expand All @@ -137,6 +140,11 @@ protected void apply(LogFile logFile, PropertyResolver resolver) {
}
}

private Charset getConsoleCharset() {
Console console = getConsole();
return (console != null) ? console.charset() : getDefaultCharset();
}

private void setSystemProperty(LoggingSystemProperty property, PropertyResolver resolver) {
setSystemProperty(property, resolver, Function.identity());
}
Expand Down
8 changes: 4 additions & 4 deletions ...rc/main/java/org/springframework/boot/logging/logback/LogbackLoggingSystemProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2024 the original author or authors.
* Copyright 2012-2025 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,7 +16,7 @@

package org.springframework.boot.logging.logback;

import java.nio.charset.Charset;
import java.io.Console;
import java.util.function.BiConsumer;
import java.util.function.Function;

Expand Down Expand Up @@ -71,8 +71,8 @@ public LogbackLoggingSystemProperties(Environment environment, Function<String,
}

@Override
protected Charset getDefaultCharset() {
return Charset.defaultCharset();
protected Console getConsole() {
return super.getConsole();
}

@Override
Expand Down
27 changes: 23 additions & 4 deletions ...ing-boot/src/test/java/org/springframework/boot/logging/LoggingSystemPropertiesTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2024 the original author or authors.
* Copyright 2012-2025 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,6 +16,9 @@

package org.springframework.boot.logging;

import java.io.Console;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.Collections;
import java.util.HashSet;
import java.util.Map;
Expand All @@ -31,6 +34,9 @@
import org.springframework.mock.env.MockEnvironment;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;

/**
* Tests for {@link LoggingSystemProperties}.
Expand Down Expand Up @@ -72,9 +78,22 @@ void consoleLogPatternIsSet() {
}

@Test
void consoleCharsetWhenNoPropertyUsesUtf8() {
new LoggingSystemProperties(new MockEnvironment()).apply(null);
assertThat(getSystemProperty(LoggingSystemProperty.CONSOLE_CHARSET)).isEqualTo("UTF-8");
void consoleCharsetWhenNoPropertyUsesCharsetDefault() {
LoggingSystemProperties loggingSystemProperties = spy(new LoggingSystemProperties(new MockEnvironment()));
given(loggingSystemProperties.getConsole()).willReturn(null);
loggingSystemProperties.apply(null);
assertThat(getSystemProperty(LoggingSystemProperty.CONSOLE_CHARSET)).isEqualTo(Charset.defaultCharset().name());
}

@Test
void consoleCharsetWhenNoPropertyUsesSystemConsoleCharsetWhenAvailable() {
LoggingSystemProperties loggingSystemProperties = spy(new LoggingSystemProperties(new MockEnvironment()));
Console console = mock(Console.class);
given(console.charset()).willReturn(StandardCharsets.UTF_16BE);
given(loggingSystemProperties.getConsole()).willReturn(console);
loggingSystemProperties.apply(null);
assertThat(getSystemProperty(LoggingSystemProperty.CONSOLE_CHARSET))
.isEqualTo(StandardCharsets.UTF_16BE.name());
}

@Test
Expand Down
27 changes: 25 additions & 2 deletions ...st/java/org/springframework/boot/logging/logback/LogbackLoggingSystemPropertiesTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2023 the original author or authors.
* Copyright 2012-2025 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,7 +16,9 @@

package org.springframework.boot.logging.logback;

import java.io.Console;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.HashSet;
import java.util.Set;

Expand All @@ -31,6 +33,9 @@
import org.springframework.mock.env.MockEnvironment;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;

/**
* Tests for {@link LogbackLoggingSystemProperties}.
Expand Down Expand Up @@ -102,11 +107,29 @@ void applySetsLogbackSystemPropertiesFromDeprecated() {

@Test
void consoleCharsetWhenNoPropertyUsesDefault() {
new LoggingSystemProperties(new MockEnvironment()).apply(null);
LogbackLoggingSystemProperties logbackLoggingSystemProperties = spy(
new LogbackLoggingSystemProperties(new MockEnvironment(), null, null));
given(logbackLoggingSystemProperties.getConsole()).willReturn(null);

logbackLoggingSystemProperties.apply(null);
assertThat(System.getProperty(LoggingSystemProperty.CONSOLE_CHARSET.getEnvironmentVariableName()))
.isEqualTo(Charset.defaultCharset().name());
}

@Test
void consoleCharsetWhenNoPropertyUsesSystemConsoleCharsetWhenAvailable() {
LogbackLoggingSystemProperties logbackLoggingSystemProperties = spy(
new LogbackLoggingSystemProperties(new MockEnvironment(), null, null));

Console console = mock(Console.class);
given(console.charset()).willReturn(StandardCharsets.UTF_16BE);
given(logbackLoggingSystemProperties.getConsole()).willReturn(console);

logbackLoggingSystemProperties.apply(null);
assertThat(System.getProperty(LoggingSystemProperty.CONSOLE_CHARSET.getEnvironmentVariableName()))
.isEqualTo(StandardCharsets.UTF_16BE.name());
}

@Test
void fileCharsetWhenNoPropertyUsesDefault() {
new LoggingSystemProperties(new MockEnvironment()).apply(null);
Expand Down
Loading