Offering extended configuration #79

ajs6f · 2018-03-21T17:41:34Z

Adds configuration for use with Trellis-Cassandra.

acoburn · 2018-03-21T17:49:57Z

This seems fine, but I wonder if it wouldn't be better for the trellis-over-cassandra-app to subclass TrellisConfiguration and add these methods there?

ajs6f · 2018-03-21T17:53:29Z

You tell me-- I know as much about Dropwizard as I do about playing poker: nothing. Should I close this and try that instead?

ajs6f · 2018-03-21T17:57:51Z

Nope, Java's bottom-of-the-barrel variance. TrellisApplication extends Application<TrellisConfiguration>, and TrellisCassandraApplication extends TrellisApplication, so I don't think I can also somehow TrellisCassandraApplication extends Application<TrellisCassandraConfiguration>. Or maybe I'm missing something?

ajs6f · 2018-03-21T17:59:39Z

Sorry-- misclick.

acoburn · 2018-03-21T18:07:11Z

A better approach might be to add a @JsonAnySetter method to TrellisConfiguration:

@JsonAnySetter
public void set(final String name, final String value) {
    internalMap.put(name, value);
}

@JsonAnyGetter
public Map<String, String> any() {
    return internalMap;
}

That adds a simple extension point for any possible subclasses.

ajs6f · 2018-03-21T18:19:44Z

Where is internalMap? Is that to be added as well?

acoburn · 2018-03-21T18:20:26Z

Yes, it's just some internal field. Call it whatever you'd like.

ajs6f · 2018-03-21T18:23:18Z

I'm not much with JSON; what about Map<String, Object>? Would that effectively account for nesting? I.e. with

@JsonAnySetter
public void set(final String name, final Object value) {
    internalMap.put(name, value);
}

@JsonAnyGetter
public Map<String, Object> any() {
    return internalMap;
}

acoburn · 2018-03-21T18:27:38Z

I haven't ever used that precise construct with Jackson serializations, but I don't see why it wouldn't work. It should be trivial to set up a test -- just add some additional fields to one of the configuration files in ./src/test/resources

ajs6f · 2018-03-21T18:34:56Z

Didn't work at all, not even with simple Strings:

io.dropwizard.configuration.ConfigurationParsingException: /Users/ajs6f/Documents/trellis/trellis/trellis-app/bin/test/config1.yml has an error:
  * Unrecognized field at: cassandraAddress
    Did you mean?:
      - assets
      - resources
      - namespaces
      - cacheMaxAge
      - binaries
        [12 more]

So maybe that works for Jackson's semantics, but not for Dropwizard's particular concrete processing code?

ajs6f · 2018-03-21T18:36:17Z

Again, I know little about Dropwizard, but I'm beginning to suspect that we're running up against its intentional limits-- it seems to be for building single apps, not ecosystems. Extensible config is really not a concern if you are build a single app.

ajs6f · 2018-03-21T19:09:18Z

Hang on-- I had a misconfig in my code. Testing further...

ajs6f · 2018-03-21T19:18:26Z

Okay, how's that?

ajs6f · 2018-03-21T19:28:31Z

incidentally, I tried going:

public class TrellisApplication extends Application<TrellisConfiguration> {

=>

public class TrellisApplication<T extends TrellisConfiguration> extends Application<T> {

but all of the test classes exploded with:

The constructor DropwizardTestSupport<TrellisConfiguration>(Class<TrellisApplication>, String, ConfigOverride, ConfigOverride, ConfigOverride) is undefined

Not really sure what's going on there, but I think the issue is that Class<TrellisApplication> isn't provably a supertype of Class<TrellisApplication<T extends TrellisConfiguration>> (or the other way 'round), so the compiler pukes into its own shirt. Shall we just rewrite the whole app in Scala?

christopher-johnson · 2018-03-21T20:24:48Z

@ajs6f I tried running the test and it works for me using IntelliJ. One side note is that the @RunWith(JUnitPlatform.class) is not required since the test class is written for JUnit5. AFAIK, that annotation invokes the JUnit4 runner.

ajs6f · 2018-03-21T20:26:56Z

Thanks much @christopher-johnson ! I didn't actually write that test, just added a few limes to it, so I'll let @acoburn remove that annotation. Unless, @acoburn , you'd like me to do it here?

acoburn

This is great. There are some trivial checkstyle-related issues to be fixed before merging

acoburn · 2018-03-22T13:34:23Z

trellis-app/src/main/java/org/trellisldp/app/config/TrellisConfiguration.java


+import static java.util.Collections.synchronizedMap;
+
+import java.util.Collections;


This isn't used and can be removed.

acoburn · 2018-03-22T13:34:47Z

trellis-app/src/main/java/org/trellisldp/app/config/TrellisConfiguration.java


 import io.dropwizard.Configuration;

+import static java.util.Collections.synchronizedMap;


Move static imports to the top (before ordinary imports)

acoburn · 2018-03-22T13:35:02Z

trellis-app/src/main/java/org/trellisldp/app/config/TrellisConfiguration.java

    private String baseUrl = null;

    private String resourceLocation = null;
+


trailing whitespace

acoburn · 2018-03-22T13:35:16Z

trellis-app/src/test/java/org/trellisldp/app/config/TrellisConfigurationTest.java

+        assertEquals("my.cluster.node", config.any().get("cassandraAddress"));
+        assertEquals((Integer)245993, config.any().get("cassandraPort"));
+        @SuppressWarnings("unchecked")
+        Map<String, Object> extraConfig = (Map<String, Object>) config.any().get("extraConfigValues");


add final here.

acoburn · 2018-03-22T13:35:24Z

trellis-app/src/test/java/org/trellisldp/app/config/TrellisConfigurationTest.java

+        Map<String, Object> extraConfig = (Map<String, Object>) config.any().get("extraConfigValues");
+        assertTrue((Boolean) extraConfig.get("one"));
+        @SuppressWarnings("unchecked")
+        List<String> list = (List<String>) extraConfig.get("two");


add final

ajs6f · 2018-03-22T13:37:51Z

I will fix those probs in two seconds, but just so you know-- I ran ./gradlew check install to check this and it passed fine. Do we need to couple checkstyleMain into check? I thought it already was?

acoburn · 2018-03-22T13:40:51Z

That's odd. It could be that running this without clean means that the changes aren't caught. I definitely get the checkstyle errors when running ./gradlew clean check install

ajs6f · 2018-03-22T13:57:44Z

Okay, I'll keep an eye peeled. I might try making a few intentional faux pas and see which Gradle invocations catch them and which do not.

ajs6f closed this Mar 21, 2018

ajs6f reopened this Mar 21, 2018

ajs6f changed the title ~~Offering Cassandra cluster metadata from configuration~~ Offering extended configuration Mar 21, 2018

ajs6f mentioned this pull request Mar 21, 2018

Make getResourcePath overridable for build systems other than Gradle #81

Merged

ajs6f added 2 commits March 22, 2018 09:30

Offering Cassandra cluster metadata from configuration

7a6abb6

Better arrangements for extended configuration

83ab1a1

ajs6f force-pushed the CassConfig branch from 99ddfe0 to 83ab1a1 Compare March 22, 2018 13:33

acoburn reviewed Mar 22, 2018

View reviewed changes

Correcting formatting

c36b9fb

acoburn merged commit 050d873 into trellis-ldp:master Mar 22, 2018

acoburn pushed a commit that referenced this pull request Mar 22, 2018

Offering extended configuration (#79)

486d1e8

christopher-johnson mentioned this pull request Apr 7, 2018

JUnitPlatform causes ClassCastException in Gradle testCompile with JPMS #96

Closed


		import static java.util.Collections.synchronizedMap;

		import java.util.Collections;


		import io.dropwizard.Configuration;

		import static java.util.Collections.synchronizedMap;

		private String baseUrl = null;

		private String resourceLocation = null;

Offering extended configuration #79

Offering extended configuration #79

Uh oh!

Conversation

ajs6f commented Mar 21, 2018

Uh oh!

acoburn commented Mar 21, 2018

Uh oh!

ajs6f commented Mar 21, 2018

Uh oh!

ajs6f commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajs6f commented Mar 21, 2018

Uh oh!

acoburn commented Mar 21, 2018

Uh oh!

ajs6f commented Mar 21, 2018

Uh oh!

acoburn commented Mar 21, 2018

Uh oh!

ajs6f commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acoburn commented Mar 21, 2018

Uh oh!

ajs6f commented Mar 21, 2018

Uh oh!

ajs6f commented Mar 21, 2018

Uh oh!

ajs6f commented Mar 21, 2018

Uh oh!

ajs6f commented Mar 21, 2018

Uh oh!

ajs6f commented Mar 21, 2018

Uh oh!

christopher-johnson commented Mar 21, 2018

Uh oh!

ajs6f commented Mar 21, 2018

Uh oh!

acoburn left a comment

Choose a reason for hiding this comment

Uh oh!

acoburn Mar 22, 2018

Choose a reason for hiding this comment

Uh oh!

acoburn Mar 22, 2018

Choose a reason for hiding this comment

Uh oh!

acoburn Mar 22, 2018

Choose a reason for hiding this comment

Uh oh!

acoburn Mar 22, 2018

Choose a reason for hiding this comment

Uh oh!

acoburn Mar 22, 2018

Choose a reason for hiding this comment

Uh oh!

ajs6f commented Mar 22, 2018

Uh oh!

acoburn commented Mar 22, 2018

Uh oh!

ajs6f commented Mar 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ajs6f commented Mar 21, 2018 •

edited

Loading

ajs6f commented Mar 21, 2018 •

edited

Loading