Fix persistence upgrader - #5211
Conversation
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
|
@kaikreuzer For your review. |
There was a problem hiding this comment.
Pull request overview
This PR fixes the persistence upgrader to properly handle persistence addon configurations without explicit configuration files. The fix addresses issue #5210 by creating default persistence configurations for installed persistence addons that don't have unmanaged (file-based) configurations.
Key Changes:
- Added logic to detect installed persistence addons and create default configurations for those without explicit .persist files
- Modified the file creation logic to handle cases where the persistence configuration database doesn't exist
- Implemented default strategy configurations for various persistence services (rrd4j, mapdb, inmemory, jdbc, influxdb, dynamodb)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return Files.list(persistenceConfigPath).filter(configFile -> configFile.endsWith(".persist")) | ||
| .map(configFile -> configFile.getFileName().toString().replace(".persist", "")).toList(); |
There was a problem hiding this comment.
The method does not handle the case where the persistence directory does not exist, which will cause an IOException when calling Files.list(). Consider checking if the directory exists first and returning an empty list if it doesn't, rather than propagating the exception.
| return Files.list(persistenceConfigPath).filter(configFile -> configFile.endsWith(".persist")) | |
| .map(configFile -> configFile.getFileName().toString().replace(".persist", "")).toList(); | |
| if (Files.notExists(persistenceConfigPath)) { | |
| return List.of(); | |
| } | |
| try (var stream = Files.list(persistenceConfigPath)) { | |
| return stream.filter(configFile -> configFile.endsWith(".persist")) | |
| .map(configFile -> configFile.getFileName().toString().replace(".persist", "")).toList(); | |
| } |
There was a problem hiding this comment.
The $OPENHAB_CONF/persistence path will always exist. (If it doesn't there serious things went wrong.)
| configLines = Files.readAllLines(addonsConfigPath); | ||
|
|
||
| for (int i = 0; i < configLines.size(); i++) { | ||
| String line = Objects.requireNonNull(configLines.get(i)); |
There was a problem hiding this comment.
The Objects.requireNonNull() call is unnecessary since configLines.get(i) will never return null for a valid list index. The call also causes confusion about the actual nullability contract. Remove this call to improve code clarity.
| return true; | ||
| // No configuration, but persistence addons are installed and there is no unmanaged configuration for it, so | ||
| // it needs to be created | ||
| try { |
There was a problem hiding this comment.
Creating the parent directories before creating the file is missing. If the jsondb directory doesn't exist, Files.createFile() will fail with a NoSuchFileException. Use Files.createDirectories() on the parent directory first to ensure the path exists.
| try { | |
| try { | |
| Path parentDir = persistenceJsonDatabasePath.getParent(); | |
| if (parentDir != null) { | |
| Files.createDirectories(parentDir); | |
| } |
There was a problem hiding this comment.
Parent directory will be present => jsondb directory is created by openHAB.
| for (int i = 0; i < configLines.size(); i++) { | ||
| String line = Objects.requireNonNull(configLines.get(i)); |
There was a problem hiding this comment.
The loop variable i is declared but never used except to iterate through the list. Replace the traditional for-loop with an enhanced for-loop to improve readability and eliminate the unused loop variable.
| for (int i = 0; i < configLines.size(); i++) { | |
| String line = Objects.requireNonNull(configLines.get(i)); | |
| for (String lineRaw : configLines) { | |
| String line = Objects.requireNonNull(lineRaw); |
| managedConfigs = managedPersistenceConfigs(installedPersistenceAddons(userdataPath), | ||
| unmanagedPersistenceConfigs(confPath)); |
There was a problem hiding this comment.
The confPath parameter is not null-checked before being passed to unmanagedPersistenceConfigs(). If confPath is null, this will cause a NullPointerException when the method tries to resolve the persistence path. Add a null check for confPath similar to the check for userdataPath.
|
|
||
| private List<String> unmanagedPersistenceConfigs(Path configPath) throws IOException { | ||
| Path persistenceConfigPath = configPath.resolve("persistence"); | ||
| return Files.list(persistenceConfigPath).filter(configFile -> configFile.endsWith(".persist")) |
There was a problem hiding this comment.
The Path.endsWith() method expects a Path argument, not a String. This call should use getFileName().toString().endsWith(".persist") to properly check the file extension. The current code will cause a compilation error or unexpected behavior.
| return Files.list(persistenceConfigPath).filter(configFile -> configFile.endsWith(".persist")) | |
| return Files.list(persistenceConfigPath) | |
| .filter(configFile -> configFile.getFileName().toString().endsWith(".persist")) |
| return Files.list(persistenceConfigPath).filter(configFile -> configFile.endsWith(".persist")) | ||
| .map(configFile -> configFile.getFileName().toString().replace(".persist", "")).toList(); |
There was a problem hiding this comment.
The Files.list() operation returns a Stream that must be closed to avoid resource leaks. Wrap this operation in a try-with-resources block to ensure the stream is properly closed after use.
|
I will take care of Copilot and the build failure. |
|
See #5212. |
|
Superseeded |
Closes #5210