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 Sun, 17 May 2020 16:11:10 GMT

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



##########
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:
       Please provide an MSBuild variable for `MicrosoftExtensionsConfigurationAbstractionsPackageVersion`.
We are getting build warnings because Lucene.NET requires it.

##########
File path: src/Lucene.Net.TestFramework/Lucene.Net.TestFramework.csproj
##########
@@ -58,20 +58,54 @@
     <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>
   </ItemGroup>
 
-  <ItemGroup Condition=" '$(TargetFramework)' == 'net45' ">
+
+  <ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
+    <PackageReference Include="Microsoft.Extensions.Configuration" Version="$(MicrosoftExtensionsConfigurationPackageVersion)"
/>

Review comment:
       Please correct the package version variables here to use their corresponding names

##########
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>

Review comment:
       Please provide an MSBuild variable for `MicrosoftExtensionsConfigurationAbstractionsPackageVersion_NET4_5_1`.
We are getting build warnings because Lucene.NET requires it.

##########
File path: src/Lucene.Net.TestFramework/Util/TestRuleSetupAndRestoreClassEnv.cs
##########
@@ -287,7 +287,7 @@ public override void Before(LuceneTestCase testInstance)
 
             // TimeZone.getDefault will set user.timezone to the default timezone of the
user's locale.
             // So store the original property value and restore it at end.
-            restoreProperties["user.timezone"] = SystemProperties.GetProperty("user.timezone");
+            restoreProperties["user:timezone"] = SystemProperties.GetProperty("user:timezone");

Review comment:
       Please comment this
   
   ```c#
   // restoreProperties["user:timezone"] = SystemProperties.GetProperty("user:timezone");
// LUCENENET specific - .NET doesn't have a way to set timezone at the user level
   ```

##########
File path: src/Lucene.Net.TestFramework/Lucene.Net.TestFramework.csproj
##########
@@ -58,20 +58,54 @@
     <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>
   </ItemGroup>
 
-  <ItemGroup Condition=" '$(TargetFramework)' == 'net45' ">
+
+  <ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
+    <PackageReference Include="Microsoft.Extensions.Configuration" Version="$(MicrosoftExtensionsConfigurationPackageVersion)"
/>
+    <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="$(MicrosoftExtensionsConfigurationPackageVersion)"
/>
+    <PackageReference Include="Microsoft.Extensions.Configuration.Xml" Version="$(MicrosoftExtensionsConfigurationPackageVersion)"
/>
+    <PackageReference Include="Microsoft.Extensions.Configuration.EnvironmentVariables"
Version="$(MicrosoftExtensionsConfigurationPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.CommandLine" Version="$(MicrosoftExtensionsConfigurationPackageVersion)"
/>
+  </ItemGroup>
+
+  <ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.1'">
+    <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="$(MicrosoftExtensionsConfigurationJsonPackageVersion)"
/>
+    <PackageReference Include="Microsoft.Extensions.Configuration.Xml" Version="$(MicrosoftExtensionsConfigurationXmlPackageVersion)"
/>
+    <PackageReference Include="Microsoft.Extensions.Configuration.EnvironmentVariables"
Version="$(MicrosoftExtensionsConfigurationEnvironmentVariablesPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.CommandLine" Version="$(MicrosoftExtensionsConfigurationCommandLinePackageVersion)"
/>
+  </ItemGroup>
+  
+  <ItemGroup Condition=" '$(TargetFramework)' == 'net451' ">
     <Reference Include="System.IO.Compression" />
     <Reference Include="System.Numerics" />
     <Reference Include="System.ServiceModel" />
     <Reference Include="System" />
     <Reference Include="Microsoft.CSharp" />
+    <PackageReference Include="Microsoft.Extensions.Configuration" Version="$(MicrosoftExtensionsConfigurationPackageVersion_NET4_5_1)"
/>

Review comment:
       Please correct the package version variables here to use their corresponding names

##########
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.Util.SystemProperties.SetProperty(testKey, testValue);
+            Assert.AreEqual(Lucene.Net.Util.SystemProperties.GetProperty(testKey), testValue);
+            Assert.Pass();
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr-FR", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+            Lucene.Net.Util.SystemProperties.SetProperty("tests:locale", "en_EN");

Review comment:
       Setting a property without unsetting it is polluting the SystemProperty state of other
tests, causing random failures.

##########
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.Util.SystemProperties.SetProperty(testKey, testValue);

Review comment:
       Setting a property without unsetting it is polluting the SystemProperty state of other
tests, causing random failures.

##########
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.Util.SystemProperties.SetProperty(testKey, testValue);
+            Assert.AreEqual(Lucene.Net.Util.SystemProperties.GetProperty(testKey), testValue);
+            Assert.Pass();
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr-FR", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+            Lucene.Net.Util.SystemProperties.SetProperty("tests:locale", "en_EN");

Review comment:
       `en_EN` is not a valid locale and causes multiple tests to fail. For a neutral English
use `en`.

##########
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.Util.SystemProperties.SetProperty(testKey, testValue);
+            Assert.AreEqual(Lucene.Net.Util.SystemProperties.GetProperty(testKey), testValue);
+            Assert.Pass();

Review comment:
       Please remove all calls to `Assert.Pass()`, as this causes the debugger to stop on
every test when debugging.

##########
File path: src/Lucene.Net.Tests.TestFramework/config.runsettings
##########
@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="utf-8"?>
+<RunSettings>
+  <!-- Parameters used by tests at runtime -->
+  <TestRunParameters>
+    <Parameter name="cli" value="localhost-runsettings" />
+    <Parameter name="lucene.version" value="4.8.3" />
+
+    <Parameter name="tests:locale" value="fr-FR" />
+    <Parameter name="tests:timezone" value="SE Asia Standard Time" />
+    <Parameter name="user.timezone" value="SE Asia Standard Time" />

Review comment:
       Please remove all references to `user.timezone` or `user:timezone`

##########
File path: src/Lucene.Net/Support/Configuration/DefaultConfigurationFactory.cs
##########
@@ -36,7 +35,8 @@ protected IConfiguration EnsureInitialized()
         {
             return LazyInitializer.EnsureInitialized(ref this.configuration, ref this.initialized,
ref this.m_initializationLock, () =>
             {
-                return Initialize();
+                this.configuration = Initialize();

Review comment:
       This isn't what I meant, but it is a positive change. The `configuration` member variable
is being set twice. Please change to
   
   ```c#
   protected IConfiguration EnsureInitialized() 
   { 
   	return LazyInitializer.EnsureInitialized(ref this.configuration, ref this.initialized,
ref this.m_initializationLock, () => 
   	{ 
   		return Initialize();
   	});
   }
   ```

##########
File path: src/Lucene.Net/Support/Configuration/LuceneDefaultConfigurationProvider.cs
##########
@@ -0,0 +1,194 @@
+// 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.Generic;
+using System.Linq;
+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()
+        {
+            Load(Environment.GetEnvironmentVariables());

Review comment:
       `Environment.GetEnvironmentVariables()` is not safe to call because it may throw a
`SecurityException` before it returns anything. Rather than loading all environment variables
at once (with a given prefix), I suggest to change the implementation to use the `GetOrAdd()`
method of `ConcurrentDictionary` to make what is now the Data property work like a cache.
In the delegate method for the add portion, you can put in error handling to catch any security
exception for a key the user doesn't have permission to read.

##########
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();

Review comment:
       Note that this isn't the correct way to set the ConfigurationFactory from a test subclass,
it should be done in a static constructor. 
   
   ```c#
   static TestSystemProperties()
   {
       ConfigurationFactory = new TestConfigurationFactory(); // This example is moot, because
this is the default setting of ConfigurationFactory
   }
   ```
   
   The way this is written will reset the cache back to an empty state for each test class
(only once). This configuration may cause unintended problems. Since `TestConfigurationFactory`
is the default, there is no reason to set it or override `BeforeClass()` here.
   
   If you intended to reset the cache on every test, then you should instead override `SetUp()`
and put these settings there. Do note, you won't be able to test the caching in a class that
does this, because each test will reset its state.




----------------------------------------------------------------
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