From 9a2ad068d3ab58b57bef36f66b1612ac0a36ace7 Mon Sep 17 00:00:00 2001 From: Chris Vest Date: Tue, 6 Nov 2018 14:31:02 +0100 Subject: [PATCH] Make it possible for Config to throw an exception if it cannot load configurations from a file. --- .../consistency/CheckConsistencyCommand.java | 19 ++++--- .../consistency/ConsistencyCheckTool.java | 11 ++-- .../neo4j/commandline/dbms/ImportCommand.java | 29 +++------- .../java/org/neo4j/tooling/ImportTool.java | 7 +-- .../neo4j/kernel/configuration/Config.java | 27 ++++++++-- .../kernel/configuration/ConfigTest.java | 54 +++++++++++++++++-- 6 files changed, 98 insertions(+), 49 deletions(-) diff --git a/community/consistency-check/src/main/java/org/neo4j/consistency/CheckConsistencyCommand.java b/community/consistency-check/src/main/java/org/neo4j/consistency/CheckConsistencyCommand.java index 32435cb5e8151..5fe1335e9fe1c 100644 --- a/community/consistency-check/src/main/java/org/neo4j/consistency/CheckConsistencyCommand.java +++ b/community/consistency-check/src/main/java/org/neo4j/consistency/CheckConsistencyCommand.java @@ -209,22 +209,22 @@ public void execute( String[] args ) throws IncorrectUsage, CommandFailed } } - private static Map loadAdditionalConfig( Optional additionalConfigFile ) + private static Config loadAdditionalConfig( Optional additionalConfigFile ) { if ( additionalConfigFile.isPresent() ) { try { - return MapUtil.load( additionalConfigFile.get().toFile() ); + return Config.fromFile( additionalConfigFile.get() ).withThrowOnFileLoadFailure().build(); } - catch ( IOException e ) + catch ( Exception e ) { throw new IllegalArgumentException( String.format( "Could not read configuration file [%s]", additionalConfigFile ), e ); } } - return new HashMap<>(); + return Config.defaults(); } private static void checkDbState( DatabaseLayout databaseLayout, Config additionalConfiguration ) throws CommandFailed @@ -254,13 +254,12 @@ private static void checkDbState( DatabaseLayout databaseLayout, Config addition } } - private static Config loadNeo4jConfig( Path homeDir, Path configDir, String databaseName, - Map additionalConfig ) + private static Config loadNeo4jConfig( Path homeDir, Path configDir, String databaseName, Config additionalConfig ) { - additionalConfig.put( GraphDatabaseSettings.active_database.name(), databaseName ); - - return Config.fromFile( configDir.resolve( Config.DEFAULT_CONFIG_FILE_NAME ) ).withHome( homeDir ).withConnectorsDisabled() - .withSettings( additionalConfig ).build(); + Config config = Config.fromFile( configDir.resolve( Config.DEFAULT_CONFIG_FILE_NAME ) ).withHome( homeDir ).withConnectorsDisabled().build(); + config.augment( additionalConfig ); + config.augment( GraphDatabaseSettings.active_database, databaseName ); + return config; } public static Arguments arguments() diff --git a/community/consistency-check/src/main/java/org/neo4j/consistency/ConsistencyCheckTool.java b/community/consistency-check/src/main/java/org/neo4j/consistency/ConsistencyCheckTool.java index a901458c39f3e..efd0a9442fa86 100644 --- a/community/consistency-check/src/main/java/org/neo4j/consistency/ConsistencyCheckTool.java +++ b/community/consistency-check/src/main/java/org/neo4j/consistency/ConsistencyCheckTool.java @@ -180,23 +180,20 @@ private File determineStoreDirectory( Args arguments ) throws ToolFailureExcepti private static Config readConfiguration( Args arguments ) throws ToolFailureException { - Map specifiedConfig = stringMap(); - String configFilePath = arguments.get( CONFIG, null ); if ( configFilePath != null ) { File configFile = new File( configFilePath ); try { - specifiedConfig = MapUtil.load( configFile ); + return Config.fromFile( configFile ).withThrowOnFileLoadFailure().build(); } - catch ( IOException e ) + catch ( Exception e ) { - throw new ToolFailureException( String.format( "Could not read configuration file [%s]", - configFilePath ), e ); + throw new ToolFailureException( String.format( "Could not read configuration file [%s]", configFilePath ), e ); } } - return Config.defaults( specifiedConfig ); + return Config.defaults(); } private String usage() diff --git a/community/dbms/src/main/java/org/neo4j/commandline/dbms/ImportCommand.java b/community/dbms/src/main/java/org/neo4j/commandline/dbms/ImportCommand.java index fd314c06ce5c1..b453a809687f6 100644 --- a/community/dbms/src/main/java/org/neo4j/commandline/dbms/ImportCommand.java +++ b/community/dbms/src/main/java/org/neo4j/commandline/dbms/ImportCommand.java @@ -258,31 +258,16 @@ private static String[] getImportToolArgs( String[] userSupplierArguments ) thro return fileArgument.isPresent() ? parseFileArgumentList( fileArgument.get().toFile() ) : userSupplierArguments; } - private static Map loadAdditionalConfig( Optional additionalConfigFile ) + private static Config loadAdditionalConfig( Optional additionalConfigFile ) { - if ( additionalConfigFile.isPresent() ) - { - try - { - return MapUtil.load( additionalConfigFile.get().toFile() ); - } - catch ( IOException e ) - { - throw new IllegalArgumentException( - String.format( "Could not read configuration file [%s]", additionalConfigFile ), e ); - } - } - - return new HashMap<>(); + return additionalConfigFile.map( path -> Config.fromFile( path ).withThrowOnFileLoadFailure().build() ).orElseGet( Config::defaults ); } - private static Config loadNeo4jConfig( Path homeDir, Path configDir, String databaseName, - Map additionalConfig ) + private static Config loadNeo4jConfig( Path homeDir, Path configDir, String databaseName, Config additionalConfig ) { - return Config.fromFile( configDir.resolve( Config.DEFAULT_CONFIG_FILE_NAME ) ) - .withHome( homeDir ) - .withSetting( GraphDatabaseSettings.active_database, databaseName ) - .withSettings( additionalConfig ) - .withConnectorsDisabled().build(); + Config config = Config.fromFile( configDir.resolve( Config.DEFAULT_CONFIG_FILE_NAME ) ).withHome( homeDir ).withConnectorsDisabled().build(); + config.augment( additionalConfig ); + config.augment( GraphDatabaseSettings.active_database, databaseName ); + return config; } } diff --git a/community/import-tool/src/main/java/org/neo4j/tooling/ImportTool.java b/community/import-tool/src/main/java/org/neo4j/tooling/ImportTool.java index 550bbe8851a56..122e7d16aa485 100644 --- a/community/import-tool/src/main/java/org/neo4j/tooling/ImportTool.java +++ b/community/import-tool/src/main/java/org/neo4j/tooling/ImportTool.java @@ -26,6 +26,7 @@ import java.io.InputStream; import java.io.OutputStream; import java.io.PrintStream; +import java.io.UncheckedIOException; import java.nio.charset.Charset; import java.util.ArrayList; import java.util.Arrays; @@ -491,7 +492,7 @@ public static void main( String[] incomingArguments, boolean defaultSettingsSuit { throw andPrintError( "Input error", e, false, err ); } - catch ( IOException e ) + catch ( IOException | UncheckedIOException e ) { throw andPrintError( "File error", e, false, err ); } @@ -657,9 +658,9 @@ private static long parseNumberOrUnlimited( Args args, Options option ) return UNLIMITED.equals( value ) ? BadCollector.UNLIMITED_TOLERANCE : Long.parseLong( value ); } - private static Config loadDbConfig( File file ) throws IOException + private static Config loadDbConfig( File file ) { - return file != null && file.exists() ? Config.defaults( MapUtil.load( file ) ) : Config.defaults(); + return Config.fromFile( file ).withThrowOnFileLoadFailure().build(); } static void printOverview( File storeDir, Collection> nodesFiles, diff --git a/community/kernel/src/main/java/org/neo4j/kernel/configuration/Config.java b/community/kernel/src/main/java/org/neo4j/kernel/configuration/Config.java index ffdcd58e0586a..a6315e3dfb2d0 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/configuration/Config.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/configuration/Config.java @@ -21,6 +21,7 @@ import java.io.File; import java.io.IOException; +import java.io.UncheckedIOException; import java.nio.file.Path; import java.util.ArrayList; import java.util.Collection; @@ -118,6 +119,7 @@ public static class Builder private File configFile; private List settingsClasses; private boolean connectorsDisabled; + private boolean throwOnFileLoadFailure; /** * Augment the configuration with the passed setting. @@ -277,6 +279,16 @@ public Builder withConnectorsDisabled() return this; } + /** + * Cause the {@link #build()} method to throw an {@link UncheckedIOException} if the given {@code withFile} configuration file could not be loaded + * for some reason. The default behaviour is to log an error instead. + */ + public Builder withThrowOnFileLoadFailure() + { + throwOnFileLoadFailure = true; + return this; + } + /** * @return The config reflecting the state of the builder. * @throws InvalidSettingException is thrown if an invalid setting is encountered and {@link @@ -294,7 +306,7 @@ public Config build() throws InvalidSettingException initialSettings.put( GraphDatabaseSettings.neo4j_home.name(), System.getProperty( "user.dir" ) ); } - Config config = new Config( configFile, initialSettings, overriddenDefaults, validators, loadableConfigs ); + Config config = new Config( configFile, throwOnFileLoadFailure, initialSettings, overriddenDefaults, validators, loadableConfigs ); if ( connectorsDisabled ) { @@ -370,6 +382,7 @@ public static Config defaults( @Nonnull final Setting setting, final String v } private Config( File configFile, + boolean throwOnFileLoadFailure, Map initialSettings, Map overriddenDefaults, Collection additionalValidators, @@ -394,7 +407,7 @@ private Config( File configFile, boolean fromFile = configFile != null; if ( fromFile ) { - loadFromFile( configFile, log ).forEach( initialSettings::putIfAbsent ); + loadFromFile( configFile, log, throwOnFileLoadFailure ).forEach( initialSettings::putIfAbsent ); } overriddenDefaults.forEach( initialSettings::putIfAbsent ); @@ -800,10 +813,14 @@ private void warnAboutDeprecations( Map userSettings ) } @Nonnull - private static Map loadFromFile( @Nonnull File file, @Nonnull Log log ) + private static Map loadFromFile( @Nonnull File file, @Nonnull Log log, boolean throwOnFileLoadFailure ) { if ( !file.exists() ) { + if ( throwOnFileLoadFailure ) + { + throw new UncheckedIOException( new IOException( "Config file [" + file + "] does not exist." ) ); + } log.warn( "Config file [%s] does not exist.", file ); return new HashMap<>(); } @@ -813,6 +830,10 @@ private static Map loadFromFile( @Nonnull File file, @Nonnull Log } catch ( IOException e ) { + if ( throwOnFileLoadFailure ) + { + throw new UncheckedIOException( "Unable to load config file [" + file + "].", e ); + } log.error( "Unable to load config file [%s]: %s", file, e.getMessage() ); return new HashMap<>(); } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/configuration/ConfigTest.java b/community/kernel/src/test/java/org/neo4j/kernel/configuration/ConfigTest.java index c98012d0bc9c6..a2029bd29c9c0 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/configuration/ConfigTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/configuration/ConfigTest.java @@ -25,6 +25,8 @@ import org.mockito.InOrder; import java.io.File; +import java.io.IOException; +import java.io.UncheckedIOException; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; @@ -53,6 +55,7 @@ import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.junit.Assume.assumeTrue; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -196,8 +199,7 @@ public void augmentAnotherConfig() } @Test - public void shouldWarnAndDiscardUnknownOptionsInReservedNamespaceAndPassOnBufferedLogInWithMethods() - throws Exception + public void shouldWarnAndDiscardUnknownOptionsInReservedNamespaceAndPassOnBufferedLogInWithMethods() throws Exception { // Given Log log = mock( Log.class ); @@ -220,8 +222,7 @@ public void shouldWarnAndDiscardUnknownOptionsInReservedNamespaceAndPassOnBuffer } @Test - public void shouldLogDeprecationWarnings() - throws Exception + public void shouldLogDeprecationWarnings() throws Exception { // Given Log log = mock( Log.class ); @@ -245,6 +246,51 @@ public void shouldLogDeprecationWarnings() verifyNoMoreInteractions( log ); } + @Test + public void shouldLogIfConfigFileCouldNotBeFound() + { + Log log = mock( Log.class ); + File confFile = testDirectory.file( "test.conf" ); // Note: we don't create the file. + + Config config = Config.fromFile( confFile ).build(); + + config.setLogger( log ); + + verify( log ).warn( "Config file [%s] does not exist.", confFile ); + } + + @Test + public void shouldLogIfConfigFileCouldNotBeRead() throws IOException + { + Log log = mock( Log.class ); + File confFile = testDirectory.file( "test.conf" ); + assertTrue( confFile.createNewFile() ); + assumeTrue( confFile.setReadable( false ) ); + + Config config = Config.fromFile( confFile ).build(); + + config.setLogger( log ); + + verify( log ).error( "Unable to load config file [%s]: %s", confFile, confFile + " (Permission denied)" ); + } + + @Test( expected = UncheckedIOException.class ) + public void mustThrowIfConfigFileCouldNotBeFound() + { + File confFile = testDirectory.file( "test.conf" ); + + Config.fromFile( confFile ).withThrowOnFileLoadFailure().build(); + } + + @Test( expected = UncheckedIOException.class ) + public void mustThrowIfConfigFileCoutNotBeRead() throws IOException + { + File confFile = testDirectory.file( "test.conf" ); + assertTrue( confFile.createNewFile() ); + assumeTrue( confFile.setReadable( false ) ); + Config.fromFile( confFile ).withThrowOnFileLoadFailure().build(); + } + @Test public void shouldSetInternalParameter() {