Skip to content

Fix persistence upgrader #5211

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mherwege wants to merge 1 commit into from
Closed

Fix persistence upgrader #5211

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 122 additions & 3 deletions tools/upgradetool/src/main/java/org/openhab/core/tools/internal/PersistenceUpgrader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
*/
package org.openhab.core.tools.internal;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Collection;
Expand All @@ -20,8 +21,10 @@

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.openhab.core.persistence.dto.PersistenceCronStrategyDTO;
import org.openhab.core.persistence.dto.PersistenceItemConfigurationDTO;
import org.openhab.core.persistence.dto.PersistenceServiceConfigurationDTO;
import org.openhab.core.persistence.strategy.PersistenceStrategy;
import org.openhab.core.storage.json.internal.JsonStorage;
import org.openhab.core.tools.Upgrader;
import org.slf4j.Logger;
Expand Down Expand Up @@ -57,11 +60,31 @@
return false;
}

List<String> managedConfigs;
try {
managedConfigs = managedPersistenceConfigs(installedPersistenceAddons(userdataPath),
unmanagedPersistenceConfigs(confPath));

Check failure on line 66 in tools/upgradetool/src/main/java/org/openhab/core/tools/internal/PersistenceUpgrader.java

View workflow job for this annotation

GitHub Actions / Build (Java 21, ubuntu-24.04) 

Null type mismatch (type annotations): required 'java.nio.file.@nonnull Path' but this expression has type 'java.nio.file.@nullable Path'
Comment on lines +65 to +66

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
} catch (IOException e) {
logger.error(e.getMessage());
return false;
}
if (managedConfigs.isEmpty()) {
// No managed persistence configurations, so no need to upgrade
return true;
}

Path persistenceJsonDatabasePath = userdataPath
.resolve(Path.of("jsondb", "org.openhab.core.persistence.PersistenceServiceConfiguration.json"));
if (Files.notExists(persistenceJsonDatabasePath)) {
// No managed persistence configurations, so no need to upgrade
return true;
// No configuration, but persistence addons are installed and there is no unmanaged configuration for it, so
// it needs to be created
try {

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
try {
try {
Path parentDir = persistenceJsonDatabasePath.getParent();
if (parentDir != null) {
Files.createDirectories(parentDir);
}

Copilot uses AI. Check for mistakes.

@florian-h05 florian-h05 Dec 19, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parent directory will be present => jsondb directory is created by openHAB.

Files.createFile(persistenceJsonDatabasePath);
} catch (IOException e) {
logger.error("Cannot create persistence configuration database '{}', check path and access rights.",
persistenceJsonDatabasePath);
return false;
}
}

logger.info("Setting default strategy on managed persistence configurations without strategy '{}'",
Expand All @@ -75,7 +98,22 @@
JsonStorage<PersistenceServiceConfigurationDTO> persistenceStorage = new JsonStorage<>(
persistenceJsonDatabasePath.toFile(), null, 5, 0, 0, List.of());

List.copyOf(persistenceStorage.getKeys()).forEach(serviceId -> {
List<String> persistenceStorageKeys = List.copyOf(persistenceStorage.getKeys());

// Add all installed persistence services without explicit configuration with their previous default
// configuration
List<String> managedConfigsToAdd = managedConfigs.stream()
.filter(serviceId -> !persistenceStorageKeys.contains(serviceId)).toList();
managedConfigsToAdd.forEach(serviceId -> {
PersistenceServiceConfigurationDTO serviceConfigDTO = defaultServiceConfig(serviceId);
if (serviceConfigDTO != null) {
persistenceStorage.put(serviceId, serviceConfigDTO);
logger.info("{}: added strategy configurations", serviceId);
}
});

// Update existing managed configurations
persistenceStorageKeys.forEach(serviceId -> {
PersistenceServiceConfigurationDTO serviceConfigDTO = Objects
.requireNonNull(persistenceStorage.get(serviceId));
Collection<String> defaults = serviceConfigDTO.defaults;
Expand All @@ -97,4 +135,85 @@
persistenceStorage.flush();
return true;
}

private List<String> installedPersistenceAddons(Path userdataPath) throws IOException {
Path addonsConfigPath = userdataPath.resolve("config/org/openhab/addons.config");
if (Files.notExists(addonsConfigPath)) {
throw new IOException(
"Cannot access addon config '" + addonsConfigPath + "', check path and access rights.");
}

List<String> configLines;
configLines = Files.readAllLines(addonsConfigPath);

for (int i = 0; i < configLines.size(); i++) {
String line = Objects.requireNonNull(configLines.get(i));

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +149 to +150

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
for (int i = 0; i < configLines.size(); i++) {
String line = Objects.requireNonNull(configLines.get(i));
for (String lineRaw : configLines) {
String line = Objects.requireNonNull(lineRaw);

Copilot uses AI. Check for mistakes.
if (line.startsWith("persistence")) {
String[] persistenceLine = line.split("=");
if (persistenceLine.length > 1) {
String[] persistenceAddons = persistenceLine[1].replace("\"", "").split(",");
return List.of(persistenceAddons).stream().map(p -> p.trim()).toList();
}
}
}
return List.of();
}

private List<String> unmanagedPersistenceConfigs(Path configPath) throws IOException {
Path persistenceConfigPath = configPath.resolve("persistence");
return Files.list(persistenceConfigPath).filter(configFile -> configFile.endsWith(".persist"))

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
return Files.list(persistenceConfigPath).filter(configFile -> configFile.endsWith(".persist"))
return Files.list(persistenceConfigPath)
.filter(configFile -> configFile.getFileName().toString().endsWith(".persist"))

Copilot uses AI. Check for mistakes.
.map(configFile -> configFile.getFileName().toString().replace(".persist", "")).toList();
Comment on lines +164 to +165

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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();
}

Copilot uses AI. Check for mistakes.

@florian-h05 florian-h05 Dec 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The $OPENHAB_CONF/persistence path will always exist. (If it doesn't there serious things went wrong.)

Comment on lines +164 to +165

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

private List<String> managedPersistenceConfigs(List<String> installedAddons, List<String> unmanagedConfigs) {
return installedAddons.stream().filter(a -> !unmanagedConfigs.contains(a)).toList();
}

private @Nullable PersistenceServiceConfigurationDTO defaultServiceConfig(String serviceId) {
PersistenceItemConfigurationDTO itemConfigDTO = new PersistenceItemConfigurationDTO();
List<String> strategies = defaultPersistenceStrategies(serviceId);
if (strategies != null) {
itemConfigDTO.items = List.of("*");
itemConfigDTO.strategies = strategies;
PersistenceServiceConfigurationDTO serviceConfigDTO = new PersistenceServiceConfigurationDTO();
serviceConfigDTO.serviceId = serviceId;
serviceConfigDTO.configs = List.of(itemConfigDTO);
List<PersistenceCronStrategyDTO> cronStrategies = defaultCronStrategies(serviceId);
if (cronStrategies != null) {
serviceConfigDTO.cronStrategies = cronStrategies;
}
return serviceConfigDTO;
}
return null;
}

private @Nullable List<String> defaultPersistenceStrategies(String service) {
return switch (service) {
case "rrd4j" -> List.of(PersistenceStrategy.Globals.RESTORE.getName(),
PersistenceStrategy.Globals.CHANGE.getName(), "everyMinute");
case "mapdb" ->
List.of(PersistenceStrategy.Globals.RESTORE.getName(), PersistenceStrategy.Globals.CHANGE.getName());
case "inmemory" -> List.of(PersistenceStrategy.Globals.FORECAST.getName());
case "jdbc" -> List.of(PersistenceStrategy.Globals.CHANGE.getName());
case "influxdb" ->
List.of(PersistenceStrategy.Globals.RESTORE.getName(), PersistenceStrategy.Globals.CHANGE.getName());
case "dynamodb" ->
List.of(PersistenceStrategy.Globals.RESTORE.getName(), PersistenceStrategy.Globals.CHANGE.getName());
default -> null;
};
}

private @Nullable List<PersistenceCronStrategyDTO> defaultCronStrategies(String service) {
return switch (service) {
case "rrd4j" -> List.of(everyMinuteStrategy());
default -> null;
};
}

private PersistenceCronStrategyDTO everyMinuteStrategy() {
PersistenceCronStrategyDTO everyMinuteStrategy = new PersistenceCronStrategyDTO();
everyMinuteStrategy.name = "everyMinute";
everyMinuteStrategy.cronExpression = "0 * * * * ?";
return everyMinuteStrategy;
}
}
Loading