Fix Array Configurable CLI options #514
Conversation
--host-whitelist and --rpc-cors-origins could not be configured as a TOML array. The underlying PicoCLI issues were resolved with revamped property types that act like Collections. A "kitchen sink" test is added that creates a TOML file that requires all CLI options to be configurable and configured in a new kitchen_sink.toml test file.
use "everything_config" instead of the colloquial "kitchen sink" and add more verbiage in the everything_config file.
| try { | ||
| final StringJoiner stringJoiner = new StringJoiner("|"); | ||
| domains.forEach(stringJoiner::add); | ||
| Pattern.compile(stringJoiner.toString()); |
There was a problem hiding this comment.
This looks like we're no longer filtering empty domains. Previously (I think) we threw an error if the overall list of domains was null or empty but if it was say domainA,,domainB we'd accept it and filter out the empty domain caused by the double commas. I'm assuming that PicoCLI won't ever try to add a null to this list but we may need to test that explicitly.
There was a problem hiding this comment.
Pico won't add nulls, but because of defense in depth I should check. I also now split on one or more commas, so a,,b will filter out the empty middle.
| return new CorsAllowedOriginsProperty(Lists.newArrayList("*")); | ||
| } else if ("none".equals(s)) { | ||
| if (!domains.isEmpty()) { | ||
| throw new IllegalArgumentException("Value '" + s + "' can't be used with other domains"); |
There was a problem hiding this comment.
It looks like we'd now accept none,domainA,domainB whereas it would have previously been rejected because when none was added the list was empty even though it later became non-empty.
There was a problem hiding this comment.
Fixed. Checking on the individual case was inefficient, so I do a mass check in the addAll now.
| throw new IllegalArgumentException("Domain cannot be empty string"); | ||
| } else if ("all".equals(s) || "*".equals(s)) { | ||
| if (!domains.isEmpty()) { | ||
| throw new IllegalArgumentException("Value '" + s + "' can't be used with other domains"); |
There was a problem hiding this comment.
Same comment as for none below.
There was a problem hiding this comment.
Done the same as for none.
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
I think this has the same potential behaviour changes as CorsAllowedOriginsProperty
There was a problem hiding this comment.
I provided the same treatment for whitelist (except for the "this is a valid regex" check) now as cors-allowed-origins
| @@ -881,8 +922,7 @@ public void jsonRpcCorsOriginsNoneWithAnotherDomainMustFail() { | |||
|
|
|||
| @Test | |||
| public void jsonRpcCorsOriginsAllWithAnotherDomainMustFail() { | |||
| final String[] origins = {"http://domain1.com", "all"}; | |||
| parseCommand("--rpc-cors-origins", String.join(",", origins)); | |||
| parseCommand("--rpc-cors-origins=http://domain1.com,all"); | |||
There was a problem hiding this comment.
I think a variants of this test with --rpc-cors-origins=all,http://domain1.com and --rpc-cors-origins= would be really useful and cover all the concerns I had above.
There was a problem hiding this comment.
Tests added for whitelist and cores. Whitelist also gained some tests that it didn't have before to act like cors as well.
* move validation into "addAll" * allow multiple commas * more tests
* Fix Array Configurable CLI options --host-whitelist and --rpc-cors-origins could not be configured as a TOML array. The underlying PicoCLI issues were resolved with revamped property types that act like Collections. A "configure everything" test is added that creates a TOML file that requires all CLI options to be configurable and configured in a new everything_config.toml test file.
PR description
The --host-whitelist and --rpc-cors-origins CLIs could not be configured as
a TOML array. The underlying PicoCLI issues were resolved with
revamped property types that act like Collections.
A "kitchen sink" test is added that creates a TOML file that
requires all CLI options to be configurable and configured in
a new kitchen_sink.toml test file.