Calling AecuService.execute does not do all history checks

I've been working on implementing a way to trigger the migration without using installhooks (see #171).

I actually got something working using this approach

a resource change listener on a certain path listening for trigger events
this can be triggered by an install of a content package
the resource change listener also triggers an execution when activated so scripts with "always" are also picked up every time.

I do however have a problem/issue/question about the execution history and how it is used.

I got this all working by making my listener call AecuService.execute. However, this method does not take into account all execution history. I was able to fix this in 2 separate ways:

Add a dependency to aecu.core and use the HookExecutionHistory class + create the methods that the InstallHook usually uses (getScriptsForExecution, shouldExecute, wasNotExecuted, ...).
Create a class ExecutionHistory or similar myself and just copy the contents from HookExecutionHistory. This eliminates the need for the aecu.core dependency. I still need to create the same boilerplate code myself (getScriptsForExecution, shouldExecute, wasNotExecuted, ...).

Since we don't want to use the aecu.core dependency I've currently settled for option 2.

However, I think this code belongs in the AecuService so it can be used from anywhere, whatever the trigger is (installhook or other), without having to write the ExecutionHistory logic yourself everytime.

This my class (redacted):

import de.valtech.aecu.api.service.AecuException;
import de.valtech.aecu.api.service.AecuService;
import de.valtech.aecu.api.service.ExecutionResult;
import de.valtech.aecu.api.service.ExecutionState;
import de.valtech.aecu.api.service.HistoryEntry;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.Collections;
import java.util.List;
import javax.jcr.Node;
import javax.jcr.RepositoryException;
import javax.jcr.Session;
import org.apache.commons.lang3.StringUtils;
import org.apache.jackrabbit.JcrConstants;
import org.apache.jackrabbit.commons.JcrUtils;
import org.apache.jackrabbit.value.DateValue;
import org.apache.sling.api.resource.LoginException;
import org.apache.sling.api.resource.PersistenceException;
import org.apache.sling.api.resource.Resource;
import org.apache.sling.api.resource.ResourceResolver;
import org.apache.sling.api.resource.ResourceResolverFactory;
import org.apache.sling.api.resource.observation.ResourceChange;
import org.apache.sling.api.resource.observation.ResourceChangeListener;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Reference;
import org.osgi.service.metatype.annotations.AttributeDefinition;
import org.osgi.service.metatype.annotations.Designate;
import org.osgi.service.metatype.annotations.ObjectClassDefinition;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
 * {@link ResourceChangeListener} implementation that listens for a specific, configurable path
 * to trigger the AECU migration without using install hooks.
 */
@Component(
        immediate = true,
        service = ResourceChangeListener.class,
        property = {
                ResourceChangeListener.PATHS + "=" + AecuMigrationTrigger.TRIGGER_LOCATION,
                ResourceChangeListener.CHANGES + "=ADDED"
        }
)
@Designate(ocd = AecuMigrationTrigger.Config.class, factory = true)
public class AecuMigrationTrigger implements ResourceChangeListener {

    @ObjectClassDefinition(
            name = "AECU migration trigger configuration",
            description = "AECU migration trigger configuration"
    )
    public @interface Config {
        @AttributeDefinition(
                name = "Trigger config",
                description = "Trigger configuration"
        )
        String triggerConfig() default "migration-trigger";
    }

    private static final Logger LOGGER = LoggerFactory.getLogger(AecuMigrationTrigger.class);

    static final String TRIGGER_LOCATION = "/conf/aecu/trigger";
    private static final String TRIGGER_TYPE = JcrConstants.NT_UNSTRUCTURED;
    private static final String AECU_TRIGGER_USER = "aecu-migration-trigger";

    @Reference
    private AecuService aecuService;
    @Reference
    private ResourceResolverFactory resourceResolverFactory;
    private Resource triggerResource;

    private String triggerConfig;

    @Activate
    public void activate(final Config config) {
        triggerConfig = config.triggerConfig();
        LOGGER.info("AECU migration trigger listener is active...");

        //do an initial check for existing trigger
        try (ResourceResolver resourceResolver = getResourceResolver()) {
            LOGGER.info("Looking for existing trigger...");
            if (getTriggerResource(resourceResolver) != null) {
                LOGGER.info("Existing trigger found!");
                startAecuMigration(resourceResolver);
            }
        }
    }

    @Override
    public void onChange(List<ResourceChange> changes) {
        try(ResourceResolver resourceResolver = getResourceResolver()) {
            changes.stream()
                    .filter(change -> isMigrationTrigger(change, resourceResolver))
                    .findAny()
                    .ifPresent(change -> {
                        startAecuMigration(resourceResolver);
                    });
        }
    }

    /**
     * Starts the AECU migration
     * @param resourceResolver the resource resolver to use
     */
    private void startAecuMigration(ResourceResolver resourceResolver) {
        try {
            LOGGER.info("AECU migration started");
            List<String> migrationScripts = getScriptsForExecution(aecuService.getFiles(AecuService.AECU_CONF_PATH_PREFIX), resourceResolver);
            if (!migrationScripts.isEmpty()) {
                HistoryEntry installationHistory = executeScripts(migrationScripts, resourceResolver);
                if (HistoryEntry.RESULT.SUCCESS.equals(installationHistory.getResult())) {
                    LOGGER.info("AECU migration executed");
                } else {
                    LOGGER.warn("AECU migration not successfully executed");
                }
            } else {
                LOGGER.info("No AECU groovy scripts to execute");
            }
        } catch(AecuException ae) {
            LOGGER.error("Error while executing AECU migration", ae);
        } finally {
            deleteTrigger(resourceResolver);
        }
    }

    /**
     * Executes groovy scripts with a historical log
     * @param scriptsForExecution the scripts to execute
     * @param resourceResolver the resource resolver to use
     * @return a historical entry for the script execution
     * @throws AecuException
     */
    private HistoryEntry executeScripts(List<String> scriptsForExecution, ResourceResolver resourceResolver)
            throws AecuException {
        HistoryEntry installationHistory = aecuService.createHistoryEntry();
        boolean stopExecution = false;
            for (String groovyScriptPath : scriptsForExecution) {
                ExecutionHistory hookExecutionHistory = new ExecutionHistory(resourceResolver.adaptTo(Session.class), groovyScriptPath);
                try {
                    if (!stopExecution) {
                        installationHistory = executeScript(installationHistory, groovyScriptPath);
                        hookExecutionHistory.setExecuted();
                        if (HistoryEntry.RESULT.FAILURE.equals(installationHistory.getResult())) {
                            // stop execution on first failed script run
                            stopExecution = true;
                        }
                    } else {
                        installationHistory = skipScript(aecuService, installationHistory, groovyScriptPath);
                    }
                }
                catch (AecuException ae) {
                    LOGGER.error("Error while executing groovy script", ae);
                }
            }
            installationHistory = aecuService.finishHistoryEntry(installationHistory);
            return installationHistory;
    }

    /**
     * Executes a single groovy script and stores the execution hisotry
     * @param installationHistory the install history
     * @param groovyScriptPath the path to the grovvy script
     * @return
     * @throws AecuException
     */
    private HistoryEntry executeScript(HistoryEntry installationHistory, String groovyScriptPath)
            throws AecuException {
        ExecutionResult result = aecuService.execute(groovyScriptPath);
        installationHistory = aecuService.storeExecutionInHistory(installationHistory, result);
        return installationHistory;
    }

    /**
     * Filters script candidates that need be executed
     * @param allScriptCandidates
     * @param resourceResolver the resource resolver to use
     * @return list of scripts to be executed
     */
    private List<String> getScriptsForExecution(List<String> allScriptCandidates, ResourceResolver resourceResolver) {
        List<String> scriptsForExecution = new ArrayList<>();
        for (String groovyScriptPath : allScriptCandidates) {
            try {
                ExecutionHistory executionHistory =
                        new ExecutionHistory(resourceResolver.adaptTo(Session.class), groovyScriptPath);
                if (shouldExecute(groovyScriptPath, executionHistory)) {
                    scriptsForExecution.add(groovyScriptPath);
                }
            } catch (AecuException e) {
                LOGGER.error("Could not obtain execution history for " + groovyScriptPath, e);
            }

        }
        return scriptsForExecution;
    }

    /**
     * Checks if a script should be executed
     * @param groovyScriptPath the path to script
     * @param executionHistory  execution history
     * @return <code>true</code> if script should be executed, <code>false</code> if not
     */
    private boolean shouldExecute(String groovyScriptPath, ExecutionHistory executionHistory) {
        if (StringUtils.endsWith(groovyScriptPath, "always.groovy") && isValidScriptPath(groovyScriptPath)) {
            return true;
        }
        boolean wasNotYetExecuted = wasNotExecuted(groovyScriptPath, executionHistory);
        if (wasNotYetExecuted) {
            LOGGER.debug("Force executing as not yet run:" + groovyScriptPath);
        }
        return wasNotYetExecuted;
    }

    private boolean isValidScriptPath(String path) {
        return StringUtils.isNotBlank(path) && aecuService.isValidScriptName(path)
                && (path.startsWith(AecuService.AECU_VAR_PATH_PREFIX) || path.startsWith(AecuService.AECU_CONF_PATH_PREFIX));
    }

    private boolean wasNotExecuted(String path, ExecutionHistory history) {
        return !history.hasBeenExecutedBefore() && (path.startsWith(AecuService.AECU_VAR_PATH_PREFIX) || path.startsWith(AecuService.AECU_CONF_PATH_PREFIX));
    }

    private HistoryEntry skipScript(AecuService aecuService, HistoryEntry installationHistory, String groovyScriptPath)
            throws AecuException {
        ExecutionResult result = new ExecutionResult(ExecutionState.SKIPPED, null, null, null, null, groovyScriptPath);
        installationHistory = aecuService.storeExecutionInHistory(installationHistory, result);
        return installationHistory;
    }

    /**
     * Checks if a changed resource is a trigger to start the AECU migration
     * @param change the changed resource
     * @param resourceResolver the resource resolver to use
     * @return <code>true</code> if migration should start, <code>false</code> if not
     */
    private boolean isMigrationTrigger(ResourceChange change, ResourceResolver resourceResolver) {
         if(buildTriggerPath().equals(change.getPath())
                 && ResourceChange.ChangeType.ADDED.equals(change.getType())) {
             Resource triggerResource = getTriggerResource(resourceResolver);
             return triggerResource != null && TRIGGER_TYPE.equals(triggerResource.getResourceType());
         }
         return false;
    }

    private String buildTriggerPath() {
        return TRIGGER_LOCATION + "/" + triggerConfig;
    }

    /**
     * Deletes the trigger resource from the JCR.
     * @param resourceResolver the resource reoslver used to delete the trigger resource
     */
    private void deleteTrigger(ResourceResolver resourceResolver) {
        try {
            if(triggerResource != null) {
                resourceResolver.delete(triggerResource);
                resourceResolver.commit();
                triggerResource = null;
            }
        } catch(PersistenceException pe) {
            throw new RuntimeException("Could not delete AECU migration-trigger", pe);
        }
    }

    /**
     * Returns the trigger resource from the JCR
     * @return the trigger resource or <code>null</code> if the used {@link ResourceResolver} is null
     *          or the resource can't be found
     * @see #getResourceResolver()
     */
    private Resource getTriggerResource(ResourceResolver resourceResolver) {
        if(triggerResource == null) {
            triggerResource = resourceResolver.getResource(buildTriggerPath());
        }
        return triggerResource;
    }

    /**
     * Returns the resource resolver to be used
     * @return the resource resolver
     */
    private ResourceResolver getResourceResolver() {
        try {
            return resourceResolverFactory.getServiceResourceResolver(
                    Collections.singletonMap(ResourceResolverFactory.SUBSERVICE, AECU_TRIGGER_USER));
        } catch(LoginException le) {
            throw new RuntimeException("Error while logging in", le);
        }
    }

    /**
     * Based on de.valtech.aecu.core.installhook.HookExecutionHistory
     */
    static class ExecutionHistory {
        private static final Logger LOG = LoggerFactory.getLogger(ExecutionHistory.class);
        private static final String HISTORY_BASE_PATH = "/var/aecu-installhook";
        private static final String PN_EXECUTED = "executed";
        private final Node hookHistory;

        public ExecutionHistory(Session session, String groovyScriptPath) throws AecuException {
            try {
                String fullPath = HISTORY_BASE_PATH + groovyScriptPath;
                this.hookHistory = JcrUtils.getOrCreateByPath(fullPath, false, "nt:unstructured", "nt:unstructured", session, true);
            } catch (RepositoryException re) {
                throw new AecuException("Error getting or creating node at /var/aecu-installhook" + groovyScriptPath, re);
            }
        }

        public boolean hasBeenExecutedBefore() {
            boolean hasBeenExecuted = false;

            try {
                hasBeenExecuted = this.hookHistory.hasProperty(PN_EXECUTED);
            } catch (RepositoryException re) {
                LOG.error(re.getMessage(), re);
            }

            return hasBeenExecuted;
        }

        public void setExecuted() throws AecuException {
            try {
                this.hookHistory.setProperty(PN_EXECUTED, new DateValue(Calendar.getInstance()));
                this.hookHistory.getSession().save();
            } catch (RepositoryException re) {
                throw new AecuException("Could not set property executed", re);
            }
        }
    }

}

Unless I'm approaching this incorrectly and there's a better way to get all script candidates taking into account their previous execution history?

EDIT: if you want to test this class, just provide a service user aecu-migration-trigger-user with read+write on /var and /conf and map it to service aecu-migration-trigger

valtech / aem-easy-content-upgrade

Calling AecuService.execute does not do all history checks #172