I've implemented this invariant and it works like a charm. Did you know TimeZone.getDefault() has a side-effect of setting user.timezone system property? If you did, I'll buy you a beer next time we meet

Anyway, lots of Solr tests leave behind system properties. Instead of trying to fix them one by one I went kind of the easy route and created a class and test rule that simply reverts all system properties from before the entry scope. This has been applied to AbstractSolrTestCase and SolrTestCaseJ4 and since these are subclasses of LuceneTestCase, the invariant will always hold, no matter what.

If Solr folks wish to fix tests one by one (which may be a good idea or a bad idea – there's lots of them), then comment out these two:

  @ClassRule
  public static TestRule solrClassRules = 
    RuleChain.outerRule(new SystemPropertiesRestoreRule());

  @Rule
  public TestRule solrTestRules = 
    RuleChain.outerRule(new SystemPropertiesRestoreRule());

Patch with changes.

+1

Properties implements java.util.Map<Object,Object>, clone as Map can simply be a "new WhateverMap(System.getProperties())". Although it's <Object,Object>, it should only contain Strings... Can we be sure?

I don't know, to be honest. I didn't want to experiment because I remember being scalded at some point in my past when I used Map interface to manipulate system properties and in certain circumstances things didn't work as they should. I can't remember the details now, unfortunately.

As for non Strings and nulls – I assume only System.setProperty/clearProperty is used to manipulate the map; if so, it should only contain Strings and it should have non-null keys and values.

I don't know, to be honest. I didn't want to experiment because I remember being scalded at some point in my past when I used Map interface to manipulate system properties and in certain circumstances things didn't work as they should. I can't remember the details now, unfortunately.

For copying you are already using the map interface (retrieving entrySet). For read access this is not a problem, but the whole Java API implementing Map is broken. Implementing the Map interface, which allows to put Object keys/values is unfortunately a no-go. But doing "new TreeMap<String,String>((Map<String,String>) properties)" is fine (with SuppressWarnings because of the cast) - because the putAll method of TreeMap is implemented exactly as your loop.

Yeah, I know. I actually felt bad about entrySet too – I think the safest way would be to use enumeration, but it felt ancient. I'll leave as is if you don't mind. It's a bit more explicit, but won't hurt anybody I think.

I don't mind, I think it's fine. But I had the same problem like you when I used the Properties class, it always feels bad and some things are not working at all.

This comment in Javadocs for public Set<String> stringPropertyNames() makes me nervous, too:

Returns a set of keys in this property list where the key and its corresponding value are strings, including distinct keys in the default property list if a key of the same name has not already been found from the main properties list. Properties whose key or value is not of type String are omitted.
The returned set is not backed by the Properties object. Changes to this Properties are not reflected in the set, or vice versa.
Returns:
a set of keys in this property list where the key and its corresponding value are strings, including the keys in the default property list.
Since:
1.6

This method is available since Java 6, so this would be the "most correct" solution. Retrieve the set, iterate via advanced for-loop and use getProperty(). But that does not work for Lucene 3.x.

As far as I remember, the main problem with the Map interface in Properties is the values coming from the parent "defaults" object. When you wrap another properties object with one that has the other one as "defaults" given in ctor, then your are fcked up if you are using the Map interface. Which is not the case for system properties, but that's not guaranteed (e.g. the Java JVM could set some basic properties in another Properties object and make the changes available only through the overlayed properties object).

To fix the "defaults" problem I would go with the Enumeration<?> (works in Java 5, too) in a "for(Enumeration<?> e=propertyNames(); e.hasMoreElements()" loop.

If you use the Set interface, the inherited props would not have been seen and the system may delete properties which are in a parent/"defaults" properties object.

Yeah, it might have been that that I experienced. So back to enumeration we go

About your commit: It should be getProperty() [without cast] and not get()... Otherwise the defaults are again fcked up

Right, good catch. Properties are seriously messed up, aren't they?

Horrible!!! I just say: die, die, die

Now I remember what it was that I had a problem with. Properties has the notion of "defaults" so it falls back to another Properties by default if the key is not present. Unfortunately get() is not overridden in Properties (and everything else is) so things like propertyNames or getProperty will return a value from the fallback set, but get() won't.

  public static void main(String[] args) {
    Properties defaults = new Properties();
    Properties p = new Properties(defaults);
    String key = "custom-key";
    String value = "value";
    defaults.put(key, value);
    for (Enumeration<?> e = p.propertyNames(); e.hasMoreElements();) {
      Object currentKey = e.nextElement();
      if (currentKey == key) {
        System.out.println("Default key found in propertyNames(), but...");
        System.out.println("p.get()=" + p.get(key));
        System.out.println("p.getProperty()=" + p.getProperty(key));
      }
    }
  }

You can easily corrupt other programs by inserting Object values into the system property set – any enumeration will then fail with a classcast. Still don't remember where this caused a problem but it definitely was.

As far as I remember, the main problem with the Map interface in Properties is the values coming from the parent "defaults" object.

Exactly, I'm kind of slow today.

No prob. That is exactly the problem I wanted to prevent with the Enumeration and getProperty(). Code using those 2 functions is the only correct one to list all properties...

This is great Dawid, thanks!

Strangely i trip the timezone issue when running any solr tests from Eclipse... but not lucene tests?

E.g. if i run TestDemo from lucene its fine, but if i run TestRussianFilter (org.apache.solr.analysis) then i hit:

java.lang.AssertionError: System properties invariant violated.
Different values:
  [old]user.timezone=
  [new]user.timezone=America/New_York

	at org.apache.lucene.util.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:46)
	at org.junit.rules.RunRules.evaluate(RunRules.java:18)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)

Well... something is changing it, the question is what it is. I'll take a look.

I know what's changing it. Eh. So – there is a warning being printed:

Mar 22, 2012 6:20:33 PM org.apache.solr.core.Config parseLuceneVersionString
WARNING: You should not use LUCENE_CURRENT as luceneMatchVersion property: if you use this setting, and then Solr upgrades to a newer release of Lucene, sizable changes may happen. If precise back compatibility is important then you should instead explicitly specify an actual Lucene version.
Mar 22, 2012 6:20:33 PM org.apache.solr.analysis.BaseTokenStreamFactory warnDeprecated
WARNING: RussianLetterTokenizerFactory is deprecated. Use StandardTokenizerFactory instead.

These warnings go through Java logging and this in turn is localized (date format, "warning" info, etc.). This in turn asks for the default TimeZone and this in turn sets the system property (I mentioned it a while ago).

I suggest that we just ignore user.timezone as it is triggered from multiple locations and doesn't seem that important?

I suggest that we just ignore user.timezone as it is triggered from multiple locations and doesn't seem that important?

+1, we know its a side effect of our testcase itself randomizing the locale/timezone...

Applied a fix for this. user.timezone is ignored (and is not reset).

Thanks Dawid... these tests are now working in my IDE