Copilot commented on code in PR #4076: URL: https://github.com/apache/logging-log4j2/pull/4076#discussion_r2983754749
########## src/changelog/.2.x.x/rfc5424-setters.xml: ########## @@ -0,0 +1,11 @@ +<?xml version="1.0" encoding="UTF-8"?> +<entry xmlns="https://logging.apache.org/xml/ns"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation=" + https://logging.apache.org/xml/ns + https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"; + type="added"> + <description format="asciidoc"> + Add missing setters to `Rfc5424LayoutBuilder`. + </description> Review Comment: This changelog entry is missing an <issue> element linking to the PR/issue. All other entries in src/changelog/.2.x.x include at least one <issue id="…" link="…"/>; please add the appropriate link for traceability and consistency. ```suggestion </description> <issue id="GH-0000" link="https://github.com/apache/logging-log4j2/pull/0000"/> ``` ########## log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java: ########## @@ -860,9 +1090,12 @@ public Rfc5424LayoutBuilder setEnterpriseNumber(Integer enterpriseNumber) { @Override public Rfc5424Layout build() { - if (includes != null && excludes != null) { + String effectiveIncludes = Objects.toString(mdcIncludes, includes); + String effectiveExcludes = Objects.toString(mdcExcludes, excludes); + + if (effectiveIncludes != null && effectiveExcludes != null) { LOGGER.error("mdcIncludes and mdcExcludes are mutually exclusive. Includes wil be ignored"); Review Comment: Typo in the error log message: "Includes wil be ignored" should be "Includes will be ignored". (This string also appears elsewhere in the class; it should be corrected consistently.) ```suggestion LOGGER.error("mdcIncludes and mdcExcludes are mutually exclusive. Includes will be ignored"); ``` ########## log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java: ########## @@ -880,19 +1113,19 @@ public Rfc5424Layout build() { id, ein, includeMDC, - includeNL, - escapeNL, + newLine || includeNL, + Objects.toString(newLineEscape, escapeNL), Review Comment: The newLine/includeNL alias handling uses `newLine || includeNL`, which means the old alias can still force newLine on even if the documented attribute is explicitly set to false. To match the precedence used for string aliases (new attribute wins), consider switching these flags to `Boolean` so you can detect “unset” and then resolve with precedence (e.g., prefer `newLine` when non-null, otherwise fall back to `includeNL`). ########## log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java: ########## @@ -780,16 +938,40 @@ public Rfc5424LayoutBuilder setIncludeMDC(final boolean includeMDC) { return this; } + /** + * @deprecated Since 2.26.0 use {@link #setNewLine} instead. + */ + @Deprecated public Rfc5424LayoutBuilder setIncludeNL(final boolean includeNL) { this.includeNL = includeNL; return this; } + /** + * @since 2.26.0 + */ + public Rfc5424LayoutBuilder setNewLine(boolean newLine) { + this.newLine = newLine; + return this; + } + + /** + * @deprecated Since 2.26.0 use {@link #setIncludeNL} instead. + */ + @Deprecated public Rfc5424LayoutBuilder setEscapeNL(final String escapeNL) { this.escapeNL = escapeNL; Review Comment: The Javadoc for the deprecated setEscapeNL method points to the wrong replacement method (it references setIncludeNL, which is unrelated and also deprecated). This should direct users to setNewLineEscape. ########## log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java: ########## @@ -880,19 +1113,19 @@ public Rfc5424Layout build() { id, ein, includeMDC, - includeNL, - escapeNL, + newLine || includeNL, + Objects.toString(newLineEscape, escapeNL), mdcId, mdcPrefix, eventPrefix, appName, messageId, - excludes, - includes, - required, + effectiveExcludes, + effectiveIncludes, + Objects.toString(mdcRequired, required), charset != null ? charset : StandardCharsets.UTF_8, exceptionPattern, - useTLSMessageFormat, + useTlsMessageFormat || useTLSMessageFormat, loggerFields); Review Comment: The useTlsMessageFormat/useTLSMessageFormat alias handling uses `useTlsMessageFormat || useTLSMessageFormat`, which prevents the documented attribute from overriding the legacy alias if both are present. Consider using `Boolean` fields and resolving with a clear precedence rule (prefer the documented attribute when set) for consistency with the string alias handling. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
