Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ public void apply(ExecutionRuntime executionRuntime) {

@Override
public void rollback(ExecutionRuntime executionRuntime) {
if (rollbackMethod == null) {
// Invariant violation: callers must gate rollback() on hasRollback(). Surfacing this
// as a meaningful error instead of letting it manifest as an NPE inside reflection.
throw new IllegalStateException("Change [" + getId() + "] has no @RollbackExecution method; "
+ "rollback() must not be called. Callers should gate on hasRollback().");
}
executeInternal(executionRuntime, rollbackMethod);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,15 @@ public interface ExecutableChange extends ChangeDescriptor {

void rollback(ExecutionRuntime executionRuntime);

/**
* Returns {@code true} when this change declares a rollback (annotated method on a code-based
* change, or {@code @Rollback}-bearing template). Callers must gate any invocation of
* {@link #rollback(ExecutionRuntime)} on this — invoking rollback when no rollback method is
* declared is an invariant violation. Every implementation must answer it explicitly so the
* decision is never left to a silent default.
*/
boolean hasRollback();

String getRollbackMethodName();

boolean isAlreadyApplied();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,10 @@ public String getRollbackMethodName() {
return rollbackMethod.getName();
}

@Override
public boolean hasRollback() {
return rollbackMethod != null;
}


}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ public void apply(ExecutionRuntime executionRuntime) {

@Override
public void rollback(ExecutionRuntime executionRuntime) {
if (rollbackMethod == null) {
// Invariant violation: callers must gate rollback() on hasRollback(). Surfacing this
// as a meaningful error instead of letting it manifest as an NPE inside reflection.
throw new IllegalStateException("Change [" + getId() + "] has no @RollbackExecution method; "
+ "rollback() must not be called. Callers should gate on hasRollback().");
}
logger.debug("Rolling back change[{}] with template: {}", loadedChange.getId(), loadedChange.getTemplateClass());
executeInternal(executionRuntime, rollbackMethod);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,16 @@ public void logAutoRollback(ExecutableChange executableChange, long duration) {
logger.info("Change rolled back [change={} duration={}]", executableChange.getId(), formattedDuration);
}

/**
* Logs a single WARN line when a failed change cannot be rolled back because it doesn't
* declare a {@code @RollbackExecution} method. The audit entry stays at {@code FAILED} (no
* manual-rollback entry follows); manual intervention is surfaced by the execution report
* separately, so this line stays terse.
*/
public void logRollbackSkippedNoMethodDeclared(String changeId) {
logger.warn("Rollback skipped [change={}]: no rollback method provided", changeId);
}

public void logManualRollbackResult(ManualRolledBackStep rolledBack) {
String changeId = rolledBack.getChange().getId();
String duration = formatDuration(rolledBack.getDuration());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
import io.flamingock.internal.util.log.FlamingockLoggerFactory;
import org.slf4j.Logger;

import java.util.Optional;

/**
* Change process strategy for non-transactional target systems.
*
Expand Down Expand Up @@ -119,9 +121,17 @@ protected ChangeProcessResult doApplyChange() {
}

private void rollbackActualChangeAndChain(FailedAfterExecutionAuditStep rollableFailedStep, ExecutionContext executionContext) {
RollableStep rollableStep = rollableFailedStep.getRollbackStep();
ManualRolledBackStep rolledBack = targetSystemOps.rollbackChange(rollableStep::rollback, buildExecutionRuntime());
stepLogger.logManualRollbackResult(rolledBack);
auditAndLogManualRollback(rolledBack, executionContext);
Optional<RollableStep> rollableStepOpt = rollableFailedStep.getRollbackStep();
if (rollableStepOpt.isPresent()) {
ManualRolledBackStep rolledBack = targetSystemOps.rollbackChange(
rollableStepOpt.get()::rollback, buildExecutionRuntime());
stepLogger.logManualRollbackResult(rolledBack);
auditAndLogManualRollback(rolledBack, executionContext);
} else {
// No @RollbackExecution declared: don't invoke rollback, don't write a manual-rollback
// audit entry. The upstream FAILED audit entry already written by auditAndLogExecution
// is the truth, and recovery is gated by the configured RecoveryStrategy.
stepLogger.logRollbackSkippedNoMethodDeclared(rollableFailedStep.getChange().getId());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
import io.flamingock.internal.util.log.FlamingockLoggerFactory;
import org.slf4j.Logger;

import java.util.Optional;

/**
* Change process strategy for transactional target systems with shared audit store.
*
Expand Down Expand Up @@ -149,10 +151,18 @@ private void auditIfExecutionFailure(Wrapper<ExecutionStep> executionStep) {

private void rollbackChain(RollableFailedStep rollableFailedStep, ExecutionContext executionContext) {
// Skip first rollback (main change) as transaction already rolled it back
RollableStep rollableStep = rollableFailedStep.getRollbackStep();
ManualRolledBackStep rolledBack = targetSystemOps.rollbackChange(rollableStep::rollback, buildExecutionRuntime());
stepLogger.logManualRollbackResult(rolledBack);
auditAndLogManualRollback(rolledBack, executionContext);
Optional<RollableStep> rollableStepOpt = rollableFailedStep.getRollbackStep();
if (rollableStepOpt.isPresent()) {
ManualRolledBackStep rolledBack = targetSystemOps.rollbackChange(
rollableStepOpt.get()::rollback, buildExecutionRuntime());
stepLogger.logManualRollbackResult(rolledBack);
auditAndLogManualRollback(rolledBack, executionContext);
} else {
// No @RollbackExecution declared: don't invoke rollback, don't write a manual-rollback
// audit entry. The upstream FAILED audit entry stands as the truth, and recovery is
// gated by the configured RecoveryStrategy.
stepLogger.logRollbackSkippedNoMethodDeclared(rollableFailedStep.getChange().getId());
}
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,18 @@

import io.flamingock.internal.core.change.navigation.step.afteraudit.RollableStep;

import java.util.Optional;

public interface RollableFailedStep extends FailedStep {

RollableStep getRollbackStep();
/**
* Returns the rollback step when the failed change declares a rollback, or {@link Optional#empty()}
* when it doesn't. {@code @RollbackExecution} is optional, so a non-transactional change that fails
* without declaring one has no rollback to invoke. Callers must branch on the Optional and skip the
* rollback path (and the manual-rollback audit entry) when absent — the upstream {@code FAILED}
* audit entry is the truth, and recovery follows the configured {@code RecoveryStrategy}.
*/
Optional<RollableStep> getRollbackStep();


}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import io.flamingock.internal.core.change.executable.ExecutableChange;
import io.flamingock.internal.util.Result;

import java.util.Optional;

public abstract class FailedAfterExecutionAuditStep extends AfterExecutionAuditStep
implements SuccessableStep, RollableFailedStep, FailedWithErrorStep {

Expand All @@ -43,8 +45,12 @@ protected FailedAfterExecutionAuditStep(ExecutableChange change, boolean success
}

@Override
public final RollableStep getRollbackStep() {
return new RollableStep(getChange());
public final Optional<RollableStep> getRollbackStep() {
// Only produce a rollback step when the change actually declares one — otherwise the
// caller would invoke change.rollback(...) on a null reflection Method and NPE.
return getChange().hasRollback()
? Optional.of(new RollableStep(getChange()))
: Optional.empty();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,20 @@
import io.flamingock.internal.core.change.navigation.step.rolledback.RolledBackStep;
import io.flamingock.internal.core.change.executable.ExecutableChange;

import java.util.Optional;

public class CompleteAutoRolledBackStep extends RolledBackStep implements SuccessableStep, RollableFailedStep {
public CompleteAutoRolledBackStep(ExecutableChange change, boolean rollbackSuccess) {
super(change, rollbackSuccess);
}


@Override
public final RollableStep getRollbackStep() {
return new RollableStep(getChange());
public final Optional<RollableStep> getRollbackStep() {
// Mirrors FailedAfterExecutionAuditStep.getRollbackStep(): absent when the change has no
// @RollbackExecution method, so callers can skip the rollback path without NPE'ing.
return getChange().hasRollback()
? Optional.of(new RollableStep(getChange()))
: Optional.empty();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/*
* Copyright 2026 Flamingock (https://www.flamingock.io)
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.flamingock.internal.core.change.executable;

import io.flamingock.internal.common.core.recovery.action.ChangeAction;
import io.flamingock.internal.core.change.loaded.AbstractReflectionLoadedChange;
import io.flamingock.internal.core.runtime.ExecutionRuntime;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import java.lang.reflect.Method;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;

/**
* Covers the {@code hasRollback()} contract and the defensive guard on {@code rollback()}
* for code-based changes. The bug we're closing: when a non-transactional change without an
* {@code @RollbackExecution} method failed, callers would invoke {@code rollback()} on a null
* reflection method and surface as an NPE inside reflection. The guard converts that into a
* meaningful {@code IllegalStateException} and {@code hasRollback()} lets callers avoid the
* call entirely.
*/
class CodeExecutableChangeRollbackGuardTest {

@Test
@DisplayName("hasRollback() returns true when the rollback method reference is present")
void hasRollbackTrueWhenMethodPresent() throws NoSuchMethodException {
Method applyMethod = Sample.class.getMethod("apply");
Method rollbackMethod = Sample.class.getMethod("rollback");

CodeExecutableChange<AbstractReflectionLoadedChange> change = new CodeExecutableChange<>(
"stage-1",
mock(AbstractReflectionLoadedChange.class),
ChangeAction.APPLY,
applyMethod,
rollbackMethod);

assertTrue(change.hasRollback(), "rollback method present → hasRollback() must be true");
}

@Test
@DisplayName("hasRollback() returns false when the rollback method reference is null")
void hasRollbackFalseWhenMethodAbsent() throws NoSuchMethodException {
Method applyMethod = Sample.class.getMethod("apply");

CodeExecutableChange<AbstractReflectionLoadedChange> change = new CodeExecutableChange<>(
"stage-1",
mock(AbstractReflectionLoadedChange.class),
ChangeAction.APPLY,
applyMethod,
null);

assertFalse(change.hasRollback(), "no rollback method → hasRollback() must be false");
}

@Test
@DisplayName("rollback() throws IllegalStateException (not NPE) when no rollback method is declared")
void rollbackGuardThrowsIllegalStateWhenNoRollbackMethod() throws NoSuchMethodException {
Method applyMethod = Sample.class.getMethod("apply");
AbstractReflectionLoadedChange loaded = mock(AbstractReflectionLoadedChange.class);
// The loaded change provides the change id used in the guard's error message.
org.mockito.Mockito.when(loaded.getId()).thenReturn("change-no-rollback");

CodeExecutableChange<AbstractReflectionLoadedChange> change = new CodeExecutableChange<>(
"stage-1",
loaded,
ChangeAction.APPLY,
applyMethod,
null);

IllegalStateException thrown = assertThrows(
IllegalStateException.class,
() -> change.rollback(mock(ExecutionRuntime.class)),
"rollback() with no @RollbackExecution method must throw IllegalStateException, not NPE");

assertTrue(thrown.getMessage().contains("change-no-rollback"),
"error message should name the offending change: " + thrown.getMessage());
assertTrue(thrown.getMessage().contains("hasRollback"),
"error message should point callers at hasRollback(): " + thrown.getMessage());
}

/** Sample type providing reachable apply/rollback methods. */
@SuppressWarnings("unused")
static class Sample {
public void apply() { /* no-op */ }
public void rollback() { /* no-op */ }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
* Copyright 2026 Flamingock (https://www.flamingock.io)
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.flamingock.internal.core.change.navigation.step.afteraudit;

import io.flamingock.internal.core.change.executable.ExecutableChange;
import io.flamingock.internal.util.Result;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import java.util.Optional;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

/**
* Verifies that {@code FailedAfterExecutionAuditStep.getRollbackStep()} returns an empty
* {@link Optional} when the failed change doesn't declare a rollback method. This is the contract
* that lets the navigator strategies skip the manual-rollback path (and its audit entry) instead
* of NPE'ing on a null reflection method.
*/
class FailedAfterExecutionAuditStepRollbackOptionalTest {

@Test
@DisplayName("Optional.empty() when the failed change has no rollback method")
void returnsEmptyWhenNoRollback() {
ExecutableChange change = mock(ExecutableChange.class);
when(change.hasRollback()).thenReturn(false);

FailedAfterExecutionAuditStep step = FailedAfterExecutionAuditStep.fromFailedApply(
change, new RuntimeException("apply failed"), Result.OK());

Optional<RollableStep> rollback = step.getRollbackStep();
assertFalse(rollback.isPresent(), "no rollback method → getRollbackStep() must be empty");
}

@Test
@DisplayName("Optional.of(step) when the failed change does declare a rollback method")
void returnsPresentWhenRollbackDeclared() {
ExecutableChange change = mock(ExecutableChange.class);
when(change.hasRollback()).thenReturn(true);

FailedAfterExecutionAuditStep step = FailedAfterExecutionAuditStep.fromFailedApply(
change, new RuntimeException("apply failed"), Result.OK());

Optional<RollableStep> rollback = step.getRollbackStep();
assertTrue(rollback.isPresent(), "rollback declared → getRollbackStep() must be present");
assertSame(change, rollback.get().getChange(),
"rollback step must wrap the failed change");
}

@Test
@DisplayName("Empty Optional regardless of which FailedAfterExecutionAuditStep variant is built")
void emptyAcrossVariants() {
ExecutableChange change = mock(ExecutableChange.class);
when(change.hasRollback()).thenReturn(false);

// Variant 1: failed apply + successful audit
FailedAfterExecutionAuditStep v1 = FailedAfterExecutionAuditStep.fromFailedApply(
change, new RuntimeException("apply failed"), Result.OK());
assertEquals(Optional.empty(), v1.getRollbackStep());

// Variant 2: failed apply + failed audit
FailedAfterExecutionAuditStep v2 = FailedAfterExecutionAuditStep.fromFailedApply(
change, new RuntimeException("apply failed"),
new Result.Error(new RuntimeException("audit failed")));
assertEquals(Optional.empty(), v2.getRollbackStep());

// Variant 3: successful apply + failed audit
FailedAfterExecutionAuditStep v3 = FailedAfterExecutionAuditStep.fromSuccessApply(
change, new Result.Error(new RuntimeException("audit failed")));
assertEquals(Optional.empty(), v3.getRollbackStep());
}
}
Loading
Loading