ctubbsii commented on a change in pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#discussion_r525298718
##########
File path:
core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java
##########
@@ -203,15 +204,18 @@ private SiteConfiguration(Map<String,String> config) {
}
// load properties from config file
+ @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "path
provided by test")
private static AbstractConfiguration getPropsFileConfig(URL
accumuloPropsLocation) {
if (accumuloPropsLocation != null) {
- var propsBuilder = new
FileBasedConfigurationBuilder<>(PropertiesConfiguration.class)
- .configure(new
Parameters().properties().setURL(accumuloPropsLocation));
- try {
- return propsBuilder.getConfiguration();
+ var config = new PropertiesConfiguration();
+ try (var reader = new FileReader(accumuloPropsLocation.getFile())) {
+ config.getLayout().load(config, reader);
} catch (ConfigurationException e) {
throw new IllegalArgumentException(e);
+ } catch (IOException e1) {
+ throw new UncheckedIOException("IOExcetion creating configuration",
e1);
Review comment:
This can be combined with the existing `ConfigurationException`
##########
File path:
core/src/test/java/org/apache/accumulo/core/conf/SiteConfigurationTest.java
##########
@@ -77,6 +78,8 @@ public void testFile() {
assertEquals("256M", conf.get(Property.TSERV_WALOG_MAX_SIZE));
assertEquals("org.apache.accumulo.core.cryptoImpl.AESCryptoService",
conf.get(Property.INSTANCE_CRYPTO_SERVICE));
+ assertEquals(System.getenv("USER"), conf.get("general.test.user.name"));
+ assertEquals("/tmp/test/dir", conf.get("general.test.user.dir"));
Review comment:
Nice checks to test interpolation!
##########
File path:
core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java
##########
@@ -194,14 +196,17 @@ public static ClientConfiguration create() {
* the path to the configuration file
* @since 1.9.0
*/
- public static ClientConfiguration fromFile(File file) {
- FileBasedConfigurationBuilder<PropertiesConfiguration> propsBuilder =
- new FileBasedConfigurationBuilder<>(PropertiesConfiguration.class)
- .configure(new Parameters().properties().setFile(file));
- try {
- return new
ClientConfiguration(Collections.singletonList(propsBuilder.getConfiguration()));
+ public static ClientConfiguration fromFile(File file) throws
FileNotFoundException {
Review comment:
This changes exceptions thrown on a public API method.
##########
File path: assemble/pom.xml
##########
@@ -106,11 +106,6 @@
<artifactId>jaxb-core</artifactId>
<optional>true</optional>
</dependency>
- <dependency>
- <groupId>commons-beanutils</groupId>
- <artifactId>commons-beanutils</artifactId>
- <optional>true</optional>
- </dependency>
Review comment:
Can probably also remove the entry from
`assemble/src/main/assemblies/component.xml`
##########
File path:
core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java
##########
@@ -194,14 +196,17 @@ public static ClientConfiguration create() {
* the path to the configuration file
* @since 1.9.0
*/
- public static ClientConfiguration fromFile(File file) {
- FileBasedConfigurationBuilder<PropertiesConfiguration> propsBuilder =
- new FileBasedConfigurationBuilder<>(PropertiesConfiguration.class)
- .configure(new Parameters().properties().setFile(file));
- try {
- return new
ClientConfiguration(Collections.singletonList(propsBuilder.getConfiguration()));
+ public static ClientConfiguration fromFile(File file) throws
FileNotFoundException {
+ var config = new PropertiesConfiguration();
+ try (var reader = new FileReader(file)) {
+ config.getLayout().load(config, reader);
+ return new ClientConfiguration(Collections.singletonList(config));
} catch (ConfigurationException e) {
throw new IllegalArgumentException("Bad configuration file: " + file, e);
+ } catch (FileNotFoundException fnfe) {
+ throw fnfe;
+ } catch (IOException e1) {
+ throw new UncheckedIOException("IOExcetion creating configuration", e1);
Review comment:
It'd be better to add these exceptions to the existing
`ConfigurationException` using multi-catch. That way, we don't introduce new
exceptions in the public API method.
##########
File path:
test/src/main/java/org/apache/accumulo/test/metrics/MetricsFileTailer.java
##########
@@ -153,11 +147,12 @@ private Configuration loadMetricsConfig() {
}
return sub;
-
- } catch (ConfigurationException ex) {
+ } catch (ConfigurationException e) {
throw new IllegalStateException(
String.format("Could not find configuration file \'%s\' on
classpath",
MetricsTestSinkProperties.METRICS_PROP_FILENAME));
+ } catch (IOException e1) {
+ throw new UncheckedIOException("IOExcetion creating configuration", e1);
Review comment:
This can be combined with the existing `ConfigurationException`
##########
File path:
core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java
##########
@@ -225,14 +230,15 @@ private static ClientConfiguration
loadFromSearchPath(List<String> paths) {
for (String path : paths) {
File conf = new File(path);
if (conf.isFile() && conf.canRead()) {
- FileBasedConfigurationBuilder<PropertiesConfiguration> propsBuilder =
- new FileBasedConfigurationBuilder<>(PropertiesConfiguration.class)
- .configure(new Parameters().properties().setFile(conf));
- try {
- configs.add(propsBuilder.getConfiguration());
+ var config = new PropertiesConfiguration();
+ try (var reader = new FileReader(conf)) {
+ config.getLayout().load(config, reader);
+ configs.add(config);
log.info("Loaded client configuration file {}", conf);
} catch (ConfigurationException e) {
throw new IllegalStateException("Error loading client configuration
file " + conf, e);
+ } catch (IOException e1) {
+ throw new UncheckedIOException("IOExcetion creating configuration",
e1);
Review comment:
This can be combined with the existing `ConfigurationException`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
