You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/05/09 06:51:55 UTC
[GitHub] [logging-log4j2] mneundorfer opened a new pull request, #836: Define eid as String instead of int to allow for Oid according to RFC5424
mneundorfer opened a new pull request, #836:
URL: https://github.com/apache/logging-log4j2/pull/836
As discussed in https://issues.apache.org/jira/browse/LOG4J2-1376, the enterprise ID is actually not an `int`, but an OID. Since converting directly to an OID is a non-trivial task, this PR proposes a first step in the right direction by using a `String`, which allows for RFC5424-conform behavior, since now an OID can be passed as enterprise ID.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871106575
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/SyslogAppender.java:
##########
@@ -226,6 +242,11 @@ public B setEnterpriseNumber(final String enterpriseNumber) {
return asBuilder();
}
+ public B setEnterpriseNumber(final int enterpriseNumber) {
Review Comment:
`@Deprecated`
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
rgoers commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871587464
##########
log4j-core/src/test/java/org/apache/logging/log4j/core/appender/TlsSyslogAppenderTest.java:
##########
@@ -94,7 +94,7 @@ private SyslogAppender createAppender() {
}
return SyslogAppender.createAppender("localhost", PORTNUM, "SSL", sslConfiguration, 0, -1, true, "Test", true,
- false, Facility.LOCAL0, "Audit", 18060, true, "RequestContext", null, null, includeNewLine, null,
+ false, Facility.LOCAL0, "Audit", "18060", true, "RequestContext", null, null, includeNewLine, null,
Review Comment:
You mean - existing tests should not be modified. New ones should be created to test the new functionality.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] mneundorfer commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
mneundorfer commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871007329
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/StructuredDataId.java:
##########
@@ -142,15 +142,15 @@ public StructuredDataId(final String name, final int enterpriseNumber, final Str
* @param maxLength The maximum length of the StructuredData Id key.
* @since 2.9
*/
- public StructuredDataId(final String name, final int enterpriseNumber, final String[] required,
+ public StructuredDataId(final String name, final String enterpriseNumber, final String[] required,
Review Comment:
I addressed this to all public signatures - kept the old one, deprecated it and added a new method with the parameter as `String`
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r867885092
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/StructuredDataId.java:
##########
@@ -127,7 +127,7 @@ public StructuredDataId(final String name, final String[] required, final String
* @param required The list of keys that are required for this id.
* @param optional The list of keys that are optional for this id.
*/
- public StructuredDataId(final String name, final int enterpriseNumber, final String[] required,
+ public StructuredDataId(final String name, final String enterpriseNumber, final String[] required,
Review Comment:
For backward compatibility, the signature of public methods can not be changed. Just deprecate the old constructor and create a new one.
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/StructuredDataId.java:
##########
@@ -142,15 +142,15 @@ public StructuredDataId(final String name, final int enterpriseNumber, final Str
* @param maxLength The maximum length of the StructuredData Id key.
* @since 2.9
*/
- public StructuredDataId(final String name, final int enterpriseNumber, final String[] required,
+ public StructuredDataId(final String name, final String enterpriseNumber, final String[] required,
Review Comment:
As above, the change must be backward compatible. This comment applies to all other signature changes of public methods.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -71,9 +71,9 @@
public final class Rfc5424Layout extends AbstractStringLayout {
/**
- * Not a very good default - it is the Apache Software Foundation's enterprise number.
+ * The default example enterprise number from RFC5424.
*/
- public static final int DEFAULT_ENTERPRISE_NUMBER = 18060;
+ public static final String DEFAULT_ENTERPRISE_NUMBER = "32473";
Review Comment:
I totally agree, `32473` should be the default.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/SyslogAppender.java:
##########
@@ -375,7 +375,7 @@ public static <B extends Builder<B>> SyslogAppender createAppender(
final boolean ignoreExceptions,
final Facility facility,
final String id,
- final int enterpriseNumber,
+ final String enterpriseNumber,
Review Comment:
This factory method is deprecated. You can leave the old signature.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -610,8 +610,8 @@ public static Rfc5424Layout createLayout(
// @formatter:off
@PluginAttribute(value = "facility", defaultString = "LOCAL0") final Facility facility,
@PluginAttribute("id") final String id,
- @PluginAttribute(value = "enterpriseNumber", defaultInt = DEFAULT_ENTERPRISE_NUMBER)
- final int enterpriseNumber,
+ @PluginAttribute(value = "enterpriseNumber", defaultString = DEFAULT_ENTERPRISE_NUMBER)
+ final String enterpriseNumber,
Review Comment:
I think it is about time to deprecate this method and create a `Rfc5424Layout.Builder` class. The builder should validate the `enterpriseNumber` parameter in its `build()` method.
The signature of the method should remain the same.
##########
log4j-core/src/test/java/org/apache/logging/log4j/core/appender/TlsSyslogAppenderTest.java:
##########
@@ -94,7 +94,7 @@ private SyslogAppender createAppender() {
}
return SyslogAppender.createAppender("localhost", PORTNUM, "SSL", sslConfiguration, 0, -1, true, "Test", true,
- false, Facility.LOCAL0, "Audit", 18060, true, "RequestContext", null, null, includeNewLine, null,
+ false, Facility.LOCAL0, "Audit", "18060", true, "RequestContext", null, null, includeNewLine, null,
Review Comment:
No need to modify the tests if backward compatibility is maintained.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz merged pull request #836: [LOG4J2-1376] Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
ppkarwasz merged PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r869474731
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -610,8 +610,8 @@ public static Rfc5424Layout createLayout(
// @formatter:off
@PluginAttribute(value = "facility", defaultString = "LOCAL0") final Facility facility,
@PluginAttribute("id") final String id,
- @PluginAttribute(value = "enterpriseNumber", defaultInt = DEFAULT_ENTERPRISE_NUMBER)
- final int enterpriseNumber,
+ @PluginAttribute(value = "enterpriseNumber", defaultString = DEFAULT_ENTERPRISE_NUMBER)
+ final String enterpriseNumber,
Review Comment:
To change the type of the `enterpriseNumber` parameter, you would need to introduce a new factory method. The current practice in Log4j is to gradually migrate to builders: every time a new factory method would be required, we create a builder instead.
The current convention for builders is to name the methods `set*` (not `with*` as in some previous code).
I would expect the `enterpriseNumber` parameter to be validated against the `\d+(\.\d+)*` regex. If the validation fails a warning should be written to the status logger and `null` should be returned.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871109379
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -634,11 +640,142 @@ public static Rfc5424Layout createLayout(
includes = null;
}
- return new Rfc5424Layout(config, facility, id, enterpriseNumber, includeMDC, newLine, escapeNL, mdcId,
+ return new Rfc5424Layout(config, facility, id, String.valueOf(enterpriseNumber), includeMDC, newLine, escapeNL, mdcId,
mdcPrefix, eventPrefix, appName, msgId, excludes, includes, required, StandardCharsets.UTF_8,
exceptionPattern, useTlsMessageFormat, loggerFields);
}
+ public static class Rfc5424LayoutBuilder {
+ private Configuration config;
+ private Facility facility;
+ private String id;
+ private String ein;
+ private boolean includeMDC;
+ private boolean includeNL;
+ private String escapeNL;
+ private String mdcId;
+ private String mdcPrefix;
+ private String eventPrefix;
+ private String appName;
+ private String messageId;
+ private String excludes;
+ private String includes;
+ private String required;
+ private Charset charset;
+ private String exceptionPattern;
+ private boolean useTLSMessageFormat;
+ private LoggerFields[] loggerFields;
+
+ public Rfc5424LayoutBuilder setConfig(Configuration config) {
+ this.config = config;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setFacility(Facility facility) {
+ this.facility = facility;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setId(String id) {
+ this.id = id;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setEin(String ein) {
+ this.ein = ein;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setIncludeMDC(boolean includeMDC) {
+ this.includeMDC = includeMDC;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setIncludeNL(boolean includeNL) {
+ this.includeNL = includeNL;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setEscapeNL(String escapeNL) {
+ this.escapeNL = escapeNL;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setMdcId(String mdcId) {
+ this.mdcId = mdcId;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setMdcPrefix(String mdcPrefix) {
+ this.mdcPrefix = mdcPrefix;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setEventPrefix(String eventPrefix) {
+ this.eventPrefix = eventPrefix;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setAppName(String appName) {
+ this.appName = appName;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setMessageId(String messageId) {
+ this.messageId = messageId;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setExcludes(String excludes) {
+ this.excludes = excludes;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setIncludes(String includes) {
+ this.includes = includes;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setRequired(String required) {
+ this.required = required;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setCharset(Charset charset) {
+ this.charset = charset;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setExceptionPattern(String exceptionPattern) {
+ this.exceptionPattern = exceptionPattern;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setUseTLSMessageFormat(boolean useTLSMessageFormat) {
+ this.useTLSMessageFormat = useTLSMessageFormat;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setLoggerFields(LoggerFields[] loggerFields) {
+ this.loggerFields = loggerFields;
+ return this;
+ }
+
+ public Rfc5424Layout build() {
+ if (includes != null && excludes != null) {
+ LOGGER.error("mdcIncludes and mdcExcludes are mutually exclusive. Includes wil be ignored");
+ includes = null;
+ }
+
+ if (ein != null && !ENTERPRISE_ID_PATTERN.matcher(ein).find()) {
Review Comment:
I would use `matches()` instead of `find()`, since the entire enterprise id must match the regular expression.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] mneundorfer commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
mneundorfer commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871163586
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/StructuredDataId.java:
##########
@@ -139,11 +139,30 @@ public StructuredDataId(final String name, final String enterpriseNumber, final
* @param enterpriseNumber The enterprise number.
* @param required The list of keys that are required for this id.
* @param optional The list of keys that are optional for this id.
+ * @deprecated Use {@link #StructuredDataId(String, String, String[], String[])} instead.
+ */
+ @Deprecated
+ public StructuredDataId(final String name, final int enterpriseNumber, final String[] required,
+ final String[] optional) {
+ this(name, String.valueOf(enterpriseNumber), required, optional, MAX_LENGTH);
+ }
+
+ /**
+ * A Constructor that helps conformance to RFC 5424.
+ *
+ * Deprecated:
+ * enterpriseNumber should be a String, use StructuredDataId(final String name, final String enterpriseNumber, final String[] required,
+ * final String[] optional, final int maxLength)
Review Comment:
Obviously, forgot to delete this one. Thanks
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -634,11 +640,142 @@ public static Rfc5424Layout createLayout(
includes = null;
}
- return new Rfc5424Layout(config, facility, id, enterpriseNumber, includeMDC, newLine, escapeNL, mdcId,
+ return new Rfc5424Layout(config, facility, id, String.valueOf(enterpriseNumber), includeMDC, newLine, escapeNL, mdcId,
mdcPrefix, eventPrefix, appName, msgId, excludes, includes, required, StandardCharsets.UTF_8,
exceptionPattern, useTlsMessageFormat, loggerFields);
}
+ public static class Rfc5424LayoutBuilder {
+ private Configuration config;
+ private Facility facility;
+ private String id;
+ private String ein;
+ private boolean includeMDC;
+ private boolean includeNL;
+ private String escapeNL;
+ private String mdcId;
+ private String mdcPrefix;
+ private String eventPrefix;
+ private String appName;
+ private String messageId;
+ private String excludes;
+ private String includes;
+ private String required;
+ private Charset charset;
+ private String exceptionPattern;
+ private boolean useTLSMessageFormat;
+ private LoggerFields[] loggerFields;
+
+ public Rfc5424LayoutBuilder setConfig(Configuration config) {
+ this.config = config;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setFacility(Facility facility) {
+ this.facility = facility;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setId(String id) {
+ this.id = id;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setEin(String ein) {
+ this.ein = ein;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setIncludeMDC(boolean includeMDC) {
+ this.includeMDC = includeMDC;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setIncludeNL(boolean includeNL) {
+ this.includeNL = includeNL;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setEscapeNL(String escapeNL) {
+ this.escapeNL = escapeNL;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setMdcId(String mdcId) {
+ this.mdcId = mdcId;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setMdcPrefix(String mdcPrefix) {
+ this.mdcPrefix = mdcPrefix;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setEventPrefix(String eventPrefix) {
+ this.eventPrefix = eventPrefix;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setAppName(String appName) {
+ this.appName = appName;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setMessageId(String messageId) {
+ this.messageId = messageId;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setExcludes(String excludes) {
+ this.excludes = excludes;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setIncludes(String includes) {
+ this.includes = includes;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setRequired(String required) {
+ this.required = required;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setCharset(Charset charset) {
+ this.charset = charset;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setExceptionPattern(String exceptionPattern) {
+ this.exceptionPattern = exceptionPattern;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setUseTLSMessageFormat(boolean useTLSMessageFormat) {
+ this.useTLSMessageFormat = useTLSMessageFormat;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setLoggerFields(LoggerFields[] loggerFields) {
+ this.loggerFields = loggerFields;
+ return this;
+ }
+
+ public Rfc5424Layout build() {
+ if (includes != null && excludes != null) {
+ LOGGER.error("mdcIncludes and mdcExcludes are mutually exclusive. Includes wil be ignored");
+ includes = null;
+ }
+
+ if (ein != null && !ENTERPRISE_ID_PATTERN.matcher(ein).find()) {
Review Comment:
Very valid, I changed it
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/SyslogAppender.java:
##########
@@ -226,6 +242,11 @@ public B setEnterpriseNumber(final String enterpriseNumber) {
return asBuilder();
}
+ public B setEnterpriseNumber(final int enterpriseNumber) {
Review Comment:
Thanks, added it
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871631509
##########
log4j-core/src/test/java/org/apache/logging/log4j/core/appender/TlsSyslogAppenderTest.java:
##########
@@ -94,7 +94,7 @@ private SyslogAppender createAppender() {
}
return SyslogAppender.createAppender("localhost", PORTNUM, "SSL", sslConfiguration, 0, -1, true, "Test", true,
- false, Facility.LOCAL0, "Audit", 18060, true, "RequestContext", null, null, includeNewLine, null,
+ false, Facility.LOCAL0, "Audit", "18060", true, "RequestContext", null, null, includeNewLine, null,
Review Comment:
Yes, exactly.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871107175
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -634,11 +640,142 @@ public static Rfc5424Layout createLayout(
includes = null;
}
- return new Rfc5424Layout(config, facility, id, enterpriseNumber, includeMDC, newLine, escapeNL, mdcId,
+ return new Rfc5424Layout(config, facility, id, String.valueOf(enterpriseNumber), includeMDC, newLine, escapeNL, mdcId,
mdcPrefix, eventPrefix, appName, msgId, excludes, includes, required, StandardCharsets.UTF_8,
exceptionPattern, useTlsMessageFormat, loggerFields);
}
+ public static class Rfc5424LayoutBuilder {
Review Comment:
Looks good to me.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] Samsung72 commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
Samsung72 commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871121018
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -634,11 +640,142 @@ public static Rfc5424Layout createLayout(
includes = null;
}
- return new Rfc5424Layout(config, facility, id, enterpriseNumber, includeMDC, newLine, escapeNL, mdcId,
+ return new Rfc5424Layout(config, facility, id, String.valueOf(enterpriseNumber), includeMDC, newLine, escapeNL, mdcId,
mdcPrefix, eventPrefix, appName, msgId, excludes, includes, required, StandardCharsets.UTF_8,
exceptionPattern, useTlsMessageFormat, loggerFields);
}
+ public static class Rfc5424LayoutBuilder {
+ private Configuration config;
+ private Facility facility;
+ private String id;
+ private String ein;
+ private boolean includeMDC;
+ private boolean includeNL;
+ private String escapeNL;
+ private String mdcId;
+ private String mdcPrefix;
+ private String eventPrefix;
+ private String appName;
+ private String messageId;
+ private String excludes;
+ private String includes;
+ private String required;
+ private Charset charset;
+ private String exceptionPattern;
+ private boolean useTLSMessageFormat;
+ private LoggerFields[] loggerFields;
+
+ public Rfc5424LayoutBuilder setConfig(Configuration config) {
+ this.config = config;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setFacility(Facility facility) {
+ this.facility = facility;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setId(String id) {
+ this.id = id;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setEin(String ein) {
+ this.ein = ein;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setIncludeMDC(boolean includeMDC) {
+ this.includeMDC = includeMDC;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setIncludeNL(boolean includeNL) {
+ this.includeNL = includeNL;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setEscapeNL(String escapeNL) {
+ this.escapeNL = escapeNL;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setMdcId(String mdcId) {
+ this.mdcId = mdcId;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setMdcPrefix(String mdcPrefix) {
+ this.mdcPrefix = mdcPrefix;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setEventPrefix(String eventPrefix) {
+ this.eventPrefix = eventPrefix;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setAppName(String appName) {
+ this.appName = appName;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setMessageId(String messageId) {
+ this.messageId = messageId;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setExcludes(String excludes) {
+ this.excludes = excludes;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setIncludes(String includes) {
+ this.includes = includes;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setRequired(String required) {
+ this.required = required;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setCharset(Charset charset) {
+ this.charset = charset;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setExceptionPattern(String exceptionPattern) {
+ this.exceptionPattern = exceptionPattern;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setUseTLSMessageFormat(boolean useTLSMessageFormat) {
+ this.useTLSMessageFormat = useTLSMessageFormat;
+ return this;
+ }
+
+ public Rfc5424LayoutBuilder setLoggerFields(LoggerFields[] loggerFields) {
+ this.loggerFields = loggerFields;
+ return this;
+ }
+
+ public Rfc5424Layout build() {
+ if (includes != null && excludes != null) {
+ LOGGER.error("mdcIncludes and mdcExcludes are mutually exclusive. Includes wil be ignored");
+ includes = null;
+ }
+
+ if (ein != null && !ENTERPRISE_ID_PATTERN.matcher(ein).find()) {
Review Comment:
> I would use `matches()` instead of `find()`, since the entire enterprise id must match the regular expression.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #836: [LOG4J2-1376] Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871689461
##########
log4j-core/src/test/java/org/apache/logging/log4j/core/layout/Rfc5424LayoutTest.java:
##########
@@ -527,4 +532,61 @@ public void testParameterizedMessage() {
appender.stop();
}
}
+
+ @Test
+ void testLayoutBuilder() {
+ for (final Appender appender : root.getAppenders().values()) {
+ root.removeAppender(appender);
+ }
+
+ final AbstractStringLayout layout = new Rfc5424Layout.Rfc5424LayoutBuilder()
+ .setFacility(Facility.LOCAL0)
+ .setId("Event")
+ .setEin("36aaa92")
Review Comment:
Probably a typo, which will cause `layout` to be `null`.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] mneundorfer commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
mneundorfer commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871008105
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -634,11 +640,142 @@ public static Rfc5424Layout createLayout(
includes = null;
}
- return new Rfc5424Layout(config, facility, id, enterpriseNumber, includeMDC, newLine, escapeNL, mdcId,
+ return new Rfc5424Layout(config, facility, id, String.valueOf(enterpriseNumber), includeMDC, newLine, escapeNL, mdcId,
mdcPrefix, eventPrefix, appName, msgId, excludes, includes, required, StandardCharsets.UTF_8,
exceptionPattern, useTlsMessageFormat, loggerFields);
}
+ public static class Rfc5424LayoutBuilder {
Review Comment:
Please take a look if this Builder meets your expectations
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] mneundorfer commented on a diff in pull request #836: [LOG4J2-1376] Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
mneundorfer commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r873376193
##########
log4j-core/src/test/java/org/apache/logging/log4j/core/layout/Rfc5424LayoutTest.java:
##########
@@ -527,4 +532,61 @@ public void testParameterizedMessage() {
appender.stop();
}
}
+
+ @Test
+ void testLayoutBuilder() {
+ for (final Appender appender : root.getAppenders().values()) {
+ root.removeAppender(appender);
+ }
+
+ final AbstractStringLayout layout = new Rfc5424Layout.Rfc5424LayoutBuilder()
+ .setFacility(Facility.LOCAL0)
+ .setId("Event")
+ .setEin("36aaa92")
Review Comment:
Indeed! I fixed this and rebased the branch
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r869474731
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -610,8 +610,8 @@ public static Rfc5424Layout createLayout(
// @formatter:off
@PluginAttribute(value = "facility", defaultString = "LOCAL0") final Facility facility,
@PluginAttribute("id") final String id,
- @PluginAttribute(value = "enterpriseNumber", defaultInt = DEFAULT_ENTERPRISE_NUMBER)
- final int enterpriseNumber,
+ @PluginAttribute(value = "enterpriseNumber", defaultString = DEFAULT_ENTERPRISE_NUMBER)
+ final String enterpriseNumber,
Review Comment:
To change the type of the `enterpriseNumber` parameter, you would need to introduce a new factory method. The current practice in Log4j is to gradually migrate to builders: every time a new factory method would be required, we create a builder instead.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] mneundorfer commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
mneundorfer commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r868963040
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -610,8 +610,8 @@ public static Rfc5424Layout createLayout(
// @formatter:off
@PluginAttribute(value = "facility", defaultString = "LOCAL0") final Facility facility,
@PluginAttribute("id") final String id,
- @PluginAttribute(value = "enterpriseNumber", defaultInt = DEFAULT_ENTERPRISE_NUMBER)
- final int enterpriseNumber,
+ @PluginAttribute(value = "enterpriseNumber", defaultString = DEFAULT_ENTERPRISE_NUMBER)
+ final String enterpriseNumber,
Review Comment:
Deprecating this constructor and introducing a Builder feels like it could be a separate MR? In case I still should give it a shot, what do you mean by "validate" the parameter? What kind o f validation would you expect there?
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871104793
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/StructuredDataId.java:
##########
@@ -139,11 +139,30 @@ public StructuredDataId(final String name, final String enterpriseNumber, final
* @param enterpriseNumber The enterprise number.
* @param required The list of keys that are required for this id.
* @param optional The list of keys that are optional for this id.
+ * @deprecated Use {@link #StructuredDataId(String, String, String[], String[])} instead.
+ */
+ @Deprecated
+ public StructuredDataId(final String name, final int enterpriseNumber, final String[] required,
+ final String[] optional) {
+ this(name, String.valueOf(enterpriseNumber), required, optional, MAX_LENGTH);
+ }
+
+ /**
+ * A Constructor that helps conformance to RFC 5424.
+ *
+ * Deprecated:
+ * enterpriseNumber should be a String, use StructuredDataId(final String name, final String enterpriseNumber, final String[] required,
+ * final String[] optional, final int maxLength)
Review Comment:
The "deprecated" comment shouldn't be in the Javadoc of the other constructor?
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on a diff in pull request #836: Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
rgoers commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871586697
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -610,8 +610,8 @@ public static Rfc5424Layout createLayout(
// @formatter:off
@PluginAttribute(value = "facility", defaultString = "LOCAL0") final Facility facility,
@PluginAttribute("id") final String id,
- @PluginAttribute(value = "enterpriseNumber", defaultInt = DEFAULT_ENTERPRISE_NUMBER)
- final int enterpriseNumber,
+ @PluginAttribute(value = "enterpriseNumber", defaultString = DEFAULT_ENTERPRISE_NUMBER)
+ final String enterpriseNumber,
Review Comment:
Note that when implementing the builder the constructor it calls should be private.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #836: [LOG4J2-1376] Define eid as String instead of int to allow for Oid according to RFC5424
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r871689461
##########
log4j-core/src/test/java/org/apache/logging/log4j/core/layout/Rfc5424LayoutTest.java:
##########
@@ -527,4 +532,61 @@ public void testParameterizedMessage() {
appender.stop();
}
}
+
+ @Test
+ void testLayoutBuilder() {
+ for (final Appender appender : root.getAppenders().values()) {
+ root.removeAppender(appender);
+ }
+
+ final AbstractStringLayout layout = new Rfc5424Layout.Rfc5424LayoutBuilder()
+ .setFacility(Facility.LOCAL0)
+ .setId("Event")
+ .setEin("36aaa92")
Review Comment:
Probably a typo, which will cause `layout` to be `null`. This test is failing.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org