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.

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

Expanded more elements to Log4j-config.xsd #1441

Closed
wants to merge 1 commit into from

Conversation

krallus
Copy link

@krallus krallus commented Apr 22, 2023

While certainly not a complete addition of all missing appenders, or all of the possible configuration of those appenders, I added some of the more common ones that I use, including: ScriptAppenderSelector, RollingFile, File, and Null. I also added the charset attribute to PatternLayout and the level attribute to AppenderRef.

Please ignore whitespace when comparing. The previous version had some elements that did not line up and so I simply reformatted the whole thing.

Note that I edited the file in place and did not run any tests. I'm making the assumption that this file is here for convenience and no tests actually rely on it.

While certainly not a complete addition of all missing appenders, or all of the possible configuration of those appenders, I added some of the more common ones, including: ScriptAppenderSelector, RollingFile, File, and Null.  I also added the charset attribute to PatternLayout and the level attribute to AppenderRef.
@ppkarwasz
Copy link
Contributor

@krallus,

thanks for the PR. This is a very old outstanding bug (cf. LOG4J2-170).

Actually the main branch has a much more complete XSD schema. Can you also add the improvements from that version to your PR?

@ppkarwasz
Copy link
Contributor

@krallus,

I have backported the Log4j-config.xsd from master in #1667 . Can you check if you can validate your configuration with it?

@ppkarwasz ppkarwasz closed this Aug 6, 2023
@krallus
Copy link
Author

krallus commented Jul 24, 2024

@krallus,

I have backported the Log4j-config.xsd from master in #1667 . Can you check if you can validate your configuration with it?

Indeed I can. Thank you. (sorry for such a long delay in replying)

@ppkarwasz
Copy link
Contributor

@krallus,

A couple of months ago we published an automatically generated XML Schema for all Log4j 2 and 3 module. You can find it at: https://logging.apache.org/xml/ns/

The schema is generated using a new internal tool (log4j-docgen-maven-plugin) that can combine metadata from multiple Log4j Plugin modules into one XML Schema. The tool is still pretty much rough around the edges, but in the future will allow to generate a different XML Schema for any set of Log4j Plugin JARs (even from third-parties).

@krallus
Copy link
Author

krallus commented Aug 6, 2024

Thank you. I ran into a some problems using the new one at https://logging.apache.org/xml/ns/.
Here is a simple log4j2.xml:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE Configuration>
<Configuration 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-config-2.xsd"
	status="WARN">
	<Properties>
		<Property name="root.logging.level">${sys:myapp.root.logging.level:-${sys:rootLogger.level:-WARN}}</Property>
	</Properties>
	<Appenders>
		<Console name="Console" target="SYSTEM_OUT">
			<PatternLayout pattern="%date{ABSOLUTE} %class %method - %message%n%throwable" />
		</Console>
	</Appenders>
	<Loggers>
		<Root level="${root.logging.level}">
			<AppenderRef ref="Console" />
		</Root>
	</Loggers>
</Configuration>

The new XSD incorrectly points out two problems:

  1. It doesn't like the way I've set the value for a Property: as content. However, that is one of the ways shown in the example at https://logging.apache.org/log4j/2.x/manual/configuration.html#concise-syntax. Currently, it will only validate if I set the value using the value attribute.
  2. More significantly, it does not accept "${root.logging.level}" for Root's level, which is using Property Substitution.

Note that the use case for the way I've written the above configuration is that if I start my Java Virtual Machine with -Dmyapp.root.logging.level=DEBUG (or some other level), it will use that for the root logger if it is set, otherwise it defaults to WARN.

@ppkarwasz
Copy link
Contributor

The new XSD incorrectly points out two problems:

1. It doesn't like the way I've set the value for a Property: as content. However, that is one of the ways shown in the example at https://logging.apache.org/log4j/2.x/manual/configuration.html#concise-syntax. Currently, it will only validate if I set the value using the `value` attribute.

This is a limitation of the current log4j-docgen version: it handles @PluginValue annotation the same way as @PluginAttribute. You can report it in the logging-log4j-tools repo.

2. More significantly, it does not accept "${root.logging.level}" for Root's `level`, which is using [Property Substitution](https://logging.apache.org/log4j/2.x/manual/configuration.html#PropertySubstitution).

The generated schema doesn't play well with arbiters or property substitution expressions:

  • The former can be placed at any point in the XML tree. We could introduce support for them, but then we can not specify any restrictions on the content of an arbiter.
  • The latter can be placed in any XML attribute. We could declare each attribute as xsd:string, but this would IMHO strongly reduce the utility of the XSD. Right now a good XML editor can suggest you the type and possible values of most attributes (e.g. you don't need to remember the possible values of the target attribute of the console appender).

We could add an option to log4j-docgen-maven-plugin to use xsd:string for all attribute, but we wouldn't use the option in the XSD schemas available on our site. @vy, what do you think?

@vy
Copy link
Member

vy commented Aug 8, 2024

We could add an option to log4j-docgen-maven-plugin to use xsd:string for all attribute, but we wouldn't use the option in the XSD schemas available on our site.

I would not be in favor of this change. This will practically remove the IDE guidance provided using the XSD, which IMHO, is the point of publishing an XSD in our case. OTOH, I see the point that level="${foo}" is syntactically invalid according to the XSD. If IDE could have recognized log4j2.xml syntax (just like it does for Spring Boot external configuration files), I doubt if @krallus will still be complaining. 🤔

@krallus, are you trying to (manually) validate your log4j2.xml against the XSD, or you're only concerned about IDE complaints?

@krallus
Copy link
Author

krallus commented Aug 8, 2024

Yes, my IDE flags the XML as having errors if I use the XSD at https://logging.apache.org/xml/ns/log4j-config-2.xsd when I have any property substitution. I think property substitution in Log4j XML configurations is probably quite common. I do it in many places. I just provided a simple example. Personally, I would be in favour of making all attributes be of type xsd:string. Not only is there property substitution to consider, but there may be other valid values that the XSD simply will not be aware of. For example, in the case of "level", the documentation in the XSD itself has documentation, "NOTE: The Log4j API supports custom levels, the following list contains only the standard ones.". There is still great utility in having an XSD both validate the structure of all of the elements and required attributes, as well as facilitating XML authoring assistance in an IDE, but just not for attribute values.

However, there is a way to define an XSD that will grant both IDE guidance for attribute values and allow for any string at the expense of slightly more sophistication in the XSD. Here is an oversimplified example just to demonstrate how this can be done.

Example XSD file that allows a list for IDE guidance but will also validate any string. Note the "levelOrSubstitution" type:

<xs:schema xmlns:xs="http://www.w3.org/2001/XMLSchema" elementFormDefault="qualified">

	<xs:simpleType name="levelOrSubstitution">
		<xs:union>
			<xs:simpleType>
				<xs:restriction base="xs:string">
					<xs:enumeration value="TRACE" />
					<xs:enumeration value="DEBUG" />
					<xs:enumeration value="INFO" />
					<xs:enumeration value="WARN" />
				</xs:restriction>
			</xs:simpleType>
			<xs:simpleType>
				<xs:restriction base="xs:string">
					<xs:pattern value=".+" />
				</xs:restriction>
			</xs:simpleType>
		</xs:union>
	</xs:simpleType>

	<!-- Root element definition -->
	<xs:element name="Configuration">
		<xs:complexType>
			<xs:sequence>
				<xs:element name="Root">
					<xs:complexType>
						<xs:sequence>
							<xs:element name="AppenderRef">
								<xs:complexType>
									<xs:attribute name="ref" type="xs:string" use="required" />
								</xs:complexType>
							</xs:element>
						</xs:sequence>
						<xs:attribute name="level" type="levelOrSubstitution" use="required" />
					</xs:complexType>
				</xs:element>
			</xs:sequence>
		</xs:complexType>
	</xs:element>

</xs:schema>

Valid XML File:

<?xml version="1.0" encoding="UTF-8"?>
<Configuration xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	xsi:noNamespaceSchemaLocation="testconfig.xsd">
	<Root level="${sys:myapp.root.logging.level:-${sys:rootLogger.level:-WARN}}">
		<AppenderRef ref="FileRfc5424Inspired" />
	</Root>
</Configuration>

Eclipse IDE guidance when filling out the XML:
(Ignore the first grey line as it is simply an informative artifact that did not refresh after I renamed the XSD)
Eclipse IDE guidance

Note that for the xs:pattern, I considered using a pattern that would match only on ${...} instead of any string, but that is not general enough. Consider fileName="knownPath/${someDerivedName}.ext".

I hope that helps some.

@krallus
Copy link
Author

krallus commented Aug 8, 2024

@ppkarwasz , I will report the @PluginValue as you mentioned. I've also noticed a few other problems with the XSD but they are more specific to a particular element or attribute. Where shall I report such issues, or is it that there are missing annotations in the log4j base code? The only other thing I've observed at the moment is that the additivity attribute is missing from the XSD's <complexType name="org.apache.logging.log4j.core.config.LoggerConfig">.

@ppkarwasz
Copy link
Contributor

@krallus,

Thanks, I have opened apache/logging-log4j-tools#135 and apache/logging-log4j-tools#134 to implement your suggestions.

@vy
Copy link
Member

vy commented Aug 9, 2024

@krallus, loved the <xs:union> suggestion! 😍
Thanks so much! 🙇
Please keep these contributions coming. 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants