Added new config option: dbms.config.strict_validation

By default this setting is false, for now. It causes config parsing to
throw errors on unknown settings (in our namespace).

Settings outside our namespaces are still retained and not warned
about.

Also changed so that warning is a flag, and it is only true when
reading the config file (so just once).

community/kernel/src/main/java/org/neo4j/graphdb/factory/GraphDatabaseSettings.java

@@ -20,7 +20,6 @@
 package org.neo4j.graphdb.factory;
 
 import java.io.File;
-import java.util.Collections;
 import java.util.List;
 
 import org.neo4j.configuration.Description;
@@ -29,9 +28,7 @@
 import org.neo4j.helpers.AdvertisedSocketAddress;
 import org.neo4j.helpers.ListenSocketAddress;
 import org.neo4j.io.ByteUnit;
-import org.neo4j.kernel.configuration.BoltConnector;
 import org.neo4j.kernel.configuration.BoltConnectorValidator;
-import org.neo4j.kernel.configuration.Config;
 import org.neo4j.kernel.configuration.ConfigurationMigrator;
 import org.neo4j.kernel.configuration.GraphDatabaseConfigurationMigrator;
 import org.neo4j.kernel.configuration.Group;
@@ -110,6 +107,11 @@ public class GraphDatabaseSettings implements LoadableConfig
     @Internal
     public static final Setting<Boolean> dump_configuration = setting("unsupported.dbms.report_configuration", BOOLEAN, FALSE );
 
+    @Description( "A strict configuration validation will prevent the database from starting up if unknown " +
+            "configuration options are specified in the neo4j settings namespace (such as dbms., ha., cypher., etc)." )
+    public static final Setting<Boolean> strict_config_validation =
+            setting("dbms.config.strict_validation", BOOLEAN, FALSE );
+
     @Description("Whether to allow a store upgrade in case the current version of the database starts against an " +
             "older store version. " +
             "Setting this to `true` does not guarantee successful upgrade, it just " +

community/kernel/src/main/java/org/neo4j/kernel/configuration/Config.java

@@ -224,7 +224,7 @@ private Config( Optional<File> configFile,
         migrator = new AnnotationBasedConfigurationMigrator( settingsClasses );
 
         Map<String,String> settings = initSettings( configFile, settingsPostProcessor, overriddenSettings, this.log );
-        replaceSettings( settings );
+        replaceSettings( settings, configFile.isPresent() );
     }
 
     /**
@@ -275,7 +275,7 @@ public Config augment( Map<String,String> changes )
     {
         Map<String,String> params = new HashMap<>( this.params );
         params.putAll( changes );
-        replaceSettings( params );
+        replaceSettings( params, false );
         return this;
     }
 
@@ -392,13 +392,14 @@ public String toString()
         return output.toString();
     }
 
-    private synchronized void replaceSettings( Map<String,String> newSettings )
+    private synchronized void replaceSettings( Map<String,String> newSettings, boolean warnOnUnknownSettings )
     {
         Map<String,String> validSettings = migrator.apply( newSettings, log );
         List<SettingValidator> settingValidators = configOptions.stream()
                 .map( ConfigOptions::settingGroup )
                 .collect( Collectors.toList() );
-        validSettings = new IndividualSettingsValidator().validate( settingValidators, validSettings, log );
+        validSettings = new IndividualSettingsValidator( warnOnUnknownSettings )
+                .validate( settingValidators, validSettings, log );
         for ( ConfigurationValidator validator : validators )
         {
             validSettings = validator.validate( settingValidators, validSettings, log );

community/kernel/src/main/java/org/neo4j/kernel/configuration/ConfigurationValidator.java

@@ -34,8 +34,8 @@
 public interface ConfigurationValidator
 {
     /**
-     * Validate a config and return its subset of valid keys and values. Unknown settings should be discarded and
-     * have a warning logged. Invalid settings should cause an error.
+     * Validate a config and return its subset of valid keys and values. Unknown settings should be discarded. Invalid
+     * settings should cause an error.
      *
      * @param settingValidators which are available
      * @param rawConfig to validate

community/kernel/src/main/java/org/neo4j/kernel/configuration/IndividualSettingsValidator.java

@@ -29,16 +29,29 @@
 
 import org.neo4j.graphdb.config.InvalidSettingException;
 import org.neo4j.graphdb.config.SettingValidator;
+import org.neo4j.graphdb.factory.GraphDatabaseSettings;
 import org.neo4j.logging.Log;
 
+import static org.neo4j.graphdb.factory.GraphDatabaseSettings.strict_config_validation;
+
 /**
  * Validates individual settings by delegating to the settings themselves without taking other aspects into
  * consideration.
  */
 public class IndividualSettingsValidator implements ConfigurationValidator
 {
     private static final List<String> reservedPrefixes =
-            Arrays.asList( "dbms.", "metrics.", "ha.", "causal_clustering.", "browser.", "tools." );
+            Arrays.asList( "dbms.", "metrics.", "ha.", "causal_clustering.", "browser.", "tools.", "unsupported." );
+    private final boolean warnOnUnknownSettings;
+
+    /**
+     *
+     * @param warnOnUnknownSettings if unknown options should be logged when strict validation is disabled
+     */
+    public IndividualSettingsValidator( boolean warnOnUnknownSettings )
+    {
+        this.warnOnUnknownSettings = warnOnUnknownSettings;
+    }
 
     @Override
     @Nonnull
@@ -51,6 +64,8 @@ public Map<String,String> validate( @Nonnull Collection<SettingValidator> settin
                 .flatMap( map -> map.entrySet().stream() )
                 .collect( Collectors.toMap( Entry::getKey, Entry::getValue ) );
 
+        final boolean strictValidation = strict_config_validation.apply( validConfig::get );
+
         rawConfig.forEach( ( key, value ) ->
         {
             if ( !validConfig.containsKey( key ) )
@@ -59,7 +74,21 @@ public Map<String,String> validate( @Nonnull Collection<SettingValidator> settin
                 // As a compromise, we only warn (and discard) for settings in our own "namespace"
                 if ( reservedPrefixes.stream().anyMatch( key::startsWith ) )
                 {
-                    log.warn( "Ignored unknown config option: %s", key );
+                    if ( warnOnUnknownSettings )
+                    {
+                        log.warn( "Unknown config option: %s", key );
+                    }
+
+                    if ( strictValidation )
+                    {
+                        throw new InvalidSettingException( String.format(
+                                "Unknown config option '%s'. To resolve either remove it from your configuration " +
+                                        "or set '%s' to false.", key, strict_config_validation.name() ) );
+                    }
+                    else
+                    {
+                        validConfig.put( key, value );
+                    }
                 }
                 else
                 {

community/kernel/src/test/java/org/neo4j/kernel/configuration/ConfigTest.java

@@ -19,26 +19,30 @@
  */
 package org.neo4j.kernel.configuration;
 
-import org.junit.Test;
-
+import java.io.File;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Map;
 import java.util.Optional;
-
 import javax.annotation.Nonnull;
 
+import org.junit.Rule;
+import org.junit.Test;
+
 import org.neo4j.configuration.LoadableConfig;
 import org.neo4j.graphdb.config.InvalidSettingException;
 import org.neo4j.graphdb.config.Setting;
 import org.neo4j.graphdb.config.SettingValidator;
+import org.neo4j.graphdb.factory.GraphDatabaseSettings;
 import org.neo4j.logging.Log;
+import org.neo4j.test.rule.TestDirectory;
 
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.mock;
@@ -55,15 +59,15 @@ public class ConfigTest
 {
     public static class MyMigratingSettings implements LoadableConfig
     {
-        @SuppressWarnings("unused") // accessed by reflection
+        @SuppressWarnings( "unused" ) // accessed by reflection
         @Migrator
         public static ConfigurationMigrator migrator = new BaseConfigurationMigrator()
         {
             {
                 add( new SpecificPropertyMigration( "old", "Old has been replaced by newer!" )
                 {
                     @Override
-                    public void setValueWithOldSetting( String value, Map<String, String> rawConfiguration )
+                    public void setValueWithOldSetting( String value, Map<String,String> rawConfiguration )
                     {
                         rawConfiguration.put( newer.name(), value );
                     }
@@ -90,11 +94,17 @@ static Config Config()
         return Config( Collections.emptyMap() );
     }
 
-    static Config Config( Map<String,String> params ) {
-        return new Config( Optional.empty(), params, s -> {}, Collections.emptyList(), Optional.empty(),
-                Arrays.asList(mySettingsWithDefaults, myMigratingSettings ) );
+    static Config Config( Map<String,String> params )
+    {
+        return new Config( Optional.empty(), params, s ->
+        {
+        }, Collections.emptyList(), Optional.empty(),
+                Arrays.asList( mySettingsWithDefaults, myMigratingSettings ) );
     }
 
+    @Rule
+    public TestDirectory testDirectory = TestDirectory.testDirectory();
+
     @Test
     public void shouldApplyDefaults()
     {
@@ -113,7 +123,7 @@ public void shouldApplyMigrations()
         assertThat( config.get( MyMigratingSettings.newer ), is( "hello!" ) );
     }
 
-    @Test(expected = InvalidSettingException.class)
+    @Test( expected = InvalidSettingException.class )
     public void shouldNotAllowSettingInvalidValues()
     {
         Config( stringMap( MySettingsWithDefaults.boolSetting.name(), "asd" ) );
@@ -139,7 +149,7 @@ public void shouldBeAbleToAugmentConfig() throws Exception
     public void shouldRetainCustomConfigOutsideNamespaceAndPassOnBufferedLogInWithMethods() throws Exception
     {
         // Given
-        Log log = mock(Log.class);
+        Log log = mock( Log.class );
         Config first = Config.embeddedDefaults( stringMap( "first.jibberish", "bah" ) );
 
         // When
@@ -150,27 +160,31 @@ public void shouldRetainCustomConfigOutsideNamespaceAndPassOnBufferedLogInWithMe
         // Then
         verifyNoMoreInteractions( log );
         assertEquals( Optional.of( "bah" ), third.getRaw( "first.jibberish" ) );
-        assertEquals( Optional.of( "baah" ), third.getRaw( "second.jibberish" ));
+        assertEquals( Optional.of( "baah" ), third.getRaw( "second.jibberish" ) );
         assertEquals( Optional.of( "baaah" ), third.getRaw( "third.jibberish" ) );
     }
 
     @Test
-    public void shouldWarnAndDiscardUnknownOptionsInReserverdNamespaceAndPassOnBufferedLogInWithMethods()
+    public void shouldWarnAndDiscardUnknownOptionsInReservedNamespaceAndPassOnBufferedLogInWithMethods()
             throws Exception
     {
         // Given
-        Log log = mock(Log.class);
-        Config first = Config.embeddedDefaults( stringMap( "dbms.jibberish", "bah" ) );
+        Log log = mock( Log.class );
+        File confFile = testDirectory.file( "test.conf" );
+        assertTrue( confFile.createNewFile() );
+
+        Config first = Config.embeddedDefaults( Optional.of( confFile ),
+                stringMap( GraphDatabaseSettings.strict_config_validation.name(), "false",
+                        "ha.jibberish", "baah",
+                        "dbms.jibberish", "booh" ) );
 
         // When
         first.setLogger( log );
-        Config second = first.with( stringMap( "ha.jibberish", "baah" ) );
-        second.withDefaults( stringMap( "causal_clustering.jibberish", "baah" ) );
+        first.with( stringMap( "causal_clustering.jibberish", "baah" ) );
 
         // Then
-        verify( log ).warn( "Ignored unknown config option: %s", "dbms.jibberish" );
-        verify( log ).warn( "Ignored unknown config option: %s", "ha.jibberish" );
-        verify( log ).warn( "Ignored unknown config option: %s", "causal_clustering.jibberish" );
+        verify( log ).warn( "Unknown config option: %s", "dbms.jibberish" );
+        verify( log ).warn( "Unknown config option: %s", "ha.jibberish" );
         verifyNoMoreInteractions( log );
     }
 

community/kernel/src/test/java/org/neo4j/kernel/configuration/IndividualSettingsValidatorTest.java

@@ -0,0 +1,105 @@
+/*
+ * Copyright (c) 2002-2017 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.kernel.configuration;
+
+import java.util.Map;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+import org.neo4j.graphdb.config.InvalidSettingException;
+import org.neo4j.logging.Log;
+
+import static java.util.Collections.singletonList;
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.neo4j.graphdb.factory.GraphDatabaseSettings.strict_config_validation;
+import static org.neo4j.helpers.collection.MapUtil.stringMap;
+import static org.neo4j.kernel.configuration.Settings.FALSE;
+import static org.neo4j.kernel.configuration.Settings.TRUE;
+
+public class IndividualSettingsValidatorTest
+{
+    @Rule
+    public ExpectedException expected = ExpectedException.none();
+    private Log log;
+
+    @Before
+    public void setup()
+    {
+        log = mock( Log.class );
+    }
+
+    @Test
+    public void nonStrictRetainsSettings() throws Exception
+    {
+        IndividualSettingsValidator iv = new IndividualSettingsValidator( true );
+
+        final Map<String,String> fullConfig = stringMap( strict_config_validation.name(), FALSE,
+                "dbms.jibber.jabber", "bla",
+                "external_plugin.foo", "bar" );
+
+        final Map<String,String> result = iv.validate( singletonList( strict_config_validation ),
+                fullConfig,
+                log );
+
+        assertEquals( fullConfig, result );
+        verify( log ).warn( "Unknown config option: %s", "dbms.jibber.jabber" );
+        verifyNoMoreInteractions( log );
+    }
+
+    @Test
+    public void strictErrorsOnUnknownSettingsInOurNamespace() throws Exception
+    {
+        IndividualSettingsValidator iv = new IndividualSettingsValidator( true );
+
+        final Map<String,String> fullConfig = stringMap( strict_config_validation.name(), TRUE,
+                "dbms.jibber.jabber", "bla",
+                "external_plugin.foo", "bar" );
+
+        expected.expect( InvalidSettingException.class );
+        expected.expectMessage( String.format( "Unknown config option 'dbms.jibber.jabber'. To resolve either remove" +
+                " it from your configuration or set '%s' to false.", strict_config_validation.name() ) );
+
+        iv.validate( singletonList( strict_config_validation ),
+                fullConfig,
+                log );
+    }
+
+    @Test
+    public void strictAllowsStuffOutsideOurNamespace() throws Exception
+    {
+        IndividualSettingsValidator iv = new IndividualSettingsValidator( true );
+
+        final Map<String,String> fullConfig = stringMap( strict_config_validation.name(), TRUE,
+                "external_plugin.foo", "bar" );
+
+        final Map<String,String> result = iv.validate( singletonList( strict_config_validation ),
+                fullConfig,
+                log );
+
+        assertEquals( fullConfig, result );
+        verifyNoMoreInteractions( log );
+    }
+}