Skip to content
This repository has been archived by the owner on Jun 29, 2023. It is now read-only.

Fix formatting with single quote arguments for String.format with JUL #225

Closed
wants to merge 1 commit into from

Conversation

rthaenert
Copy link

Make sure that:

  • You have read the contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

Hey Mark,

we are using logstash-gelf for Applications on JBoss EAP 7.

We noticed that some of the JBoss startup messages are not displayed properly when sending via logstash-gelf.
Example after formatting:
WFLYEJB0473: JNDI bindings for session bean named %s in deployment unit %s are as follows:%s
Message Format Template:
JNDI bindings for session bean named '%s' in deployment unit '%s' are as follows:%s

This message comes from Wildfly (see the following line in Wildfly: https://github.com/wildfly/wildfly/blob/cd1a5e2393052f3a91e77e581dfeee839d6f819d/ejb3/src/main/java/org/jboss/as/ejb3/logging/EjbLogger.java#L3059 ).

The log message has single quotes which get removed by the MessageFormat call. Afterwards a subsequent String.format will not be applied (because the String was changed and is now missing the single quotes).

One easy fix for that is to check whether there is a MessageFormat identifier ("{") inside the message and skip MessageFormat if that's not the case.
Although this won't solve all cases it should be good enough to fix the Startup Messages and not break something else.

@mp911de
Copy link
Owner

mp911de commented Jan 10, 2020

Would it make sense to switch the order of calls (String.format(…), then MessageFormat.format(…))?

@rthaenert
Copy link
Author

This idea came up as well.

However, the documentation of java.util.Logger points to MessageFormat (Typically, formatters use java.text.MessageFormat style formatting) and slf4j uses MessageFormat as well in their documentation.

When putting String.format in front of the MessageFormat the following would not work as there is a quote sign somewhere in the message:
logger.error("%sdfsdfk#! {0}", "someParam");
These are rare cases, however I wouldn't feel confident switching the order as this might break someone's log message formatting.

@mp911de
Copy link
Owner

mp911de commented Jan 11, 2020

In any case, having two formatting approaches within the same logger is kind of weird. We can merge this PR. If we encounter further issues, we can add a config property to control formatting order and enablement (formatter=message,format or something like that).

@mp911de mp911de added the type: task A general task label Jan 11, 2020
@mp911de mp911de added this to the 1.14.0 milestone Jan 11, 2020
@rthaenert
Copy link
Author

Thanks, we'll look out for further issues regarding the formatting.

mp911de added a commit that referenced this pull request Jan 13, 2020
Use indexOf(char) to determine MessageFormat presence. Simplify MessageFormat test. Add author tags.
@mp911de
Copy link
Owner

mp911de commented Jan 13, 2020

Thank you for your contribution. That's merged and polished now.

@mp911de mp911de closed this Jan 13, 2020
@mp911de mp911de changed the title fix issue with single quote arguments for String.format in EAP7 Fix formatting with single quote arguments for String.format with JUL Jan 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants