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

Set FAIL_ON_UNKNOWN_PROPERTIES in DefaultConfigurationFactoryFactory #1677

Merged
merged 2 commits into from Aug 6, 2016

Conversation

Projects
None yet
3 participants
@cakofony
Contributor

cakofony commented Aug 6, 2016

The logic is moved to DefaultConfigurationFactoryFactory from
YamlConfigurationFactory, making it easier to modify with a
custom ConfigurationFactoryFactory implementation.

There are some (uncommon) use cases where it's benefitial to
update an application without modifying the configuration, which
may fail if the application configuration has removed a configuration
property.

Carter Kozak
Set FAIL_ON_UNKNOWN_PROPERTIES in DefaultConfigurationFactoryFactory
The logic is moved to DefaultConfigurationFactoryFactory from
YamlConfigurationFactory, making it easier to modify with a
custom ConfigurationFactoryFactory implementation.

There are some (uncommon) use cases where it's benefitial to
update an application without modifying the configuration, which
may fail if the application configuration has removed a configuration
property.

@cakofony cakofony force-pushed the cakofony:bugfix/config_allow_ignore_unknown_properties branch from 4d00ccc to a3bbd17 Aug 6, 2016

@coveralls

This comment has been minimized.

coveralls commented Aug 6, 2016

Coverage Status

Coverage increased (+0.003%) to 82.398% when pulling a3bbd17 on cakofony:bugfix/config_allow_ignore_unknown_properties into ab8eb67 on dropwizard:master.

@coveralls

This comment has been minimized.

coveralls commented Aug 6, 2016

Coverage Status

Coverage increased (+0.003%) to 82.398% when pulling a3bbd17 on cakofony:bugfix/config_allow_ignore_unknown_properties into ab8eb67 on dropwizard:master.

@evnm

This comment has been minimized.

Member

evnm commented Aug 6, 2016

I agree that this makes sense as something that the library should offer a means of overriding.

Instead of simply moving the location of the hardcoded ObjectMapper#enable call, what if we added a protected ObjectMapper configureObjectMapper(ObjectMapper) method to DefaultConfigurationFactoryFactory? That way, consumers could enable ObjectMapper functionality without having to touch the create method.

@cakofony

This comment has been minimized.

Contributor

cakofony commented Aug 6, 2016

Thanks for the quick feedback! I implemented your suggestion and added testing.
I'm not sure how to run the style tests locally, I might have to re-push in a couple minutes

Carter Kozak
Add protected DefaultConfigurationFactoryFactory.configureObjectMapper
This method allows the configuration factory to be extended to
apply (or remove) features for configuration deserialization.

@cakofony cakofony force-pushed the cakofony:bugfix/config_allow_ignore_unknown_properties branch from c8bd9b0 to a92c1ca Aug 6, 2016

@coveralls

This comment has been minimized.

coveralls commented Aug 6, 2016

Coverage Status

Coverage increased (+0.003%) to 82.398% when pulling a92c1ca on cakofony:bugfix/config_allow_ignore_unknown_properties into ab8eb67 on dropwizard:master.

@coveralls

This comment has been minimized.

coveralls commented Aug 6, 2016

Coverage Status

Coverage increased (+0.003%) to 82.398% when pulling a92c1ca on cakofony:bugfix/config_allow_ignore_unknown_properties into ab8eb67 on dropwizard:master.

*
* @param objectMapper template to be configured
* @return configured object mapper
*/

This comment has been minimized.

@evnm

evnm Aug 6, 2016

Member

A+ Javadoc, would read again

@evnm evnm added this to the 1.0.1 milestone Aug 6, 2016

@evnm evnm added the improvement label Aug 6, 2016

@evnm evnm merged commit 1e40fef into dropwizard:master Aug 6, 2016

arteam added a commit that referenced this pull request Aug 9, 2016

Set FAIL_ON_UNKNOWN_PROPERTIES in DefaultConfigurationFactoryFactory (#…
…1677)

ObjectMapper-customization logic is moved from YamlConfigurationFactory
to DefaultConfigurationFactoryFactory, making it easier to modify with a
custom ConfigurationFactoryFactory implementation.

There are some (uncommon) use cases where it's beneficial to update an
application without modifying the configuration, which may fail if the
application configuration has removed a configuration property.

A protected configureObjectMapper method is added to
DefaultConfigurationFactoryFactory to allow the configuration
factory to be extended to apply (or remove) deserialization
features.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment