fix(application): Application version comparison (#2724)

* Use a [semver version for comparison instead of a String](https://www.baeldung.com/java-comparing-versions) * Make sure to cover all cases of the `switch` statement * Do not try to disable a process already disabled * Update configuration in transaction * Improve log output of ImportStatus Closes [DEV-499](https://bonitasoft.atlassian.net/browse/DEV-499) Closes [DEV-518](https://bonitasoft.atlassian.net/browse/DEV-518) Closes [DEV-519](https://bonitasoft.atlassian.net/browse/DEV-519)
bonitasoft · Sep 19, 2023 · 8c96d2f · 8c96d2f
1 parent 50f2c79
commit 8c96d2f
Show file tree

Hide file tree

Showing 10 changed files with 162 additions and 118 deletions.
diff --git a/...cal/src/test/java/org/bonitasoft/engine/application/installer/ApplicationInstallerIT.java b/...cal/src/test/java/org/bonitasoft/engine/application/installer/ApplicationInstallerIT.java
@@ -16,6 +16,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 
+import java.io.File;
 import java.util.Optional;
 
 import org.bonitasoft.engine.CommonAPIIT;
@@ -117,11 +118,10 @@ public void custom_application_should_be_installed_with_configuration() throws E
 
         // when:
         try (var applicationAsStream = ApplicationInstallerIT.class
-                .getResourceAsStream("/simple-app-1.0.0-SNAPSHOT-local.zip");
-                var confStream = ApplicationInstallerIT.class
-                        .getResourceAsStream("/simple-app-1.0.0-SNAPSHOT-local.bconf");) {
+                .getResourceAsStream("/simple-app-1.0.0-SNAPSHOT-local.zip")) {
             var applicationArchive = applicationArchiveReader.read(applicationAsStream);
-            applicationArchive.setConfigurationFile(Optional.of(confStream));
+            applicationArchive.setConfigurationFile(Optional.of(new File(ApplicationInstallerIT.class
+                    .getResource("/simple-app-1.0.0-SNAPSHOT-local.bconf").getFile())));
             applicationInstallerImpl.install(applicationArchive, "1.0.0-SNAPSHOT");
         }
 

diff --git a/...ommon/src/main/java/org/bonitasoft/engine/exception/ApplicationInstallationException.java b/...ommon/src/main/java/org/bonitasoft/engine/exception/ApplicationInstallationException.java
@@ -13,7 +13,7 @@
  **/
 package org.bonitasoft.engine.exception;
 
-public class ApplicationInstallationException extends Exception {
+public class ApplicationInstallationException extends RuntimeException {
 
     public ApplicationInstallationException(String message) {
         super(message);

diff --git a/bpm/bonita-core/bonita-process-engine/build.gradle b/bpm/bonita-core/bonita-process-engine/build.gradle
@@ -5,6 +5,7 @@ plugins {
 dependencies {
     api platform("org.bonitasoft.engine:bonita-artifacts-model-dependencies:${Deps.bonitaArtifactsModelVersion}")
 
+    api "com.vdurmont:semver4j:${Deps.semver4j}"
     api "org.bonitasoft.engine:bonita-organization-model"
     api project(':bpm:bonita-core:bonita-home-server')
     api project(':bpm:bonita-core:bonita-actor-mapping')
@@ -85,6 +86,7 @@ dependencies {
     compileOnly "org.projectlombok:lombok:${Deps.lombokVersion}"
 
     testImplementation "org.junit.jupiter:junit-jupiter-api:${Deps.junit5Version}"
+    testImplementation "org.junit.jupiter:junit-jupiter-params:${Deps.junit5Version}"
     testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine:${Deps.junit5Version}"
     testRuntimeOnly "org.junit.vintage:junit-vintage-engine:${Deps.junit5Version}"
     testAnnotationProcessor "org.projectlombok:lombok:${Deps.lombokVersion}"

diff --git a/...rc/main/java/org/bonitasoft/engine/api/impl/application/installer/ApplicationArchive.java b/...rc/main/java/org/bonitasoft/engine/api/impl/application/installer/ApplicationArchive.java
@@ -15,7 +15,6 @@
 
 import java.io.File;
 import java.io.IOException;
-import java.io.InputStream;
 import java.nio.file.Files;
 import java.util.ArrayList;
 import java.util.List;
@@ -46,7 +45,7 @@ public class ApplicationArchive implements AutoCloseable {
     private List<File> ignoredFiles = new ArrayList<>();
 
     @Singular
-    private Optional<InputStream> configurationFile = Optional.empty();
+    private Optional<File> configurationFile = Optional.empty();
 
     public ApplicationArchive addPage(File page) {
         pages.add(page);

diff --git a/.../main/java/org/bonitasoft/engine/api/impl/application/installer/ApplicationInstaller.java b/.../main/java/org/bonitasoft/engine/api/impl/application/installer/ApplicationInstaller.java
@@ -48,7 +48,6 @@
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
-import java.io.InputStream;
 import java.io.Serializable;
 import java.net.URLConnection;
 import java.nio.file.Files;
@@ -84,6 +83,7 @@
 import org.bonitasoft.engine.bpm.bar.BusinessArchive;
 import org.bonitasoft.engine.bpm.bar.BusinessArchiveFactory;
 import org.bonitasoft.engine.bpm.bar.InvalidBusinessArchiveFormatException;
+import org.bonitasoft.engine.bpm.process.ActivationState;
 import org.bonitasoft.engine.bpm.process.Problem;
 import org.bonitasoft.engine.bpm.process.ProcessDefinitionNotFoundException;
 import org.bonitasoft.engine.bpm.process.ProcessDeployException;
@@ -236,8 +236,8 @@ protected List<Long> installArtifacts(ApplicationArchive applicationArchive, Exe
         installOrUpdateThemes(applicationArchive, executionResult);
         installLivingApplications(applicationArchive, executionResult, FAIL_ON_DUPLICATES);
         var installedProcessIds = installProcesses(applicationArchive, executionResult);
-        installConfigurationFileIfPresent(applicationArchive.getConfigurationFile(),
-                executionResult);
+        applicationArchive.getConfigurationFile().ifPresent(configFile -> installConfiguration(configFile,
+                executionResult));
         return installedProcessIds;
     }
 
@@ -323,8 +323,8 @@ protected List<Long> updateArtifacts(ApplicationArchive applicationArchive, Exec
         installLivingApplications(applicationArchive, executionResult, ApplicationImportPolicy.REPLACE_DUPLICATES);
 
         List<Long> newlyInstalledProcessIds = installProcesses(applicationArchive, executionResult);
-        installConfigurationFileIfPresent(applicationArchive.getConfigurationFile(),
-                executionResult);
+        applicationArchive.getConfigurationFile().ifPresent(configFile -> installConfiguration(configFile,
+                executionResult));
         return newlyInstalledProcessIds;
     }
 
@@ -364,10 +364,12 @@ public void disableOldProcesses(List<Long> installedProcessIds, ExecutionResult
         for (Long processId : deployedProcessIds) {
             // get process Info
             ProcessDeploymentInfo info = getProcessDeploymentInfo(processId);
-            disableProcess(processId);
-            executionResult.addStatus(infoStatus(PROCESS_DEPLOYMENT_DISABLEMENT_OK,
-                    format("Process %s (%s) has been disabled successfully",
-                            info.getDisplayName(), info.getVersion())));
+            if (info.getActivationState() == ActivationState.ENABLED) {
+                disableProcess(processId);
+                executionResult.addStatus(infoStatus(PROCESS_DEPLOYMENT_DISABLEMENT_OK,
+                        format("Process %s (%s) has been disabled successfully",
+                                info.getDisplayName(), info.getVersion())));
+            }
         }
     }
 
@@ -795,25 +797,40 @@ protected List<Long> installProcesses(ApplicationArchive applicationArchive, Exe
         return processDefinitionIds;
     }
 
-    void installConfigurationFileIfPresent(Optional<InputStream> configurationFileArchive,
+    /**
+     * Must be called in a transaction with active session
+     *
+     * @param configurationFileArchive
+     * @param executionResult
+     * @throws ApplicationInstallationException
+     */
+    void installConfiguration(File configurationFileArchive,
             ExecutionResult executionResult)
             throws ApplicationInstallationException {
-        try {
-            configurationFileArchive.ifPresent(confFile -> {
-                log.info("Installing application configuration from file");
-                try {
-                    installationService.install(null, FileOperations.readFully(confFile));
-                    executionResult.addStatus(Status.infoStatus(Status.okStatus().getCode(),
-                            "Configuration file has been imported"));
-                } catch (IOException | InstallationFailedException e) {
-                    throw new RuntimeException(e);
-                }
-            });
-        } catch (Exception e) {
+        try (var is = Files.newInputStream(configurationFileArchive.toPath())) {
+            log.info("Installing application configuration from file");
+            installationService.install(null, is.readAllBytes());
+            executionResult.addStatus(Status.infoStatus(Status.okStatus().getCode(),
+                    "Configuration file has been imported"));
+        } catch (IOException | InstallationFailedException e) {
             throw new ApplicationInstallationException("The Application Archive install operation has been aborted", e);
         }
     }
 
+    /**
+     * Update configuration with the given bconf file
+     *
+     * @param configurationFileArchive A bconf file
+     * @param executionResult
+     * @throws Exception
+     */
+    public void updateConfiguration(File configurationFileArchive, ExecutionResult executionResult) throws Exception {
+        inSession(() -> inTransaction(() -> {
+            installConfiguration(configurationFileArchive, executionResult);
+            return null;
+        }));
+    }
+
     protected Long deployProcess(BusinessArchive businessArchive, ExecutionResult executionResult)
             throws ProcessDeployException {
         final String processName = businessArchive.getProcessDefinition().getName();
@@ -877,8 +894,23 @@ private ServiceAccessor getServiceAccessor() {
     void logInstallationResult(ExecutionResult result) {
         log.info("Result of the installation of the application:");
         for (Status s : result.getAllStatus()) {
-            log.info("[{}] - {} - {} - {}", s.getLevel(), s.getCode(), s.getMessage(),
-                    s.getContext().toString());
+            var message = s.getContext() != null && !s.getContext().isEmpty()
+                    ? String.format("%s - %s - %s", s.getCode(), s.getMessage(),
+                            s.getContext().toString())
+                    : String.format("%s - %s", s.getCode(), s.getMessage());
+            switch (s.getLevel()) {
+                case ERROR:
+                    log.error(message);
+                    break;
+                case WARNING:
+                    log.warn(message);
+                    break;
+                case INFO:
+                case OK:
+                default:
+                    log.info(message);
+                    break;
+            }
         }
     }
 
@@ -889,4 +921,5 @@ static class IconContent {
         private byte[] bytes;
         private String mimeType;
     }
+
 }
diff --git a/...bonitasoft/engine/api/impl/application/installer/CustomOrDefaultApplicationInstaller.java b/...bonitasoft/engine/api/impl/application/installer/CustomOrDefaultApplicationInstaller.java
@@ -22,6 +22,8 @@
 import java.util.Properties;
 import java.util.zip.ZipFile;
 
+import com.vdurmont.semver4j.Semver;
+import com.vdurmont.semver4j.Semver.SemverType;
 import lombok.Getter;
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
@@ -51,6 +53,8 @@
 @RequiredArgsConstructor
 public class CustomOrDefaultApplicationInstaller {
 
+    private static final String VERSION_PROPERTY = "version";
+
     public static final String CUSTOM_APPLICATION_DEFAULT_FOLDER = "my-application";
 
     public static final String VERSION_FILE_NAME = "application.properties";
@@ -106,32 +110,26 @@ public void autoDeployDetectedCustomApplication(PlatformStartedEvent event)
             log.info("Bonita now tries to install it automatically...");
             installCustomApplication(customApplication, newVersion);
         } else {
-            final String currentVersion = applicationVersionService.retrieveApplicationVersion();
+            var currentVersion = new Semver(applicationVersionService.retrieveApplicationVersion(), SemverType.LOOSE);
             log.info("Detected application version: '{}'; Current deployed version: '{}'",
                     newVersion,
                     currentVersion);
-            switch (newVersion.compareTo(currentVersion)) {
-                case 0: {
-                    log.info("Updating process configuration only ...");
-                    findAndUpdateConfiguration();
-                    break;
-                }
-                case 1: {
-                    log.info("Updating the application ...");
-                    updateCustomApplication(customApplication, newVersion);
-                    break;
-                }
-                case -1: {
-                    throw new ApplicationInstallationException("An application has been detected, but its newVersion "
-                            + newVersion + " is inferior to the one deployed: " + currentVersion
-                            + ". Nothing will be updated, and the Bonita engine startup has been aborted.");
-                }
+            if (newVersion.isGreaterThan(currentVersion)) {
+                log.info("Updating the application...");
+                updateCustomApplication(customApplication, newVersion);
+            } else if (newVersion.isEquivalentTo(currentVersion)) {
+                log.info("Updating process configuration only...");
+                findAndUpdateConfiguration();
+            } else {
+                throw new ApplicationInstallationException("An application has been detected, but its newVersion "
+                        + newVersion + " is inferior to the one deployed: " + currentVersion
+                        + ". Nothing will be updated, and the Bonita engine startup has been aborted.");
             }
         }
     }
 
     @VisibleForTesting
-    Optional<String> readApplicationVersion(Resource customApplication) throws IOException {
+    Optional<Semver> readApplicationVersion(Resource customApplication) throws IOException {
         if (customApplication != null) {
             try (var customApplicationZip = new ZipFile(customApplication.getFile())) {
                 var applicationPropertiesEntry = customApplicationZip.getEntry(VERSION_FILE_NAME);
@@ -141,12 +139,20 @@ Optional<String> readApplicationVersion(Resource customApplication) throws IOExc
                 var properties = new Properties();
                 var applicationPropertiesInputStream = customApplicationZip.getInputStream(applicationPropertiesEntry);
                 properties.load(applicationPropertiesInputStream);
-                return Optional.ofNullable(properties.getProperty("version"));
+                return Optional.ofNullable(toSemver(properties.getProperty(VERSION_PROPERTY)));
             }
         }
         return Optional.empty();
     }
 
+    @VisibleForTesting
+    Semver toSemver(String version) {
+        if (version == null) {
+            return null;
+        }
+        return new Semver(version, SemverType.LOOSE);
+    }
+
     boolean isPlatformFirstInitialization() {
         return mandatoryLivingApplicationImporter.isFirstRun();
     }
@@ -198,12 +204,12 @@ Resource[] getCustomAppResourcesFromClasspath() throws IOException {
      * @param version
      * @throws ApplicationInstallationException if unable to install the application
      */
-    protected void updateCustomApplication(final Resource customApplication, String version)
+    protected void updateCustomApplication(final Resource customApplication, Semver version)
             throws Exception {
         try (final InputStream applicationZipFileStream = customApplication.getInputStream();
                 ApplicationArchive applicationArchive = getApplicationArchive(applicationZipFileStream)) {
             setConfigurationFile(applicationArchive);
-            applicationInstaller.update(applicationArchive, version);
+            applicationInstaller.update(applicationArchive, version.getValue());
         } catch (IOException | ApplicationInstallationException e) {
             throw new ApplicationInstallationException(
                     "Unable to update the application " + customApplication.getFilename(), e);
@@ -215,7 +221,7 @@ private void setConfigurationFile(ApplicationArchive applicationArchive)
         detectConfigurationFile().ifPresent(resource -> {
             try {
                 log.info("Found application configuration file " + resource.getFilename());
-                applicationArchive.setConfigurationFile(Optional.of(resource.getInputStream()));
+                applicationArchive.setConfigurationFile(Optional.of(resource.getFile()));
             } catch (IOException e) {
                 throw new UncheckedIOException(e);
             }
@@ -227,11 +233,11 @@ private void setConfigurationFile(ApplicationArchive applicationArchive)
      * @param version
      * @throws ApplicationInstallationException if unable to install the application
      */
-    protected void installCustomApplication(final Resource customApplication, String version) throws Exception {
+    protected void installCustomApplication(final Resource customApplication, Semver version) throws Exception {
         try (final InputStream applicationZipFileStream = customApplication.getInputStream();
                 ApplicationArchive applicationArchive = getApplicationArchive(applicationZipFileStream)) {
             setConfigurationFile(applicationArchive);
-            applicationInstaller.install(applicationArchive, version);
+            applicationInstaller.install(applicationArchive, version.getValue());
         } catch (IOException | ApplicationInstallationException e) {
             throw new ApplicationInstallationException(
                     "Unable to install the application " + customApplication.getFilename(), e);
@@ -291,10 +297,9 @@ protected void findAndUpdateConfiguration() throws ApplicationInstallationExcept
         detectConfigurationFile().ifPresent(resource -> {
             try {
                 log.info("Found application configuration file " + resource.getFilename());
-                applicationInstaller
-                        .installConfigurationFileIfPresent(Optional.of(resource.getInputStream()), executionResult);
-            } catch (IOException | ApplicationInstallationException e) {
-                throw new RuntimeException(e);
+                applicationInstaller.updateConfiguration(resource.getFile(), executionResult);
+            } catch (Exception e) {
+                throw new ApplicationInstallationException("Failed to update configuration.", e);
             }
         });
         applicationInstaller.logInstallationResult(executionResult);

diff --git a/...t/java/org/bonitasoft/engine/api/impl/application/installer/ApplicationInstallerTest.java b/...t/java/org/bonitasoft/engine/api/impl/application/installer/ApplicationInstallerTest.java
@@ -43,7 +43,6 @@
 
 import java.io.File;
 import java.io.IOException;
-import java.io.InputStream;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -394,11 +393,12 @@ public void should_not_fail_when_no_organization() throws Exception {
     public void install_should_call_install_configuration_file_on_installation_service() throws Exception {
         // given:
         final ExecutionResult executionResult = new ExecutionResult();
-        final Optional<InputStream> inputStream = Optional.of(mock(InputStream.class));
         doNothing().when(installationService).install(eq(null), any());
 
         // when:
-        applicationInstaller.installConfigurationFileIfPresent(inputStream, executionResult);
+        applicationInstaller.installConfiguration(
+                new File(ApplicationInstaller.class.getResource("/RequestLoan_conf_with_null_params.bconf").getFile()),
+                executionResult);
 
         // then:
         verify(installationService).install(eq(null), any());

diff --git a/.../org/bonitasoft/engine/api/impl/application/installer/ApplicationInstallerUpdateTest.java b/.../org/bonitasoft/engine/api/impl/application/installer/ApplicationInstallerUpdateTest.java
@@ -218,9 +218,11 @@ public void should_install_and_enable_resolved_process_and_disable_previous_vers
 
         ProcessDeploymentInfo deployedProcess1 = spy(ProcessDeploymentInfo.class);
         doReturn("1.1").when(deployedProcess1).getVersion();
+        doReturn(ActivationState.ENABLED).when(deployedProcess1).getActivationState();
 
         ProcessDeploymentInfo deployedProcess2 = spy(ProcessDeploymentInfo.class);
         doReturn("1.2").when(deployedProcess2).getVersion();
+        doReturn(ActivationState.ENABLED).when(deployedProcess2).getActivationState();
 
         doReturn(deployedProcess1).when(applicationInstaller).getProcessDeploymentInfo(1L);
         doReturn(deployedProcess2).when(applicationInstaller).getProcessDeploymentInfo(2L);