Support optional: prefix with logging.log4j2.config.override

[rgoers]

Log4j2LoggingSystem contains the following method:

	protected void loadConfiguration(String location, LogFile logFile, List<String> overrides) {
		Assert.notNull(location, "'location' must not be null");
		try {
			List<Configuration> configurations = new ArrayList<>();
			LoggerContext context = getLoggerContext();
			configurations.add(load(location, context));
			for (String override : overrides) {
				configurations.add(load(override, context));
			}
			Configuration configuration = (configurations.size() > 1) ? createComposite(configurations)
					: configurations.iterator().next();
			context.start(configuration);
		}
		catch (Exception ex) {
			throw new IllegalStateException("Could not initialize Log4J2 logging from " + location, ex);
		}
	}

If an error occurs loading an override file the exception will percolate to the catch block and fail loading the logging configuration. The log4j-spring-boot module provided by Log4j had:

                        try {
                            final Configuration config =
                                    ConfigurationFactory.getInstance().getConfiguration(ctx, source);
                            if (config instanceof AbstractConfiguration) {
                                configs.add((AbstractConfiguration) config);
                            } else {
                                LOGGER.warn(
                                        "Configuration at {} cannot be combined in a CompositeConfiguration",
                                        sourceLocation);
                                return;
                            }
                        } catch (Exception ex) {
                            if (!first) {
                                LOGGER.warn(
                                        "Error accessing {}: {}. Ignoring override", sourceLocation, ex.getMessage());
                            } else {
                                throw ex;
                            }
                        }

So with log4j-spring-boot a warning message would be logged that the override file couldn't be loaded but configuration would still take place with the primary config file.

This behavior in Log4j2 was intentional. All the services I work with specify a shared primary logging configuration and a per application override location such as

logging:
  label: ${spring.cloud.config.label:master}
  config: "https://spring-configuration-server.acme.org/some-service/default/${logging.label}/log4j2-json-dev.xml"
  log4j2:
    config:
      override: "https://spring-configuration-server.acme.org/some-service/default/${logging.label}/log4j2-some-service-dev.xml"

If the application doesn't need to override anything then it just uses the primary configuration. If we want to add an override we can just add it to Spring Cloud Config without having to change the application.

In short, the configurations.add() call needs to be placed in its own try/catch block that logs a similar message to what log4j-spring-boot was doing and ignore the failure.

[nosan]

In short, the configurations.add() call needs to be placed in its own try/catch block
that logs a similar message to what log4j-spring-boot was doing and ignore the failure.

I am not sure that is a good way to go, there could be a lot of people who rely on
current behaviour, and adding try { } catch (...) { } will introduce breaking change.

I believe a good compromise would be to add support for the optional: prefix. This approach won't impact those who currently using override files and expecting an Exception to be thrown, while also offering the flexibility to include optional override files. If the file location is prefixed with optional: and does not exist, it will not be loaded.

logging:
  label: ${spring.cloud.config.label:master}
  config: "https://spring-configuration-server.acme.org/some-service/default/${logging.label}/log4j2-json-dev.xml"
  log4j2:
    config:
      override: "optional:https://spring-configuration-server.acme.org/some-service/default/${logging.label}/log4j2-some-service-dev.xml"

I've added optional: support in main...nosan:spring-boot:gh-44399

[rgoers]

I would expect that many users using this feature were using log4j-spring-boot, so removing that jar exposes this behavior. IOW, the breaking change was done in Spring Boot. However, I am open to making a change similar to what you propose but I don't think changing the URI string is the appropriate way to do it. I would expect it to be something like:

logging:
  label: ${spring.cloud.config.label:master}
  config: "https://spring-configuration-server.acme.org/some-service/default/${logging.label}/log4j2-json-dev.xml"
  log4j2:
    config:
      optional-override: "https://spring-configuration-server.acme.org/some-service/default/${logging.label}/log4j2-some-service-dev.xml"

or

logging:
  label: ${spring.cloud.config.label:master}
  config: "https://spring-configuration-server.acme.org/some-service/default/${logging.label}/log4j2-json-dev.xml"
  log4j2:
    config:
      optional:
        override: "https://spring-configuration-server.acme.org/some-service/default/${logging.label}/log4j2-some-service-dev.xml"

[wilkinsona]

There's support elsewhere in Boot for using optional: as a prefix. If we go down the route of wanting to indicate that the configuration is optional, @nosan's suggestion would be the idiomatic of doing it in Boot.

[wilkinsona]

Reading through the issue again, I think it's important to be able to distinguish between an override that's present yet broken and an override that's not there. I think the former should cause a failure while the latter should not. This would be consistent with, for example, our handling of application.yaml where an app will start without one but will fail to start if the file's present but its syntax is invalid.

[nosan]

Agreed, that's why I chose not to include a try {} catch(...) {} block and instead added checks only for Resource.exists().

[rgoers]

If using optional: as a prefix is the approved way of handling this kind of thing in Spring then I am fine with the proposed solution.

[nosan]

I think the former should cause a failure while the latter should not. This would be consistent with, for example, our handling of application.yaml where an app will start without one but will fail to start if the file's present but its syntax is invalid.

Unfortunately, it is not possible to have the same behaviour with Log4j2. If the configuration file is invalid, Log4j2 does not throw an exception.

@Test
@WithNonDefaultXmlResource
@WithResource(name = "override.xml", content = """
		<?xml version="1.0" encoding="UTF-8"?>
		<Configuration status="WARN"
		""")
void loadOptionalOverrideConfigurationWhenConfigurationIsInvalid() {
	this.environment.setProperty("logging.log4j2.config.override", "optional:classpath:override.xml");

        //Exception is not thrown here. 
	assertThatException().isThrownBy(
			() -> this.loggingSystem.initialize(this.initializationContext, "classpath:nondefault.xml", null));
}

XmlConfiguration
https://github.com/apache/logging-log4j2/blob/d8cbe77a0a1f60e6708a2bd8f84ea2fb8666c95a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java#L136-L138

JsonConfiguration/YamlConfiguration
https://github.com/apache/logging-log4j2/blob/d8cbe77a0a1f60e6708a2bd8f84ea2fb8666c95a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/json/JsonConfiguration.java#L97-L99

BuiltConfiguration/PropertiesConfiguration
https://github.com/apache/logging-log4j2/blob/d8cbe77a0a1f60e6708a2bd8f84ea2fb8666c95a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/builder/impl/BuiltConfiguration.java#L122-L129