Fix solr search cache location on Windows #3775
Conversation
properly escaped. Also added regression test.
|
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. |
| } | ||
| String dataDir = properties.getProperty("dataDir"); | ||
| assertNotNull(dataDir, "dataDir property from properties file was null: "+file.toString()); | ||
| String fileSeperator = System.getProperty("file.separator"); |
There was a problem hiding this comment.
Just a codestyle detail: you may want to use the more obvious File.separator (or File.separatorChar) for this.
| File corePropertiesFile = corePath.resolve(CORE_PROPERTIES_FILENAME).toFile(); | ||
| Properties coreProperties = new Properties(); | ||
| // we used to append this property to this file. | ||
| // I'm not sure any other properties are ever in here, but jic let's be passive |
There was a problem hiding this comment.
It's very unlikely, since #createCacheCore is supposed to be used only to create a new cache.
| } | ||
| } | ||
| coreProperties.setProperty(DATA_DIR_PROPERTY, dataDir.toString()); | ||
| // Normally we write using the Apache Commons FileUtils, but this is a properties file. |
There was a problem hiding this comment.
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).
| coreProperties.setProperty(DATA_DIR_PROPERTY, dataDir.toString()); | ||
| // Normally we write using the Apache Commons FileUtils, but this is a properties file. | ||
| // Use the standard library Properties class writes it in the proper format. | ||
| try(PrintWriter out = new PrintWriter(corePropertiesFile, StandardCharsets.UTF_8); ) |
There was a problem hiding this comment.
I'm not sure Solr expect an UTF8 properties file, without more information I would use the standard Properties (OutputStream) serializer.
Yes, that test comment and error reporting is very Windows oriented but since you go through Properties, it should work fine on both. |
|
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. |
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
it dates back to 12.10 so any versions still supported need it.