Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 3 committers have signed the CLA.

✅ MalloD12
❌ Mahoney
❌ tati-qalified
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

To be closed when #6627 is merged.

Walkthrough

Adds two integration tests and changelog fixtures for missing custom-change classes, implements lazy loading and safer error handling in CustomChangeWrapper, and skips a network test on macOS.

Changes

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Runner as Test/Runtime
participant Wrapper as CustomChangeWrapper
participant Loader as ClassLoader
participant DB as Database
Note over Wrapper,Loader: New lazy-loading path for custom changes
Runner->>Wrapper: request getCustomChange()/validate/generateCheckSum
alt custom change not yet loaded
Wrapper->>Loader: loadCustomChange(className)
Loader-->>Wrapper: instance or ClassNotFound
alt loaded
Wrapper-->>Runner: return CustomChange instance
Runner->>DB: execute change (may write DATABASECHANGELOG)
else load failed
Wrapper-->>Runner: log warning / return fallback behavior
Runner->>DB: proceed (failOnError=false or precondition MARK_RAN may avoid execution)
end
else already loaded
Wrapper-->>Runner: return existing instance
end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Review focus areas:
  • CustomChangeWrapper.java — lazy-loading behavior, exception handling, and logging semantics across validate/checksum/configure paths.
  • New tests in AbstractIntegrationTest.java — ensure database verification logic and SQLite guard are correct.
  • Changelog XMLs — correct attributes (failOnError, preconditions) and parameters.

Poem

🐇 I hopped through changes, light and spry,
Lazy loads now peek before they try.
Tests keep watch where classes are shy,
Marked ran or skipped — no loudly cried cry.
A tiny rabbit pats the log — goodbye! ✨

Pre-merge checks and finishing touches

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Pull Request Review: Allow skipping a missing custom change

Summary

This PR implements lazy loading for custom change classes to allow graceful handling when a custom change class is missing. The changes enable scenarios where:

1. A changeset with failOnError="false" can skip a missing custom change
2. A precondition can skip a changeset with a missing custom change

Code Quality & Best Practices

✅ Strengths

1. Lazy Loading Pattern: The refactoring to use lazy loading (loadCustomChange()) is well-structured and extracted into a private method for reuse
2. Graceful Degradation: The code properly handles missing classes by logging warnings instead of throwing exceptions in non-critical paths
3. Test Coverage: Good integration tests that verify both scenarios (failOnError and precondition skipping)
4. Clear Intent: The code changes are focused and address a specific use case

⚠️ Areas for Improvement

1. Method Signature Change - Breaking API (CustomChangeWrapper.java:60)

public CustomChange getCustomChange() throws CustomChangeException

The getCustomChange() method now throws CustomChangeException, which is a breaking change to the public API. Any code calling this method will need to handle the new checked exception.

Impact: This could break external code or plugins that use this method.

Recommendation: Consider one of:

• Document this as a breaking change in release notes
• Add @deprecated to the old signature and create a new method
• Evaluate if you can catch and wrap the exception internally

2. Inconsistent Error Handling (CustomChangeWrapper.java:136-156)

In the validate() method:

• If loading fails, it returns a warning (line 141)
• If configuration fails, it throws UnexpectedLiquibaseException (line 149)

This inconsistency could be confusing. The validate method should either:

• Return validation errors for all failures, OR
• Throw exceptions for all failures

Recommendation: Consider returning ValidationErrors for configuration failures too, for consistency.

3. Null Safety in validate() (CustomChangeWrapper.java:152)

return customChange.validate(database);

After catching the loading exception at line 140, customChange might still be null if loading fails. Line 152 will throw NPE.

Fix Required: Add null check:

if (customChange == null) {
    return new ValidationErrors().addWarning("Custom change class could not be loaded");
}
return customChange.validate(database);

4. Resource Leak in Test (AbstractIntegrationTest.java:1418-1429)

ResultSet rs = stmt.executeQuery(...);
assertTrue(..., rs.next());
rs.close();
stmt.close();

If assertTrue() fails, the ResultSet and Statement won't be closed.

Fix Required: Use try-with-resources:

try (Statement stmt = conn.createStatement();
     ResultSet rs = stmt.executeQuery(...)) {
    assertTrue("Expected changeset...", rs.next());
}

5. Platform-Specific Test Modification (NetUtilTest.groovy:8)

@IgnoreIf({ OperatingSystem.current.macOs })
def getLocalHostAddress() {

This change appears unrelated to the PR's purpose and lacks explanation.

Questions:

• Why is this test being skipped on macOS?
• Is there a flaky test issue on macOS?
• Should this be in a separate PR/issue?

Recommendation: Either remove this from the PR or add a comment explaining why it's needed.

Potential Bugs & Edge Cases

🐛 Critical Issue: Null Pointer Exception

Location: CustomChangeWrapper.java:152

As mentioned above, customChange can be null after the catch block at line 140, leading to NPE at line 152.

⚠️ Medium Priority Issues

1. Double Loading Attempt: In validate(), if loadCustomChange() fails at line 139, then configureCustomChange() at line 147 will try to load again at line 286. This is inefficient but not incorrect.

2. Inconsistent State: The configured flag doesn't account for failed loads. If loading fails, configured remains false, which could lead to repeated load attempts.

3. Error Message Context: When logging warnings about missing classes (lines 237, 376), consider including more context like the changeset ID or changelog file to help users identify which changelog has the issue.

Performance Considerations

✅ Positive Impact

• Lazy Loading: Loading custom change classes only when needed is a performance improvement
• Caching: Once loaded, the customChange instance is reused

⚠️ Minor Concern

• Multiple Load Attempts: If a class fails to load, the code may attempt to load it multiple times in different code paths (validate, generateCheckSum, etc.). Consider adding a flag to track load failures to avoid repeated class loading attempts.

Security Concerns

✅ No Major Issues

• Class loading uses existing security mechanisms
• No new attack vectors introduced
• Proper exception handling prevents information leakage in most cases

💡 Suggestion

Consider rate-limiting or circuit-breaking repeated failed class loads to prevent potential DoS if someone provides many changelogs with missing classes.

Test Coverage

✅ Good Coverage

1. Integration Tests: Two comprehensive integration tests covering the main scenarios
2. Database Verification: Tests verify both execution success and database state
3. Clear Test Names: Test names clearly describe what they're testing

⚠️ Missing Coverage

1. Unit Tests: No unit tests added to CustomChangeWrapperTest.groovy for:

   • The new lazy loading behavior
   • Failed class loading scenarios
   • Multiple load attempts
   • Null handling in validate()

2. Edge Cases Not Tested:

   • What happens with generateCheckSum() when class loading fails?
   • What happens with generateStatements() when class is missing?
   • What happens with supportsRollback() when customChange is null?
   • Behavior when failOnError is true (should still fail?)

Recommendation: Add unit tests in CustomChangeWrapperTest.groovy:

def "validate handles missing custom change class gracefully"() {
    when:
    def changeWrapper = new CustomChangeWrapper()
    changeWrapper.setClass("com.example.NonExistentClass")
    def errors = changeWrapper.validate(new MockDatabase())
    
    then:
    errors.hasWarnings()
    errors.getWarningMessages()[0].contains("NonExistentClass")
}

Summary of Required Fixes

Must Fix Before Merge

1. ✅ Fix NPE in validate() - Add null check at line 152
2. ✅ Fix resource leak - Use try-with-resources in test at line 1418

Should Address

3. Document the breaking API change in getCustomChange()
4. Remove or explain the NetUtilTest.groovy change
5. Make error handling consistent in validate()
6. Add unit tests for new lazy loading behavior

Nice to Have

7. Add context (changeset ID) to error messages
8. Optimize to avoid repeated failed load attempts
9. Add more edge case testing

Overall Assessment

This is a valuable feature that addresses a real use case. The core implementation is sound, but there are some critical issues that need to be addressed before merging:

Rating: ⚠️ Needs Revisions

The lazy loading pattern is well-implemented, but the NPE risk and resource leak are blockers. Once these are fixed and test coverage is expanded, this will be ready to merge.

Review generated with Claude Code

Pull Request Review

Summary

This PR successfully implements lazy loading for custom change classes, allowing changesets with missing custom change classes to be skipped when failOnError="false" or when preconditions with onFail="MARK_RAN" evaluate to skip the changeset. This addresses issue #6520 and improves the robustness of changelog execution.

Code Quality and Architecture ✅

Strengths:

• Excellent architectural improvement: Moving from eager to lazy loading in CustomChangeWrapper is the right solution. The refactoring to extract loadCustomChange() as a separate method (line 82-101) improves code reusability and maintainability.
• Consistent error handling: The PR properly handles exceptions at multiple entry points (getCustomChange(), validate(), generateCheckSum(), customLoadLogic()) with appropriate logging.
• Good use of logging: Using Logger for warnings when classes can't be loaded (lines 243-244, 382-383) provides visibility without breaking execution.

Minor Concerns:

1. Potential null pointer issues: In getConfirmationMessage() (line 277-285), if configureCustomChange() succeeds but customChange is still null, line 284 will throw a NPE. Consider adding a null check.
2. Inconsistent exception handling in validate(): Lines 141-147 catch CustomChangeException and return a warning, but then lines 150-154 catch it again and return an error. This might be intentional but could be confusing.

Potential Bugs and Issues ⚠️

1. Critical: Null dereference in getConfirmationMessage() (line 277-285)

   • If customChange fails to load, configureCustomChange() will succeed (because it loads it), but there's a window where NPE could occur.
   • Recommendation: Add null check after configureCustomChange()

2. Inconsistent behavior in generateStatements() and generateRollbackStatements()

   • These methods (lines 181-203, 213-235) call configureCustomChange() which can throw CustomChangeException, wrapped in UnexpectedLiquibaseException.
   • However, they don't handle the case where customChange might be null after calling configureCustomChange().
   • Recommendation: Add null checks or let validation errors propagate more gracefully.

3. Thread safety consideration

   • The configured flag and lazy initialization pattern could have race conditions in multi-threaded scenarios.
   • While Liquibase may not commonly execute changes concurrently, consider if thread safety is a concern.

Performance Considerations ✅

Positive impacts:

• Lazy loading reduces upfront classpath scanning overhead during changelog parsing
• Failed class loading is logged only once per operation (good caching behavior)
• No performance regressions expected

Security Concerns ✅

No security issues identified. The PR:

• Doesn't introduce new user input handling
• Maintains existing class loading security boundaries
• Properly logs exceptions without exposing sensitive information

Test Coverage 👍

Excellent test coverage:

• Two new integration tests cover the main use cases:
  • makeSureNoErrorIsReturnedWhenNonexistentCustomChangeIsRunWithFailOnErrorFalse() (lines 1374-1405)
  • makeSureNoErrorIsReturnedWhenNonexistentCustomChangeIsSkippedByPrecondition() (lines 1407-1444)
• Tests properly verify both successful execution AND database state (DATABASECHANGELOG entries)
• Good use of Given-When-Then comments for clarity
• Appropriate test changelogs created

Minor test improvement suggestions:

1. SQL injection in test query (line 1434-1437): While this is test code, consider using prepared statements as a best practice example.
2. Resource cleanup: The test doesn't close resources in a finally block. Consider using try-with-resources for ResultSet and Statement.
3. NetUtilTest.groovy change (line +8): The @IgnoreIf for macOS seems unrelated to this PR. If it's fixing a flaky test, it should be documented or separated into its own commit.

Specific Code Comments

CustomChangeWrapper.java (line 60-68)

public CustomChange getCustomChange() {
    if (this.customChange == null) {
        try {
            this.customChange = loadCustomChange(className);
        } catch (CustomChangeException e) {
            throw new UnexpectedLiquibaseException(e);
        }
    }
    return customChange;
}

Issue: This throws an exception if the class can't be loaded, which defeats the purpose of lazy loading. Callers expecting this to be safe may fail unexpectedly.
Recommendation: Consider returning null and letting callers handle it, or document that this method can throw exceptions.

CustomChangeWrapper.java (line 139-163) - validate() method

Observation: The logic flow is:

1. Try to load (returns warning on failure)
2. Try to configure (returns error on failure)
3. Check if null (returns warning)
4. Call validate (returns error on failure)

This creates a hierarchy of error severity which seems correct, but the distinction between "loading" warnings and "configuring" errors could be clearer in the validation messages.

Integration tests (line 1392-1404, 1425-1443)

Observation: Both tests have identical SQLite-skipping logic and commit handling. Consider extracting this into a helper method to reduce duplication.

Recommendations

Must fix:

1. Add null check in getConfirmationMessage() before calling customChange.getConfirmationMessage()

Should fix:
2. Add null checks in generateStatements() and generateRollbackStatements() after configureCustomChange()
3. Document the @IgnoreIf change in NetUtilTest or move to separate PR
4. Use try-with-resources in integration tests for proper resource cleanup

Nice to have:
5. Extract duplicate test setup/teardown logic into helper methods
6. Consider adding a unit test specifically for loadCustomChange() method
7. Add JavaDoc to loadCustomChange() method explaining its purpose and exception behavior

Conclusion

This is a well-designed PR that solves a real user pain point. The architectural change from eager to lazy loading is sound, and the test coverage demonstrates the fix works as intended.

The main concerns are around null safety in a few methods that could cause unexpected failures in edge cases. With the recommended null checks added, this PR will be production-ready.

Approval recommendation: Approve with minor changes requested (null safety fixes)