Fix solr search cache location on Windows

[jameswartell]

Jira URL

https://jira.xwiki.org/browse/XWIKI-22741

Changes

In xwiki-platform/xwiki-platform-search-solr-api/src/main/java/org/xwiki/search/solr/internal/EmbeddedSolr.java:

Instead of appending a string to the properties file, load any existing properties file using the java.util.Properties class, set the property, then save it using the method on the class.

Description

The dataDir property in \xwiki\store\solr\search\core.properties" is not being written in the proper format. This showed up specifically because backslashes require escape on Windows, but not using the java.util.Properties standard library class to write properties files may cause other yet to be encountered formatting issues. The comment in the Properties store method says "Converts unicodes to encoded \uxxxx and escapes special characters with a preceding slash".

The effect was that the property ends up as "......cachesolrsearch" instead of "......\cache\solr\search" so the solr cache ends up in "\store\solr\search......cachesolrsearch" instead of \cache\solr\search".

Using blame the issue goes back to 12.10.

Executed Tests

I added a test to: xwiki-platform-search-solr-api/src/test/java/org/xwiki/search/solr/internal/EmbeddedSolrInitializationTest.java

I confirmed that this test fails without the code change and passes with it. I wasn't sure if tests are always run on supported OS's before release, but this showed up as a problem on Windows, and I expect the test will only find the regression on Windows.

Expected merging strategy

• Prefers squash: Yes - I found I had misnamed the property after testing. Whoops.
• Backport on branches:

it dates back to 12.10 so any versions still supported need it.

[jameswartell]

I had a weird minute where I thought my test would fail on Linux, but it won't. It should just use a different file.seperator that doesn't require escape.

[tmortagne]

I don't think you need to mention the fact that FileUtils used to be used, using Properties tool is more correct and there is no reason to justify it (that's more something for the pull request explanation than the code).

[tmortagne]

I had a weird minute where I thought my test would fail on Linux, but it won't. It should just use a different file.seperator that doesn't require escape.

Yes, that test comment and error reporting is very Windows oriented but since you go through Properties, it should work fine on both.

[tmortagne]

General note: your changes don't follow XWiki codestyle. The build is passing because EmbeddedSolr is excluded from checkstyle for other reasons right now. In case you are using Eclipse or IntelliJ you can find some helper configuration on https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/#HConfiguringyourIDEtousetheXWikicodestyle. It's fine for a first contribution, I can clean that up a bit, just wanted to give all the details if you feel like contributing more.

[jameswartell]

I'm trying to create a test with the cache in the wrong location, but once Solr is started I can't seem to get it to shut down again resulting in errors when I try to start it again and my code tries to cleanup the bad location.

I've tried many combinations with the objects I've found like:

       Solr instance = this.componentManager.getInstance(Solr.class, EmbeddedSolr.TYPE);
       assertNotNull(instance);

       EmbeddedSolr implementation = (EmbeddedSolr) instance;
       CoreContainer container = implementation.getContainer();
       SolrCore core = container.getCore(container.getLoadedCoreNames().get(0)); 

       implementation.getClient("search").close();
       core.close();
       core.closeAndWait();
       core.getCoreContainer().shutdown();
       container.shutdown();

       core = null;
       container = null;
       componentManager.release(instance);
       instance = null;

But the files stay locked

[tmortagne]

I'm trying to create a test with the cache in the wrong location, but once Solr is started I can't seem to get it to shut down again resulting in errors when I try to start it again and my code tries to cleanup the bad location.

It's not very clear to me why you need to restart Solr. I would expect this test to be about preparing some files before Solr starts and make sure they are migrated as expected.

That being said, I'm not sure why you need to actually start Solr, wouldn't testing EmbeddedSolr in isolation be enough ? That's what the various tests in https://github.com/xwiki/xwiki-platform/blob/master/xwiki-platform-core/xwiki-platform-search/xwiki-platform-search-solr/xwiki-platform-search-solr-api/src/test/java/org/xwiki/search/solr/internal/EmbeddedSolrInitializationTest.java are doing (preparing some stuff on filesystem and check how EmbeddedSolrInitializationTest behaves).

[jameswartell]

It's not very clear to me why you need to restart Solr. I would expect this test to be about preparing some files before Solr starts and make sure they are migrated as expected.

That being said, I'm not sure why you need to actually start Solr, wouldn't testing EmbeddedSolr in isolation be enough ? That's what the various tests in https://github.com/xwiki/xwiki-platform/blob/master/xwiki-platform-core/xwiki-platform-search/xwiki-platform-search-solr/xwiki-platform-search-solr-api/src/test/java/org/xwiki/search/solr/internal/EmbeddedSolrInitializationTest.java are doing (preparing some stuff on filesystem and check how EmbeddedSolrInitializationTest behaves).

So when you unpack the default core from the zip file there is no cache. The "core.properties" file is empty (which makes sense since it needs an OS specific file separator). I ended up just writing a file and a nested file where the cache files would be, and setting the property, but it seems to me like it would be a better test to use actual cache files.

I don't move the cache files. I delete them. I wasn't sure if they self-reference their absolute location at all, so it seemed safer to just start over. Seems like it is expected the cache sometimes needs to be wiped anyway (for new versions of solr for instance). I'll check in what I've got.

[jameswartell]

ready for re-review

[tmortagne]

I feel like it would make more sense to move all the analysis regarding the bad core properties in isSearchCoreValid(). After all that's the whole point of that method.

[jameswartell]

My thinking was "isSearchCoreValid()" had no side effects previously. It merely performed checks. I thought it was kind of surprising if one says, 'hey check if the core is valid', because if it is I'll recreate it, but now, what was a check, actually starts deleting files off the hard drive. Also there was already some clean-up that happens in the other ("else") logical branch (where if the core isn't found at the expected location it tries to delete it from an old location, so this as written isn't really any different than that).

Alternatively, I could check for the bug in isSearchCoreValid() (without doing the clean-up), and then do the clean-up inside the logical "if" branch that follows the call, except that ends up looking practically identical to what I already have. Most of the code would need to be duplicated to do the check and then again for the clean up.

But I'm not really married to either way. If you prefer it this way, it's fine with me.

[tmortagne]

Thanks a lot for all the work @jameswartell, and sorry I'm not more reactive these days…

Just a last details, and I think we are good.