lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [lucenenet] NightOwl888 commented on a change in pull request #287: Implemenation of "System Properties" in .NET (addresses #254)
Date Tue, 19 May 2020 13:14:07 GMT

NightOwl888 commented on a change in pull request #287:
URL: https://github.com/apache/lucenenet/pull/287#discussion_r427181252



##########
File path: src/dotnet/tools/lucene-cli/ConfigurationBase.cs
##########
@@ -1,4 +1,5 @@
 using Lucene.Net.Cli.CommandLine;
+using Lucene.Net.Configuration;

Review comment:
       Please revert this class to its original implementation on the master branch

##########
File path: src/dotnet/tools/lucene-cli/Configuration/ConfigurationRootFactory.cs
##########
@@ -0,0 +1,70 @@
+//using Lucene.Net.Configuration;
+//using Microsoft.Extensions.Configuration;
+//using System;
+//using System.Collections.Generic;
+
+//namespace Lucene.Net.Configuration
+//{
+//    /*
+//     * Licensed to the Apache Software Foundation (ASF) under one or more
+//     * contributor license agreements.  See the NOTICE file distributed with
+//     * this work for additional information regarding copyright ownership.
+//     * The ASF licenses this file to You under the Apache License, Version 2.0
+//     * (the "License"); you may not use this file except in compliance with
+//     * the License.  You may obtain a copy of the License at
+//     *
+//     *     http://www.apache.org/licenses/LICENSE-2.0
+//     *
+//     * Unless required by applicable law or agreed to in writing, software
+//     * distributed under the License is distributed on an "AS IS" BASIS,
+//     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//     * See the License for the specific language governing permissions and
+//     * limitations under the License.
+//     */
+
+using Lucene.Net.Configuration;
+using Microsoft.Extensions.Configuration;
+using System;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+
+    internal class ConfigurationRootFactory : IConfigurationRootFactory

Review comment:
       Please remove the unnecessary caching. This application reads configuration from a single folder. The implementation can be simplified to:
   
   ```c#
       internal class ConfigurationRootFactory : IConfigurationRootFactory
       {
           private readonly IConfigurationRoot configurationRoot;
   
           public ConfigurationRootFactory()
           {
               configurationRoot = new ConfigurationBuilder()
                   .AddEnvironmentVariables(prefix: "lucene:") // Use a custom prefix to only load Lucene.NET settings
                   .AddJsonFile("appsettings.json")
                   .Build();
           }
   
           public IConfigurationRoot CreateConfiguration()
           {
               return configurationRoot;
           }
       }
   ```

##########
File path: src/dotnet/tools/lucene-cli/Configuration/ConfigurationRootFactory.cs
##########
@@ -0,0 +1,70 @@
+//using Lucene.Net.Configuration;

Review comment:
       Please remove commented code

##########
File path: src/dotnet/tools/Lucene.Net.Tests.Cli/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,96 @@
+using Lucene.Net.Configuration;
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Cli.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]

Review comment:
       Please remove this commented block

##########
File path: src/Lucene.Net/Lucene.Net.csproj
##########
@@ -51,10 +51,12 @@
     
   <ItemGroup Condition=" '$(TargetFramework)' == 'netstandard2.1' ">
     <PackageReference Include="Microsoft.CSharp" Version="$(MicrosoftCSharpPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="$(MicrosoftExtensionsConfigurationPackageVersion)" />

Review comment:
       Please change the variable name to `$(MicrosoftExtensionsConfigurationAbstractionsPackageVersion)`

##########
File path: src/dotnet/tools/Lucene.Net.Tests.Cli/Configuration/TestDefaultSystemProperties.cs
##########
@@ -0,0 +1,37 @@
+using Lucene.Net.Util;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Cli.Configuration
+{
+
+
+    [TestFixture]
+    class TestDefaultSystemProperties : LuceneTestCase
+    {
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    //ConfigurationFactory = new DefaultConfigurationFactory(false);
+        //    //base.BeforeClass();
+        //}
+        [Test]
+        public virtual void ReadEnvironmentTest()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Assert.AreEqual(testValue, Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey]);

Review comment:
       Since we determined the `SetProperty` behavior is fundamentally different in .NET, `SystemProperties` should contain only `GetProperty` overloads, no `SetProperty` method.
   
   The point here is that the only way to set the properties should be with .NET APIs and .NET behavior. `ConfigurationSettings.CurrentConfiguration` is a public API for end users, but `SystemProperties` is an internal facade to make it similar in syntax to Java.
   
   However, testing that facade is a valid case. This class is made for testing `SystemProperties`, so it should be testing each of the `GetProperty` calls to verify the correct values are loaded.

##########
File path: src/Lucene.Net/Support/Configuration/DefaultConfigurationRootFactory.cs
##########
@@ -0,0 +1,53 @@
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Linq;
+using System.Security;
+using System.Text;
+using System.Threading;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Primitives;
+
+namespace Lucene.Net.Configuration
+{
+    internal class DefaultConfigurationRootFactory : IConfigurationRootFactory
+    {
+        private readonly bool ignoreSecurityExceptionsOnRead;

Review comment:
       Please remove this unused field.

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,94 @@
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;
+            Assert.AreEqual(Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey], testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"] = "en";
+            Assert.AreEqual("en", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+        }
+
+        [Test]
+        public virtual void TestSetandUnset()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"] = "en";
+            Assert.AreEqual("en", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+            ConfigurationSettings.CurrentConfiguration.Reload();
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+        }
+
+        [Test]
+        public virtual void TestDefaults()
+        {
+            Assert.AreEqual("perMethod", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:cleanthreads:sysprop"]);
+        }
+
+        [Test]
+        public virtual void TestHashCodeReadProperty()
+        {
+
+            Assert.AreEqual(0xf6a5c420, (uint)StringHelper.Murmurhash3_x86_32(new BytesRef("foo"), 0));
+
+            Assert.AreEqual(16, StringHelper.GOOD_FAST_HASH_SEED);
+            // Hashes computed using murmur3_32 from https://code.google.com/p/pyfasthash
+            Assert.AreEqual(0xcd018ef6, (uint)StringHelper.Murmurhash3_x86_32(new BytesRef("foo"), StringHelper.GOOD_FAST_HASH_SEED));
+        }
+
+        [Test]
+        public virtual void TestXMLConfiguration()
+        {
+            // TODO - not working with XML.
+            Assert.AreEqual("0x00000010", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:seed"]);
+        }
+
+        [Test]
+        public virtual void TestCommandLineProperty()
+        {
+            TestContext.Progress.WriteLine("TestContext.Parameters ({0})", TestContext.Parameters.Count);
+            foreach (var x in TestContext.Parameters.Names)
+                TestContext.Progress.WriteLine(string.Format("{0}={1}", x, TestContext.Parameters[x]));
+        }
+
+        [Test]
+        public virtual void TestCachedConfigProperty()
+        {
+            Assert.AreEqual("0x00000020", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:seed"]);

Review comment:
       Please test `SystemProperties.GetProperty()` here

##########
File path: src/Lucene.Net/Support/Util/SystemProperties.cs
##########
@@ -36,7 +38,7 @@ namespace Lucene.Net.Util
     /// to change the read behavior of these methods to throw the underlying exception 
     /// instead of returning the default value.
     /// </summary>
-    public static class SystemProperties
+    internal static class SystemProperties

Review comment:
       Please update all of the documentation comments in this class to indicate that this class reads "properties" rather than "environment variables". Also include a brief section in the class header that links to the `ConfigurationSettings.SetConfigurationRootFactory()` method and explains that the `IConfigurationRootFactory` instance that is set there is where system properties are retrieved.
   
   Although this class is marked internal, the documentation comments are used to generate the API docs - [Class SystemProperties](https://lucenenet.apache.org/docs/4.8.0-beta00007/api/Lucene.Net/Lucene.Net.Support.SystemProperties.html).
   
   > NOTE: It is in the wrong namespace in the documentation because it has been moved from the `Lucene.Net.Support` to the `Lucene.Net.Util` namespace in 4.8.0-beta00008, but the documents for that version haven't been generated yet.

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,94 @@
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;
+            Assert.AreEqual(Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey], testValue);

Review comment:
       Please test `SystemProperties.GetProperty()` here

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,94 @@
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;
+            Assert.AreEqual(Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey], testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"] = "en";
+            Assert.AreEqual("en", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+        }
+
+        [Test]
+        public virtual void TestSetandUnset()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"] = "en";
+            Assert.AreEqual("en", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+            ConfigurationSettings.CurrentConfiguration.Reload();
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+        }
+
+        [Test]
+        public virtual void TestDefaults()
+        {
+            Assert.AreEqual("perMethod", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:cleanthreads:sysprop"]);

Review comment:
       Please test `SystemProperties.GetProperty()` here

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,94 @@
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;
+            Assert.AreEqual(Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey], testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"] = "en";
+            Assert.AreEqual("en", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+        }
+
+        [Test]
+        public virtual void TestSetandUnset()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"] = "en";
+            Assert.AreEqual("en", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+            ConfigurationSettings.CurrentConfiguration.Reload();
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+        }
+
+        [Test]
+        public virtual void TestDefaults()
+        {
+            Assert.AreEqual("perMethod", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:cleanthreads:sysprop"]);
+        }
+
+        [Test]
+        public virtual void TestHashCodeReadProperty()
+        {
+
+            Assert.AreEqual(0xf6a5c420, (uint)StringHelper.Murmurhash3_x86_32(new BytesRef("foo"), 0));
+
+            Assert.AreEqual(16, StringHelper.GOOD_FAST_HASH_SEED);
+            // Hashes computed using murmur3_32 from https://code.google.com/p/pyfasthash
+            Assert.AreEqual(0xcd018ef6, (uint)StringHelper.Murmurhash3_x86_32(new BytesRef("foo"), StringHelper.GOOD_FAST_HASH_SEED));
+        }
+
+        [Test]
+        public virtual void TestXMLConfiguration()
+        {
+            // TODO - not working with XML.
+            Assert.AreEqual("0x00000010", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:seed"]);

Review comment:
       Please test `SystemProperties.GetProperty()` here

##########
File path: src/dotnet/tools/Lucene.Net.Tests.Cli/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,96 @@
+using Lucene.Net.Configuration;
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Cli.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Util.SystemProperties.SetProperty(testKey, testValue);
+            Assert.AreEqual(Lucene.Net.Util.SystemProperties.GetProperty(testKey), testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+            Lucene.Net.Util.SystemProperties.SetProperty("tests:locale", "en");

Review comment:
       Please note the request again. Since we determined the `SetProperty` behavior is fundamentally different in .NET, `SystemProperties` should contain only `GetProperty` overloads, no `SetProperty` method.
   
   The point here is that the only way to set the properties should be with .NET APIs and .NET behavior. `ConfigurationSettings.CurrentConfiguration` is a public API for end users, but `SystemProperties` is an internal facade to make it similar in syntax to Java.
   
   However, testing that facade is a valid case. This class is made for testing `SystemProperties`, so it should be testing each of the `GetProperty` calls to verify the correct values are loaded.
   
   

##########
File path: src/dotnet/tools/Lucene.Net.Tests.Cli/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,96 @@
+using Lucene.Net.Configuration;
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Cli.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Util.SystemProperties.SetProperty(testKey, testValue);
+            Assert.AreEqual(Lucene.Net.Util.SystemProperties.GetProperty(testKey), testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+            Lucene.Net.Util.SystemProperties.SetProperty("tests:locale", "en");
+            Assert.AreEqual("en", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+        }
+
+        [Test]
+        public virtual void TestSetandUnset()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+            Lucene.Net.Util.SystemProperties.SetProperty("tests:locale", "en");

Review comment:
       Please note the request again. Since we determined the `SetProperty` behavior is fundamentally different in .NET, `SystemProperties` should contain only `GetProperty` overloads, no `SetProperty` method.
   
   The point here is that the only way to set the properties should be with .NET APIs and .NET behavior. `ConfigurationSettings.CurrentConfiguration` is a public API for end users, but `SystemProperties` is an internal facade to make it similar in syntax to Java.
   
   However, testing that facade is a valid case. This class is made for testing `SystemProperties`, so it should be testing each of the `GetProperty` calls to verify the correct values are loaded.

##########
File path: src/Lucene.Net/Support/Configuration/LuceneDefaultConfigurationProvider.cs
##########
@@ -0,0 +1,217 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Primitives;
+using System;
+using System.Collections;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Linq;
+using System.Security;
+using System.Threading;
+
+namespace Lucene.Net.Configuration
+{
+    /// <summary>
+    /// An environment variable based <see cref="ConfigurationProvider"/>.
+    /// </summary>
+    internal class LuceneDefaultConfigurationProvider : IConfigurationProvider
+    {
+        private readonly bool ignoreSecurityExceptionsOnRead;
+
+        private const string MySqlServerPrefix = "MYSQLCONNSTR_";
+        private const string SqlAzureServerPrefix = "SQLAZURECONNSTR_";
+        private const string SqlServerPrefix = "SQLCONNSTR_";
+        private const string CustomPrefix = "CUSTOMCONNSTR_";
+
+        private const string ConnStrKeyFormat = "ConnectionStrings:{0}";
+        private const string ProviderKeyFormat = "ConnectionStrings:{0}_ProviderName";
+
+        private readonly string _prefix;
+
+        /// <summary>
+        /// Initializes a new instance.
+        /// </summary>
+        public LuceneDefaultConfigurationProvider() : this(string.Empty)
+        { }
+
+        /// <summary>
+        /// Initializes a new instance with the specified prefix.
+        /// </summary>
+        /// <param name="prefix">A prefix used to filter the environment variables.</param>
+        public LuceneDefaultConfigurationProvider(string prefix, bool ignoreSecurityExceptionsOnRead = false)
+        {
+            _prefix = prefix ?? string.Empty;
+            this.ignoreSecurityExceptionsOnRead = ignoreSecurityExceptionsOnRead;
+        }
+
+        /// <summary>
+        /// Loads the environment variables.
+        /// </summary>
+        public void Load()
+        {
+            Data = new ConcurrentDictionary<string, string>();
+            
+            //Load(Environment.GetEnvironmentVariables());

Review comment:
       Please remove this comment

##########
File path: src/Lucene.Net/Lucene.Net.csproj
##########
@@ -70,13 +72,15 @@
     <PackageReference Include="System.Net.Sockets" Version="$(SystemNetSocketsPackageVersion)" />
     <PackageReference Include="System.Threading.Thread" Version="$(SystemThreadingThreadPackageVersion)" />
     <PackageReference Include="System.Threading.ThreadPool" Version="$(SystemThreadingThreadPoolPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="$(MicrosoftExtensionsConfigurationPackageVersion)" />
   </ItemGroup>
 
   <ItemGroup Condition=" '$(TargetFramework)' == 'net45' ">
     <Reference Include="System" />
     <Reference Include="Microsoft.CSharp" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="$(MicrosoftExtensionsConfigurationAbstractionsPackageVersion_NET4_5_1)" />

Review comment:
       Please change the variable name to `$(MicrosoftExtensionsConfigurationAbstractionsPackageVersion)`

##########
File path: src/dotnet/tools/Lucene.Net.Tests.Cli/Configuration/TestDefaultSystemProperties.cs
##########
@@ -0,0 +1,37 @@
+using Lucene.Net.Util;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Cli.Configuration
+{
+
+
+    [TestFixture]
+    class TestDefaultSystemProperties : LuceneTestCase
+    {
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    //ConfigurationFactory = new DefaultConfigurationFactory(false);
+        //    //base.BeforeClass();
+        //}
+        [Test]
+        public virtual void ReadEnvironmentTest()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Assert.AreEqual(testValue, Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey]);
+        }
+        [Test]
+        public virtual void SetEnvironmentTest()
+        {
+            string testKey  = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;
+            Assert.AreEqual(testValue, Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey]);

Review comment:
       Since we determined the `SetProperty` behavior is fundamentally different in .NET, `SystemProperties` should contain only `GetProperty` overloads, no `SetProperty` method.
   
   The point here is that the only way to set the properties should be with .NET APIs and .NET behavior. `ConfigurationSettings.CurrentConfiguration` is a public API for end users, but `SystemProperties` is an internal facade to make it similar in syntax to Java.
   
   However, testing that facade is a valid case. This class is made for testing `SystemProperties`, so it should be testing each of the `GetProperty` calls to verify the correct values are loaded.

##########
File path: src/Lucene.Net/Support/Util/SystemProperties.cs
##########
@@ -163,7 +149,8 @@ private static T GetProperty<T>(string key, T defaultValue, Func<string, T> conv
         /// <exception cref="SecurityException">The caller does not have the required permission to perform this operation.</exception>
         public static void SetProperty(string key, string value)

Review comment:
       I apologize if what I said above wasn't clear. What I meant was that all tests that call **`SystemProperties.SetProperty(key, value)`** should be changed to the syntax `ConfigurationSettings.CurrentConfiguration[key] = value`. The `SystemProperty.GetProperty()` calls should be put back in place in all tests.
   
   `SetProperty` should be removed from `SystemProperties` because its behavior is fundamentally different than the usage of the corresponding method was in Java. Since many calls to `System.setProperty()` in Java require the a call to `System.clearProperty()`, keeping this method here will ultimately end up with calls to this method that don't behave as expected when doing future porting.
   
   But please put the following comment here instead of `SetProperty()`:
   
   ```
   // LUCENENET NOTE: The .NET configuration allows setting individual properties, but individual
   // properties cannot be cleared. For this reason, analogous methods for System.setProperty()
   // and System.clearProperty() in Java were not ported to .NET. In many cases, 
   // IConfigurationRoot.ReloadConfiguration() is a suitable workaround for System.clearProperty(),
   // but there is no direct equivalent that can clear individual properties.
   ```

##########
File path: src/Lucene.Net/Support/Configuration/LuceneDefaultConfigurationProvider.cs
##########
@@ -0,0 +1,217 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Primitives;
+using System;
+using System.Collections;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Linq;
+using System.Security;
+using System.Threading;
+
+namespace Lucene.Net.Configuration
+{
+    /// <summary>
+    /// An environment variable based <see cref="ConfigurationProvider"/>.
+    /// </summary>
+    internal class LuceneDefaultConfigurationProvider : IConfigurationProvider
+    {
+        private readonly bool ignoreSecurityExceptionsOnRead;
+
+        private const string MySqlServerPrefix = "MYSQLCONNSTR_";
+        private const string SqlAzureServerPrefix = "SQLAZURECONNSTR_";
+        private const string SqlServerPrefix = "SQLCONNSTR_";
+        private const string CustomPrefix = "CUSTOMCONNSTR_";
+
+        private const string ConnStrKeyFormat = "ConnectionStrings:{0}";
+        private const string ProviderKeyFormat = "ConnectionStrings:{0}_ProviderName";
+
+        private readonly string _prefix;
+
+        /// <summary>
+        /// Initializes a new instance.
+        /// </summary>
+        public LuceneDefaultConfigurationProvider() : this(string.Empty)
+        { }
+
+        /// <summary>
+        /// Initializes a new instance with the specified prefix.
+        /// </summary>
+        /// <param name="prefix">A prefix used to filter the environment variables.</param>
+        public LuceneDefaultConfigurationProvider(string prefix, bool ignoreSecurityExceptionsOnRead = false)
+        {
+            _prefix = prefix ?? string.Empty;
+            this.ignoreSecurityExceptionsOnRead = ignoreSecurityExceptionsOnRead;
+        }
+
+        /// <summary>
+        /// Loads the environment variables.
+        /// </summary>
+        public void Load()
+        {
+            Data = new ConcurrentDictionary<string, string>();
+            
+            //Load(Environment.GetEnvironmentVariables());
+        }
+        //public bool TryGetEnvironmentVariable(string name, out string value)
+        //{
+        //    try
+        //    {
+        //        value = Environment.GetEnvironmentVariable(name);
+        //        return !string.IsNullOrEmpty(value);
+        //    }
+        //    catch (SecurityException)
+        //    {
+        //    }
+        //}
+
+        //internal void Load(IDictionary envVariables)
+        //{
+        //    Data = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
+
+        //    var filteredEnvVariables = envVariables
+        //        .Cast<DictionaryEntry>()
+        //        .SelectMany(AzureEnvToAppEnv)
+        //        .Where(entry => ((string)entry.Key).StartsWith(_prefix, StringComparison.OrdinalIgnoreCase));
+
+        //    foreach (var envVariable in filteredEnvVariables)
+        //    {
+        //        var key = ((string)envVariable.Key).Substring(_prefix.Length);
+        //        Data[key] = (string)envVariable.Value;
+        //    }
+        //}
+
+        //private static string NormalizeKey(string key)
+        //{
+        //    return key.Replace("__", ConfigurationPath.KeyDelimiter);
+        //}
+
+        //private static IEnumerable<DictionaryEntry> AzureEnvToAppEnv(DictionaryEntry entry)
+        //{
+        //    var key = (string)entry.Key;
+        //    var prefix = string.Empty;
+        //    var provider = string.Empty;
+
+        //    if (key.StartsWith(MySqlServerPrefix, StringComparison.OrdinalIgnoreCase))
+        //    {
+        //        prefix = MySqlServerPrefix;
+        //        provider = "MySql.Data.MySqlClient";
+        //    }
+        //    else if (key.StartsWith(SqlAzureServerPrefix, StringComparison.OrdinalIgnoreCase))
+        //    {
+        //        prefix = SqlAzureServerPrefix;
+        //        provider = "System.Data.SqlClient";
+        //    }
+        //    else if (key.StartsWith(SqlServerPrefix, StringComparison.OrdinalIgnoreCase))
+        //    {
+        //        prefix = SqlServerPrefix;
+        //        provider = "System.Data.SqlClient";
+        //    }
+        //    else if (key.StartsWith(CustomPrefix, StringComparison.OrdinalIgnoreCase))
+        //    {
+        //        prefix = CustomPrefix;
+        //    }
+        //    else
+        //    {
+        //        entry.Key = NormalizeKey(key);
+        //        yield return entry;
+        //        yield break;
+        //    }
+
+        //    // Return the key-value pair for connection string
+        //    yield return new DictionaryEntry(
+        //        string.Format(ConnStrKeyFormat, NormalizeKey(key.Substring(prefix.Length))),
+        //        entry.Value);
+
+        //    if (!string.IsNullOrEmpty(provider))
+        //    {
+        //        // Return the key-value pair for provider name
+        //        yield return new DictionaryEntry(
+        //            string.Format(ProviderKeyFormat, NormalizeKey(key.Substring(prefix.Length))),
+        //            provider);
+        //    }
+        //}
+
+        /// <summary>
+        /// The configuration key value pairs for this provider.
+        /// </summary>
+        protected ConcurrentDictionary<string, string> Data { get; set; }
+
+        /// <summary>
+        /// Attempts to find a value with the given key, returns true if one is found, false otherwise.
+        /// </summary>
+        /// <param name="key">The key to lookup.</param>
+        /// <param name="value">The value found at key if one is found.</param>
+        /// <returns>True if key has a value, false otherwise.</returns>
+        //public virtual bool TryGet(string key, out string value)             => Data.TryGetValue(key, out value);

Review comment:
       Please remove this commented code

##########
File path: src/Lucene.Net/Support/Configuration/LuceneDefaultConfigurationSource.cs
##########
@@ -0,0 +1,28 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.Extensions.Configuration;
+
+namespace Lucene.Net.Configuration
+{
+    /// <summary>
+    /// Represents environment variables as an <see cref="IConfigurationSource"/>.
+    /// </summary>
+    internal class LuceneDefaultConfigurationSource : IConfigurationSource
+    {
+        /// <summary>
+        /// A prefix used to filter environment variables.
+        /// </summary>
+        public string Prefix { get; set; }
+
+        /// <summary>
+        /// Builds the <see cref="LuceneDefaultConfigurationProvider"/> for this source.
+        /// </summary>
+        /// <param name="builder">The <see cref="IConfigurationBuilder"/>.</param>
+        /// <returns>A <see cref="LuceneDefaultConfigurationProvider"/></returns>
+        public IConfigurationProvider Build(IConfigurationBuilder builder)
+        {
+            return new LuceneDefaultConfigurationProvider(Prefix);

Review comment:
       Please pass `ignoreSecurityExceptionsOnRead` through to the `LuceneDefaultConfigurationProvider`.

##########
File path: src/Lucene.Net/Support/Configuration/LuceneDefaultConfigurationProvider.cs
##########
@@ -0,0 +1,217 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Primitives;
+using System;
+using System.Collections;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Linq;
+using System.Security;
+using System.Threading;
+
+namespace Lucene.Net.Configuration
+{
+    /// <summary>
+    /// An environment variable based <see cref="ConfigurationProvider"/>.
+    /// </summary>
+    internal class LuceneDefaultConfigurationProvider : IConfigurationProvider
+    {
+        private readonly bool ignoreSecurityExceptionsOnRead;
+
+        private const string MySqlServerPrefix = "MYSQLCONNSTR_";
+        private const string SqlAzureServerPrefix = "SQLAZURECONNSTR_";
+        private const string SqlServerPrefix = "SQLCONNSTR_";
+        private const string CustomPrefix = "CUSTOMCONNSTR_";
+
+        private const string ConnStrKeyFormat = "ConnectionStrings:{0}";
+        private const string ProviderKeyFormat = "ConnectionStrings:{0}_ProviderName";
+
+        private readonly string _prefix;
+
+        /// <summary>
+        /// Initializes a new instance.
+        /// </summary>
+        public LuceneDefaultConfigurationProvider() : this(string.Empty)
+        { }
+
+        /// <summary>
+        /// Initializes a new instance with the specified prefix.
+        /// </summary>
+        /// <param name="prefix">A prefix used to filter the environment variables.</param>
+        public LuceneDefaultConfigurationProvider(string prefix, bool ignoreSecurityExceptionsOnRead = false)
+        {
+            _prefix = prefix ?? string.Empty;
+            this.ignoreSecurityExceptionsOnRead = ignoreSecurityExceptionsOnRead;
+        }
+
+        /// <summary>
+        /// Loads the environment variables.
+        /// </summary>
+        public void Load()
+        {
+            Data = new ConcurrentDictionary<string, string>();
+            
+            //Load(Environment.GetEnvironmentVariables());
+        }
+        //public bool TryGetEnvironmentVariable(string name, out string value)

Review comment:
       Please remove this commented code block

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,94 @@
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;
+            Assert.AreEqual(Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey], testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"] = "en";
+            Assert.AreEqual("en", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+        }
+
+        [Test]
+        public virtual void TestSetandUnset()

Review comment:
       Please move this test to a new class that is specifically for testing `ConfigurationSettings`

##########
File path: src/Lucene.Net/Support/Configuration/DefaultConfigurationRootFactory.cs
##########
@@ -0,0 +1,53 @@
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Linq;
+using System.Security;
+using System.Text;
+using System.Threading;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Primitives;
+
+namespace Lucene.Net.Configuration
+{
+    internal class DefaultConfigurationRootFactory : IConfigurationRootFactory
+    {
+        private readonly bool ignoreSecurityExceptionsOnRead;
+        private bool initialized = false;
+        protected object m_initializationLock = new object();
+        private readonly IConfigurationBuilder builder;
+        private IConfigurationRoot configuration;
+
+        public DefaultConfigurationRootFactory(bool ignoreSecurityExceptionsOnRead)
+        {
+            this.builder = new ConfigurationBuilder();
+            builder.Add(new LuceneDefaultConfigurationSource() { Prefix = "lucene:" });

Review comment:
       Please pass `ignoreSecurityExceptionsOnRead` through to the `LuceneDefaultConfigurationSource`.

##########
File path: src/Lucene.Net.TestFramework/Lucene.Net.TestFramework.csproj
##########
@@ -58,21 +58,46 @@
     <PackageReference Include="NUnit" Version="$(NUnitPackageVersion)" />
   </ItemGroup>
 
-  <ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.6' ">
+  <!--<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.6' ">
     <PackageReference Include="System.Diagnostics.StackTrace" Version="$(SystemDiagnosticsStackTracePackageVersion)" />
     <PackageReference Include="System.Diagnostics.TraceSource" Version="$(SystemDiagnosticsTraceSourcePackageVersion)" />
     <PackageReference Include="System.Net.Primitives" Version="$(SystemNetPrimitivesPackageVersion)" />
     <PackageReference Include="System.Threading.Thread" Version="$(SystemThreadingThreadPackageVersion)" />
+  </ItemGroup>-->
+
+
+  <ItemGroup>

Review comment:
       Please remove this stray `ItemGroup`

##########
File path: src/Lucene.Net/Support/Configuration/LuceneDefaultConfigurationProvider.cs
##########
@@ -0,0 +1,217 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Primitives;
+using System;
+using System.Collections;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Linq;
+using System.Security;
+using System.Threading;
+
+namespace Lucene.Net.Configuration
+{
+    /// <summary>
+    /// An environment variable based <see cref="ConfigurationProvider"/>.
+    /// </summary>
+    internal class LuceneDefaultConfigurationProvider : IConfigurationProvider
+    {
+        private readonly bool ignoreSecurityExceptionsOnRead;
+
+        private const string MySqlServerPrefix = "MYSQLCONNSTR_";

Review comment:
       Please remove any unused constants

##########
File path: src/Lucene.Net/Lucene.Net.csproj
##########
@@ -70,13 +72,15 @@
     <PackageReference Include="System.Net.Sockets" Version="$(SystemNetSocketsPackageVersion)" />
     <PackageReference Include="System.Threading.Thread" Version="$(SystemThreadingThreadPackageVersion)" />
     <PackageReference Include="System.Threading.ThreadPool" Version="$(SystemThreadingThreadPoolPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="$(MicrosoftExtensionsConfigurationPackageVersion)" />

Review comment:
       Please change the variable name to `$(MicrosoftExtensionsConfigurationAbstractionsPackageVersion)`

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestDefaultSystemProperties.cs
##########
@@ -0,0 +1,37 @@
+using Lucene.Net.Util;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestDefaultSystemProperties : LuceneTestCase
+    {
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    //ConfigurationFactory = new DefaultConfigurationFactory(false);
+        //    //base.BeforeClass();
+        //}
+        [Test]
+        public virtual void ReadEnvironmentTest()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Assert.AreEqual(testValue, Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey]);
+        }
+        [Test]
+        public virtual void SetEnvironmentTest()
+        {
+            string testKey  = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;

Review comment:
       Please do **not** use `SystemProperties.SetProperty()` here, leave like this

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,94 @@
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]

Review comment:
       Please remove this comment block

##########
File path: build/Dependencies.props
##########
@@ -46,6 +46,19 @@
     <MicrosoftCodeAnalysisVisualBasicWorkspacesPackageVersion>3.4.0</MicrosoftCodeAnalysisVisualBasicWorkspacesPackageVersion>
     <MicrosoftCSharpPackageVersion>4.4.0</MicrosoftCSharpPackageVersion>
     <MicrosoftExtensionsDependencyModelPackageVersion>2.0.0</MicrosoftExtensionsDependencyModelPackageVersion>
+
+    <MicrosoftExtensionsConfigurationPackageVersion_NET4_5_1>1.1.2</MicrosoftExtensionsConfigurationPackageVersion_NET4_5_1>
+    <MicrosoftExtensionsConfigurationJsonPackageVersion_NET4_5_1>$(MicrosoftExtensionsConfigurationPackageVersion_NET4_5_1)</MicrosoftExtensionsConfigurationJsonPackageVersion_NET4_5_1>
+    <MicrosoftExtensionsConfigurationXmlPackageVersion_NET4_5_1>$(MicrosoftExtensionsConfigurationPackageVersion_NET4_5_1)</MicrosoftExtensionsConfigurationXmlPackageVersion_NET4_5_1>
+    <MicrosoftExtensionsConfigurationEnvironmentVariablesPackageVersion_NET4_5_1>$(MicrosoftExtensionsConfigurationPackageVersion_NET4_5_1)</MicrosoftExtensionsConfigurationEnvironmentVariablesPackageVersion_NET4_5_1>
+    <MicrosoftExtensionsConfigurationCommandLinePackageVersion_NET4_5_1>$(MicrosoftExtensionsConfigurationPackageVersion_NET4_5_1)</MicrosoftExtensionsConfigurationCommandLinePackageVersion_NET4_5_1>
+
+    <MicrosoftExtensionsConfigurationPackageVersion>3.1.3</MicrosoftExtensionsConfigurationPackageVersion>
+    <MicrosoftExtensionsConfigurationJsonPackageVersion>$(MicrosoftExtensionsConfigurationPackageVersion)</MicrosoftExtensionsConfigurationJsonPackageVersion>

Review comment:
       `MicrosoftExtensionsConfigurationAbstractionsPackageVersion` variable is still missing. Please add it and update all of the corresponding `PackageReference` elements to utilize it instead of `MicrosoftExtensionsConfigurationPackageVersion`.

##########
File path: src/Lucene.Net/Lucene.Net.csproj
##########
@@ -51,10 +51,12 @@
     
   <ItemGroup Condition=" '$(TargetFramework)' == 'netstandard2.1' ">
     <PackageReference Include="Microsoft.CSharp" Version="$(MicrosoftCSharpPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="$(MicrosoftExtensionsConfigurationPackageVersion)" />
   </ItemGroup>
 
   <ItemGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' ">
     <PackageReference Include="Microsoft.CSharp" Version="$(MicrosoftCSharpPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="$(MicrosoftExtensionsConfigurationPackageVersion)" />

Review comment:
       Please change the variable name to `$(MicrosoftExtensionsConfigurationAbstractionsPackageVersion)`

##########
File path: src/Lucene.Net/Support/Configuration/ConfigurationSettings.cs
##########
@@ -0,0 +1,54 @@
+using Lucene.Net.Configuration;
+using Microsoft.Extensions.Configuration;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+
+    public static class ConfigurationSettings
+    {
+        private static IConfigurationRootFactory configurationRootFactory = new DefaultConfigurationRootFactory(false);

Review comment:
       Please change the default setting to `ignoreSecurityExceptionsOnRead: true`

##########
File path: src/Lucene.Net/Support/Configuration/LuceneDefaultConfigurationProvider.cs
##########
@@ -0,0 +1,217 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Primitives;
+using System;
+using System.Collections;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Linq;
+using System.Security;
+using System.Threading;
+
+namespace Lucene.Net.Configuration
+{
+    /// <summary>
+    /// An environment variable based <see cref="ConfigurationProvider"/>.
+    /// </summary>
+    internal class LuceneDefaultConfigurationProvider : IConfigurationProvider
+    {
+        private readonly bool ignoreSecurityExceptionsOnRead;
+
+        private const string MySqlServerPrefix = "MYSQLCONNSTR_";
+        private const string SqlAzureServerPrefix = "SQLAZURECONNSTR_";
+        private const string SqlServerPrefix = "SQLCONNSTR_";
+        private const string CustomPrefix = "CUSTOMCONNSTR_";
+
+        private const string ConnStrKeyFormat = "ConnectionStrings:{0}";
+        private const string ProviderKeyFormat = "ConnectionStrings:{0}_ProviderName";
+
+        private readonly string _prefix;
+
+        /// <summary>
+        /// Initializes a new instance.
+        /// </summary>
+        public LuceneDefaultConfigurationProvider() : this(string.Empty)
+        { }
+
+        /// <summary>
+        /// Initializes a new instance with the specified prefix.
+        /// </summary>
+        /// <param name="prefix">A prefix used to filter the environment variables.</param>
+        public LuceneDefaultConfigurationProvider(string prefix, bool ignoreSecurityExceptionsOnRead = false)

Review comment:
       Please change the default for `ignoreSecurityExceptionsOnRead` to `true`.

##########
File path: src/dotnet/tools/Lucene.Net.Tests.Cli/Configuration/TestDefaultSystemProperties.cs
##########
@@ -0,0 +1,37 @@
+using Lucene.Net.Util;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Cli.Configuration
+{
+
+
+    [TestFixture]
+    class TestDefaultSystemProperties : LuceneTestCase
+    {
+        //[OneTimeSetUp]

Review comment:
       Please remove this comment block

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,94 @@
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;
+            Assert.AreEqual(Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey], testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);

Review comment:
       Please test `SystemProperties.GetProperty()` here

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,94 @@
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;
+            Assert.AreEqual(Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey], testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"] = "en";

Review comment:
       Please test `SystemProperties.GetProperty()` here

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,94 @@
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;
+            Assert.AreEqual(Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey], testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"] = "en";
+            Assert.AreEqual("en", Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"]);

Review comment:
       Please test `SystemProperties.GetProperty()` here

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestDefaultSystemProperties.cs
##########
@@ -0,0 +1,37 @@
+using Lucene.Net.Util;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestDefaultSystemProperties : LuceneTestCase
+    {
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    //ConfigurationFactory = new DefaultConfigurationFactory(false);
+        //    //base.BeforeClass();
+        //}
+        [Test]
+        public virtual void ReadEnvironmentTest()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Assert.AreEqual(testValue, Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey]);
+        }
+        [Test]
+        public virtual void SetEnvironmentTest()
+        {
+            string testKey  = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;
+            Assert.AreEqual(testValue, Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey]);

Review comment:
       Please test `SystemProperties.GetProperty()` here

##########
File path: src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
##########
@@ -685,13 +689,16 @@ public virtual void TearDown()
             /* LUCENENET TODO: Not sure how to convert these
                 ParentChainCallRule.TeardownCalled = true;
                 */
+            ConfigurationSettings.CurrentConfiguration.Reload();

Review comment:
       Rethinking this. This seems like it could cause a major negative performance impact when running all of the Lucene.NET tests. We are reloading all of the configuration for each test file. Perhaps we should just leave it up to the user to call this if they need to during testing. 
   
   It would seem that for the vast majority of users will not benefit from this, only a few who decide to set a configuration variable. The performance cost seems very high for so little benefit.
   
   Thoughts?

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestDefaultSystemProperties.cs
##########
@@ -0,0 +1,37 @@
+using Lucene.Net.Util;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestDefaultSystemProperties : LuceneTestCase
+    {
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    //ConfigurationFactory = new DefaultConfigurationFactory(false);
+        //    //base.BeforeClass();
+        //}
+        [Test]
+        public virtual void ReadEnvironmentTest()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Assert.AreEqual(testValue, Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey]);

Review comment:
       Please test `SystemProperties.GetProperty()` here

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestDefaultSystemProperties.cs
##########
@@ -0,0 +1,37 @@
+using Lucene.Net.Util;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestDefaultSystemProperties : LuceneTestCase
+    {
+        //[OneTimeSetUp]

Review comment:
       Please remove this comment block




----------------------------------------------------------------
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:
users@infra.apache.org



Mime
View raw message