Allow unknown properties by default in YAML config

[bowsersenior]

Problem:

Currently, the JSON configuration parser allows unknown properties by default (but can be configured to not allow them), but for some reason, the YAML configuration parser behaves differently. From the current dropwizard docs:

The YAML configuration parser will fail on unknown properties regardless of the object mapper configuration.

In the issue & PR that changed the default for JSON configs to allow unknown properties, I couldn't find any explanation for why YAML behavior was not similarly updated.

Allowing unknown properties by default are desirable in YAML config files for the same reasons listed for allowing them by default in JSON config files:

• Most, if not all, Dropwizard projects that I’ve seen disables it.
• It follows Postel’s law (be conservative in what you send, be liberal in what you accept).
• All other JSON libraries that I know allows unknown properties by default: GSON, the json package in Go and Json.NET. Spring Boot (which uses Jackson) also disables it by default.

Solution:

Makes YAML config file behavior consistent with that of JSON

Result:

By default, an unknown property in a config.yml file will no longer cause an exception to be thrown when loaded.

[joschi]

@bowsersenior Thanks for your contribution!

I disagree with the premise, though. In my opinion, Dropwizard should fail-fast if the configuration files contain unexpected strings.

I've seen it more than once that people thought they had a valid configuration file which contained a small typo so that some settings weren't really configured and which only was detected by a misbehaving service in production.

For JSON payloads received via network, Dropwizard should follow the Robustness principle:

be conservative in what you do, be liberal in what you accept from others

This is the reason why FAIL_ON_UNKNOWN_PROPERTIES is enabled in the ObjectMapper used to parse the configuration files and the reason it is disabled in the ObjectMapper instance used for serializing and deserializing data received over network.

[bowsersenior]

Hi @joschi , thanks for your comments and welcome!

The main issue is that the current default behavior is inconsistent. If you use JSON, unknown properties are allowed by default but for if you use YAML, then unknown properties throw an error by default. Not sure if you had a chance to look at #2570 which changed the default behavior for JSON only.

As to the which default is better, I think defaulting to not throwing an error is often the more practical option, since in most systems (eg. env-var based configs), extra configs are ignored or just logged. I think the extra strictness should be an opt-in, but I can see things from your perspective as well and there are other systems that don't allow unknown properties (eg. many newer CLIs don't let you set extra params to avoid mistakes). There is a use case for setting extra params if you use an external system to set system properties. In this case, it is helpful to decouple the properties from the config file, allowing independent updates to either the application JAR which contains the config file, or the system properties which are set on deployment. This would make deployment more flexible.

[bowsersenior]

Another thing to consider is that there are no docs on how to change this default config for the YAML config parser. The current DefaultConfigurationFactoryFactory.configureObjectMapper() code will always enable FAIL_ON_UNKNOWN_PROPERTIES, even if it was explicitly set to disabled in the initialize(). If there is another simple way to change the default behavior for YAML config files, it should be added to the docs.

[pstackle]

I strongly agree with @joschi on this issue. Dropwizard is an opinionated framework and one of those opinions is that the application should fail-fast if the configuration file is not valid.

The main issue is that the current default behavior is inconsistent. If you use JSON, unknown properties are allowed by default but for if you use YAML, then unknown properties throw an error by default.

I disagree that the behavior is inconsistent. You are suggesting that the delineation is on JSON vs YAML, but in fact it is different behavior for configuration parsing vs JAX-RS API parsing. For example, you can technically have your configuration file use a JSON format instead of YAML, in which case, it will still fail if the configuration is deemed to be invalid.

The arguments for changing the API parsing behavior were compelling because as an API evolves, you need to be able to maintain compatibility between the service API and any clients. When the previous behavior of failing on unknown properties was in place, it made it harder to evolve an app's API and allow for more freedom for the server and client versions to vary while still allowing things to function correctly. The same justifications for changing the API parsing behavior do not hold true for configuration parsing.

Another thing to consider is that there are no docs on how to change this default config for the YAML config parser.

In your PR, you specifically are deleting a line from the docs that mentions the current config parsing behavior. I think the use case of disabling this config validation is not recommended or something that we should specifically document how to achieve.

Again, while I don't recommend doing what you are trying to do, if you really want to change the behavior for your app, I can think of a couple options:

1. have you tried defining the @JsonIgnoreProperties(ignoreUnknown = true) annotation on your app's configuration class? I think there is a chance that the annotation on your app's specific configuration class would override the ObjectMapper setting.
2. if that doesn't work, you could set your own ConfigurationFactoryFactory on the app's Bootstrap instance in the app's initialize() method. This could be as simple as a class that extends from DefaultConfigurationFactoryFactory and overrides the configureObjectMapper() method.

[bowsersenior]

Thanks for clarifying. I had assumed that this was an inconsistency, based on my background with spring where the similar feature of ignoreUnknownFields is not often used and marked as deprecated.

I'll close the issue and try your suggestions. But let me clarify my use case as I don't think you have considered them. I did not want to change this config validation behavior to support mistaken or invalid config file entries, but to allow flexibility in independently updating and rolling back config & code.

If you follow the 12-Factor App pattern of strictly separating config from code, then you will need some flexibility between configuration classes and passed config values, similar to the use case between clients and APIs.

Let's say I am adding a new feature X in code version N that has a configuration A. While rolling out this feature, the config file may first be updated to include A before the code is updated with feature X, or vice-versa. Requiring a simultaneous update of code & config introduces an unnecessary coupling that is impractical at scale.

It is also possible for a code rollback to occur after the code is deployed with feature X. In such cases, the strictness of disallowing an unknown config can interfere with the flexibility of deployment and separation between config & code.