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

added JsonConfigurationFactory to support all JSON files #1897

Merged
merged 9 commits into from Jan 30, 2017

Conversation

Projects
None yet
5 participants
@manuel-hegner
Contributor

manuel-hegner commented Jan 19, 2017

changed YAMLConfigurationFactory to support JSON files with different whitespaces

added JSON version of ConfigurationFactoryTest

fixes #1896

changed YAMLConfigurationFactory to support JSON files with different…
… whitespaces

added JSON version of ConfigurationFactoryTest

@arteam arteam force-pushed the dropwizard:master branch from 6668cfa to d98bfe5 Jan 21, 2017

@arteam

This comment has been minimized.

Member

arteam commented Jan 21, 2017

Thank you for your interest in contributing to Dropwizard! We would be interested in the first-class support for JSON configuration files. But, I believe such support should be provided as a separate configuration factory rather than as a fix for YamlConfigurationFactory. Dropwizard is opinionated about its config format and I would prefer maintain YamlConfigurationFactory as a parser strictly for YAML files. Despite JSON is a subset of YAML and, in theory, should be parsed it, we can't guarantee such compatibility. I think your original solution with a JSON configuration factory is a right way to go with this feature. At least, in this format it can be accepted.

@arteam arteam added the feature label Jan 21, 2017

@manuel-hegner manuel-hegner changed the title from changed YAMLConfigurationFactory to support all JSON files to added JsonConfigurationFactory to support all JSON files Jan 24, 2017

@coveralls

This comment has been minimized.

coveralls commented Jan 24, 2017

Coverage Status

Changes Unknown when pulling 6b6dfba on manuel-hegner:configuration_full_json_support into ** on dropwizard:master**.

@manuel-hegner

This comment has been minimized.

Contributor

manuel-hegner commented Jan 26, 2017

Okay, I did this as a new ConfigurationFactory now. The remaining codeclimate issues should be all false positives now.

*
* @param <T> the type of the configuration objects to produce
*/
public class JsonConfigurationFactory<T> implements ConfigurationFactory<T> {

This comment has been minimized.

@arteam

arteam Jan 27, 2017

Member

Very good! But I think we could avoid duplication by extending JsonConfigurationFactory from YamlConfigurationFactory. We could expose the getParser method in YamlConfigurationFactory. The method will create a parser from an input stream. JsonConfigurationFactory can override this method to produce a JsonParser instead of a YAMLParser. I had something like that in mind:

public class JsonConfigurationFactory<T> extends YamlConfigurationFactory<T> {

    private final JsonFactory jsonFactory = new JsonFactory();

    public JsonConfigurationFactory(Class<T> klass,
                                    Validator validator,
                                    ObjectMapper objectMapper,
                                    String propertyPrefix) {
        super(klass, validator, objectMapper, propertyPrefix);
    }

    @Override
    protected JsonParser getParser(InputStream input) throws IOException {
        return jsonFactory.createParser(input);
    }

    @Override
    public T build(ConfigurationSourceProvider provider, String path) throws IOException, ConfigurationException {
        try {
            return super.build(provider, path);
        } catch (JsonParseException e) {
            throw ConfigurationParsingException
                .builder("Malformed JSON")
                .setCause(e)
                .setLocation(e.getLocation())
                .setDetail(e.getMessage())
                .build(path);
        }
    }
}

YamlConfigurationFactory

protected JsonParser getParser(InputStream input) throws IOException {
        return yamlFactory.createParser(input);
    }

    @Override
    public T build(ConfigurationSourceProvider provider, String path) throws IOException, ConfigurationException {
        try (InputStream input = provider.open(requireNonNull(path))) {
            final JsonNode node = mapper.readTree(getParser(input));
        //...
}

This comment has been minimized.

@mattnelson

mattnelson Jan 27, 2017

Contributor

It feels wrong to have a json extend yaml. What about creating a base factory that both yaml and json extend?

This comment has been minimized.

@arteam

arteam Jan 27, 2017

Member

It feels wrong to have a json extend yaml

I also thought about that, should be other way around. The base factory approach looks even better to me.

}

@Override
public void printsDetailedInformationOnMalformedYaml() throws Exception {

This comment has been minimized.

@arteam

arteam Jan 27, 2017

Member

I guess we could name the test printsDetailedInformationOnMalformedJson.

@@ -0,0 +1,3 @@
{
"name": "Boop"
}

This comment has been minimized.

@arteam

arteam Jan 27, 2017

Member

Just a nitpick, could you please add a newline symbol to the end of JSON files? That way they're displayed correctly in terminal windows.

This comment has been minimized.

@manuel-hegner

manuel-hegner Jan 30, 2017

Contributor

I think I did that now but I am not sure if my GIT client removed them again. The github view is confusing for these cases.

"propertis": {
"admin": true
},
"servers": [

This comment has been minimized.

@arteam

arteam Jan 27, 2017

Member

Very nice that you use tabs to verify that we can parse any JSON file.

manuel-hegner added some commits Jan 30, 2017

* JsonConfigurationFactory and YamlConfigurationFactory now both exte…
…nd a BaseConfigurationFactory

* did the same for the tests
* added line break at the end of the test JSON files
@coveralls

This comment has been minimized.

coveralls commented Jan 30, 2017

Coverage Status

Changes Unknown when pulling 20a99d8 on manuel-hegner:configuration_full_json_support into ** on dropwizard:master**.

@coveralls

This comment has been minimized.

coveralls commented Jan 30, 2017

Coverage Status

Changes Unknown when pulling 20a99d8 on manuel-hegner:configuration_full_json_support into ** on dropwizard:master**.

@arteam arteam merged commit ec264da into dropwizard:master Jan 30, 2017

@arteam

This comment has been minimized.

Member

arteam commented Jan 30, 2017

@manuel-hegner Thank you very much for the contribution and your persistence in addressing feedback!

@manuel-hegner

This comment has been minimized.

Contributor

manuel-hegner commented Jan 30, 2017

No problem. It is a pleasure to work for an open source project with a contribution process that works so well.

@jplock jplock added this to the 1.1.0 milestone Jan 30, 2017

arteam added a commit that referenced this pull request Jan 30, 2017

@manuel-hegner manuel-hegner deleted the manuel-hegner:configuration_full_json_support branch Feb 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment