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 Mon, 18 May 2020 05:38:14 GMT

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



##########
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:
       Reviewing the source code in Java, the test framework doesn't do this on its own. It
keeps a private dictionary named `restoreProperties` that cleans up **only the test framework's
temporarily set state.** It has no effect on the state that is set in tests.
   
   In Java there is a `System.clearProperty(String)` method that is used to "undo" the state.
   
   Setting a value to `null` with `Environment.SetEnvironmentVariable` resets it to the state
of the machine's environment variable. This behavior is the workaround we did to make it work
in a similar way in .NET.
   
   ## Untested Solution
   
   It looks like the best workaround is to add a `ConcurrentDictionary` to use as a cache
in `SystemProperties`. We can then add a `SystemProperties.ClearProperty(string key)` method
to mimic the Java experience.
   
   1. `SetProperty` writes the setting to the cache instead of writing it to the underlying
configuration provider
   2. `GetProperty` calls `TryGetValue` on the cache
     a. If `true`, return the value that the method "got"
     b. If `false`, return the value from the configuration provider
   3. `ClearProperty` removes the item from the cache
   
   Since our IConfigurationFactory might be doing its own caching of IConfiguration, each
instance of `IConfiguration` should have its own cache of properties.
   
   This implementation should work, but needs testing to verify:
   
   ```c#
       /// <summary>
       /// Helper for system properties. This class helps to convert configuration provider
settings
       /// to <see cref="int"/> or <see cref="bool"/> data types.
       /// </summary>
       public static class SystemProperties
       {
           private static readonly ConcurrentDictionary<IConfiguration, ConcurrentDictionary<string,
string>> settingsCache
               = new ConcurrentDictionary<IConfiguration, ConcurrentDictionary<string,
string>>();
   
           /// <summary>
           /// Retrieves the value of a property from the current configuration cache.
           /// </summary>
           /// <param name="key">The name of the environment variable.</param>
           /// <returns>The environment variable value.</returns>
           public static string GetProperty(string key)
           {
               return GetProperty(key, null);
           }
   
           /// <summary>
           /// Retrieves the value of a property from the current configuration cache,
           /// with a default value if it doens't exist or the caller doesn't have 
           /// permission to read the value.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           /// <param name="defaultValue">The value to use if the property does not
exist 
           /// or the caller doesn't have permission to read the value.</param>
           /// <returns>The environment variable value.</returns>
           public static string GetProperty(string key, string defaultValue)
           {
               return GetProperty(key, defaultValue, (str) => str);
           }
   
           /// <summary>
           /// Retrieves the value of a property from the current configuration cache
           /// as <see cref="bool"/>. If the value cannot be cast to <see cref="bool"/>,
returns <c>false</c>.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           /// <returns>The property value.</returns>
           public static bool GetPropertyAsBoolean(string key)
           {
               return GetPropertyAsBoolean(key, false);
           }
   
           /// <summary>
           /// Retrieves the value of a property from the current configuration cache as <see
cref="bool"/>, 
           /// with a default value if it doens't exist, the caller doesn't have permission
to read the value, 
           /// or the value cannot be cast to a <see cref="bool"/>.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           /// <param name="defaultValue">The value to use if the property does not
exist,
           /// the caller doesn't have permission to read the value, or the value cannot be
cast to <see cref="bool"/>.</param>
           /// <returns>The environment variable value.</returns>
           public static bool GetPropertyAsBoolean(string key, bool defaultValue)
           {
               return GetProperty(key, defaultValue,
                   (str) =>
                   {
                       return bool.TryParse(str, out bool value) ? value : defaultValue;
                   }
               );
           }
   
           /// <summary>
           /// Retrieves the value of a property from the current process
           /// as <see cref="int"/>. If the value cannot be cast to <see cref="int"/>,
returns <c>0</c>.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           /// <returns>The property value.</returns>
           public static int GetPropertyAsInt32(string key)
           {
               return GetPropertyAsInt32(key, 0);
           }
   
           /// <summary>
           /// Retrieves the value of a property from the current process as <see cref="int"/>,

           /// with a default value if it doens't exist, the caller doesn't have permission
to read the value, 
           /// or the value cannot be cast to a <see cref="int"/>.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           /// <param name="defaultValue">The value to use if the property does not
exist,
           /// the caller doesn't have permission to read the value, or the value cannot be
cast to <see cref="int"/>.</param>
           /// <returns>The property value.</returns>
           public static int GetPropertyAsInt32(string key, int defaultValue)
           {
               return GetProperty<int>(key, defaultValue,
                   (str) =>
                   {
                       return int.TryParse(str, out int value) ? value : defaultValue;
                   }
               );
           }
   
           private static T GetProperty<T>(string key, T defaultValue, Func<string,
T> conversionFunction)
           {
               IConfiguration configuration = ConfigurationSettings.GetConfigurationFactory().CreateConfiguration();
               var cache = GetConfigurationCacheFor(configuration);
   
               // Only get the value from the underlying configuration provider if the value
is not already cached.
               if (!cache.TryGetValue(key, out string setting))
                   setting = configuration[key];
   
               return (setting == null)
                   ? defaultValue
                   : conversionFunction(setting);
           }
   
           private static ConcurrentDictionary<string, string> GetConfigurationCacheFor(IConfiguration
configuration)
           {
               // Create a separate cache for each IConfiguration instance. This will allow
the IConfigurationFactory
               // to utilize any caching mechanism it needs, but allows us to cache settings
here rather than writing them
               // to the underlying provider (which has different behavior in .NET than the
Java system properties).
               // This allows us to also implement a ClearProperty method here, even though
no such feature exists in IConfiguration.
               return settingsCache
                   .GetOrAdd(configuration, (configuration) => new ConcurrentDictionary<string,
string>());
           }
   
           /// <summary>
           /// Creates or modifies a property stored in the current configuration cache.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           /// <param name="value">The new property value.</param>
           public static void SetProperty(string key, string value)
           {
               IConfiguration configuration = ConfigurationSettings.GetConfigurationFactory().CreateConfiguration();
               var cache = GetConfigurationCacheFor(configuration);
   
               // Do not set the key to the configuration provider, since it has different
behavior than what
               // was expected of system properties in Java. Instead, we use a local cache
that is based
               // on the IConfiguration instance.
               cache[key] = value;
           }
   
           /// <summary>
           /// Clears a property stored in the current configuration cache.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           public static void ClearProperty(string key)
           {
               IConfiguration configuration = ConfigurationSettings.GetConfigurationFactory().CreateConfiguration();
               var cache = GetConfigurationCacheFor(configuration);
   
               cache.TryRemove(key, out string _);
           }
       }
   ```
   
   > Please note that I updated the documentation comments above also.
   
   > I recommend the caching of each directory be verified before testing this solution,
as it depends on the same caching behavior.
   
   > The docs for `ConfigurationSettings/IConfigurationFactory` will need to make it clear
to end users that should they implement their own overrides of `Equals` or `GetHashCode` on
`IConfiguration`, it may affect how the instance (and therefore writing settings) is cached.
   
   ## Updating the Application
   
   All tests that call `SetProperty` are then responsible for calling `ClearProperty` to clear
the key that was set in the test in order to avoid polluting the state of other tests.
   
   Any existing tests that call `System.clearProperty` in Java Lucene 4.8.0 should be modified
to call `SystemProperties.ClearProperty` in Lucene.NET.




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