Skip to content

Commit

Permalink
Make it possible for Config to throw an exception if it cannot load c…
Browse files Browse the repository at this point in the history 
…onfigurations from a file.
  • Loading branch information
@chrisvest
chrisvest committed Nov 27, 2018
1 parent 255df9e commit 9a2ad06
Show file tree
Hide file tree
Showing 6 changed files with 98 additions and 49 deletions.
19 changes: 9 additions & 10 deletions community/consistency-check/src/main/java/org/neo4j/consistency/CheckConsistencyCommand.java
View file
Expand Up @@ -209,22 +209,22 @@ public void execute( String[] args ) throws IncorrectUsage, CommandFailed
}
}

private static Map<String,String> loadAdditionalConfig( Optional<Path> additionalConfigFile )
private static Config loadAdditionalConfig( Optional<Path> 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
Expand Down Expand Up @@ -254,13 +254,12 @@ private static void checkDbState( DatabaseLayout databaseLayout, Config addition
}
}

private static Config loadNeo4jConfig( Path homeDir, Path configDir, String databaseName,
Map<String,String> 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()
Expand Down
11 changes: 4 additions & 7 deletions community/consistency-check/src/main/java/org/neo4j/consistency/ConsistencyCheckTool.java
View file
Expand Up @@ -180,23 +180,20 @@ private File determineStoreDirectory( Args arguments ) throws ToolFailureExcepti

private static Config readConfiguration( Args arguments ) throws ToolFailureException
{
Map<String,String> 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()
Expand Down
29 changes: 7 additions & 22 deletions community/dbms/src/main/java/org/neo4j/commandline/dbms/ImportCommand.java
View file
Expand Up @@ -258,31 +258,16 @@ private static String[] getImportToolArgs( String[] userSupplierArguments ) thro
return fileArgument.isPresent() ? parseFileArgumentList( fileArgument.get().toFile() ) : userSupplierArguments;
}

private static Map<String,String> loadAdditionalConfig( Optional<Path> additionalConfigFile )
private static Config loadAdditionalConfig( Optional<Path> 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<String,String> 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;
}
}
7 changes: 4 additions & 3 deletions community/import-tool/src/main/java/org/neo4j/tooling/ImportTool.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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 );
}
Expand Down Expand Up @@ -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<Option<File[]>> nodesFiles,
Expand Down
27 changes: 24 additions & 3 deletions community/kernel/src/main/java/org/neo4j/kernel/configuration/Config.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -118,6 +119,7 @@ public static class Builder
private File configFile;
private List<LoadableConfig> settingsClasses;
private boolean connectorsDisabled;
private boolean throwOnFileLoadFailure;

/**
* Augment the configuration with the passed setting.
Expand Down Expand Up @@ -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
Expand All @@ -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 )
{
Expand Down Expand Up @@ -370,6 +382,7 @@ public static Config defaults( @Nonnull final Setting<?> setting, final String v
}

private Config( File configFile,
boolean throwOnFileLoadFailure,
Map<String,String> initialSettings,
Map<String,String> overriddenDefaults,
Collection<ConfigurationValidator> additionalValidators,
Expand All @@ -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 );
Expand Down Expand Up @@ -800,10 +813,14 @@ private void warnAboutDeprecations( Map<String,String> userSettings )
}

@Nonnull
private static Map<String,String> loadFromFile( @Nonnull File file, @Nonnull Log log )
private static Map<String,String> 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<>();
}
Expand All @@ -813,6 +830,10 @@ private static Map<String,String> 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<>();
}
Expand Down
54 changes: 50 additions & 4 deletions community/kernel/src/test/java/org/neo4j/kernel/configuration/ConfigTest.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -196,8 +199,7 @@ public void augmentAnotherConfig()
}

@Test
public void shouldWarnAndDiscardUnknownOptionsInReservedNamespaceAndPassOnBufferedLogInWithMethods()
throws Exception
public void shouldWarnAndDiscardUnknownOptionsInReservedNamespaceAndPassOnBufferedLogInWithMethods() throws Exception
{
// Given
Log log = mock( Log.class );
Expand All @@ -220,8 +222,7 @@ public void shouldWarnAndDiscardUnknownOptionsInReservedNamespaceAndPassOnBuffer
}

@Test
public void shouldLogDeprecationWarnings()
throws Exception
public void shouldLogDeprecationWarnings() throws Exception
{
// Given
Log log = mock( Log.class );
Expand All @@ -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()
{
Expand Down

0 comments on commit 9a2ad06

Please sign in to comment.