include(All), property file/path attribute starting with '.' sets relativeToChangelogFile` true by default #7410
Conversation
…uired for 'includeAll' instead of: Unexpected error running Liquibase: Error parsing inc.yml : null - missing `file`, `value` or `name` resulted in NullPointerException: Cannot invoke "String.equalsIgnoreCase(String)" because the return value of "liquibase.changelog.ChangeLogParameters$ChangeLogParameter.getKey()" is null new : Either 'file' or 'name' + 'value' must be set for 'property' - empty `file` attribute for `include` results a nice error message `'file' is empty for 'include'` instead of a cryptic error listing all files on the search path - starting `include@file`, `includeAll@path`, `property@file` with `.` sets `relativeToChangelogFile` true by default - Warning if 'file' is set for 'property', then 'value' and 'name' are ignored - Add ' character around both the field and the tag in validation error messages like `'file' is empty for 'include'`
|
|
WalkthroughDatabaseChangeLog refactored to use Lombok for generated setters on multiple fields. New helper methods introduced for validation and relative path resolution. ValidationErrors updated with Lombok Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
liquibase-standard/src/test/groovy/liquibase/exception/ValidationErrorsTests.groovy (1)
6-8: Remove duplicate import.Line 7 and line 8 both import
assertTrue, creating a duplicate import statement.Apply this diff to remove the duplicate:
import static org.junit.Assert.assertFalse import static org.junit.Assert.assertTrue -import static org.junit.Assert.assertTrue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
liquibase-standard/src/main/java/liquibase/changelog/DatabaseChangeLog.java(16 hunks)liquibase-standard/src/main/java/liquibase/exception/ValidationErrors.java(4 hunks)liquibase-standard/src/main/java/liquibase/sdk/resource/MockResourceAccessor.java(2 hunks)liquibase-standard/src/test/groovy/liquibase/change/AbstractChangeTest.groovy(1 hunks)liquibase-standard/src/test/groovy/liquibase/change/core/LoadDataChangeTest.groovy(1 hunks)liquibase-standard/src/test/groovy/liquibase/changelog/DatabaseChangeLogTest.groovy(3 hunks)liquibase-standard/src/test/groovy/liquibase/exception/ValidationErrorsTests.groovy(4 hunks)liquibase-standard/src/test/java/liquibase/exception/ValidatorErrorsTest.java(0 hunks)
💤 Files with no reviewable changes (1)
- liquibase-standard/src/test/java/liquibase/exception/ValidatorErrorsTest.java
🔇 Additional comments (17)
liquibase-standard/src/test/groovy/liquibase/change/AbstractChangeTest.groovy (1)
256-256: LGTM!The test correctly reflects the new error message format with quoted field and change names, aligning with the ValidationErrors updates.
liquibase-standard/src/test/groovy/liquibase/change/core/LoadDataChangeTest.groovy (1)
932-932: LGTM!The test correctly reflects the new error message format with quoted identifiers.
liquibase-standard/src/test/groovy/liquibase/changelog/DatabaseChangeLogTest.groovy (5)
643-655: LGTM!The test validates that properties can be loaded from a file specified with a relative path starting with './', correctly verifying the new default behavior for relative path resolution.
1151-1161: LGTM!The test confirms that include files with paths starting with './' are correctly resolved relative to the changelog.
1162-1171: LGTM!The test confirms that includeAll paths starting with './' are correctly resolved relative to the changelog.
1173-1185: LGTM!The test validates that properties can be loaded from a relative properties file, confirming the feature works end-to-end.
1187-1220: LGTM!Comprehensive error case testing for property definitions and include/includeAll tags. The tests validate:
- Missing required fields
- Empty field values
- Clear, quoted error messages
liquibase-standard/src/test/groovy/liquibase/exception/ValidationErrorsTests.groovy (2)
22-27: LGTM!The tests correctly reflect the new error message format with quoted field and change names.
Also applies to: 41-45
58-77: LGTM!The new tests add valuable coverage for
checkRequiredFieldandhasErrorsmethods, validating the expected behavior.liquibase-standard/src/main/java/liquibase/exception/ValidationErrors.java (2)
19-22: LGTM!Lombok @Getter annotations appropriately replace explicit getter methods, reducing boilerplate while maintaining the same public API.
76-90: LGTM!The error message updates consistently quote field names and change names, improving readability and clarity for end users.
liquibase-standard/src/main/java/liquibase/changelog/DatabaseChangeLog.java (6)
167-170: LGTM!The new constructor provides a convenient way to initialize both the physical file path and change log parameters together, maintaining backward compatibility with the existing single-argument constructor.
489-497: LGTM!The new helper methods improve code clarity:
throwIfcentralizes validation error handlinggetRelativeToChangelogimplements the PR's core feature: defaulting torelativeToChangelogFile=truewhen the path starts with '.'
581-613: LGTM!The refactored
handleIncludeAllmethod:
- Uses
throwIffor validation (line 584)- Applies
getRelativeToChangelogfor relative path resolution (line 602)- Uses
determineContextExpressionfor consistent context handling (line 606)The changes align with the PR objectives and improve code consistency.
616-638: LGTM!The refactored
handleIncludemethod:
- Uses
throwIffor validation (line 619)- Applies
getRelativeToChangelogfor relative path resolution (line 626)- Maintains backward compatibility while adding the new default behavior
640-646: LGTM!The
determineContextExpressionhelper method centralizes context expression resolution logic, checkingcontextFilterfirst and falling back tocontext. This improves code maintainability.
499-564: No issues found—the warning logic is correctly guarded.The condition at line 521 (
if(null != value || null != name)) correctly ensures the warning logs only when at least one ofnameorvalueis provided. When onlyfileis specified (bothnameandvalueare null), the warning is properly suppressed. The property handling for this edge case is already correct as implemented.
| public List<Resource> getAll(String path) throws IOException { | ||
| path = path.replace("\\", "/"); | ||
| path = normalizePath(path); | ||
| String content = contentByFileName.get(path); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify consistent path normalization across methods.
The getAll method now uses DatabaseChangeLog.normalizePath, but the search method (line 45) still uses the old path.replace("\\", "/") approach. This inconsistency could lead to different path resolution behavior between these methods.
🏁 Script executed:
#!/bin/bash
# Check if search method in MockResourceAccessor should also use normalizePath
rg -n "def search|public.*search" liquibase-standard/src/main/java/liquibase/sdk/resource/MockResourceAccessor.java -A 10Length of output: 561
🏁 Script executed:
#!/bin/bash
# Get complete MockResourceAccessor.java to see all path handling
cat liquibase-standard/src/main/java/liquibase/sdk/resource/MockResourceAccessor.javaLength of output: 1927
🏁 Script executed:
#!/bin/bash
# Check what normalizePath does in DatabaseChangeLog
rg -n "public.*static.*normalizePath|private.*static.*normalizePath" -A 5 -B 2Length of output: 1798
🏁 Script executed:
#!/bin/bash
# Check if there are other path manipulations in MockResourceAccessor
rg -n "replace.*\\\\\\\\" liquibase-standard/src/main/java/liquibase/sdk/resource/MockResourceAccessor.javaLength of output: 42
🏁 Script executed:
#!/bin/bash
# Check what imports are at the top of MockResourceAccessor
head -30 liquibase-standard/src/main/java/liquibase/sdk/resource/MockResourceAccessor.javaLength of output: 775
Replace path normalization in search() method to use normalizePath().
The inconsistency is confirmed. The search() method at line 45 still uses path.replace("\\", "/") while getAll() uses normalizePath(). Since normalizePath() handles null values and protocol prefixes (like classpath protocol), the simple string replacement is insufficient. This causes search() to potentially fail to match paths that are stored with protocol handling. Change line 45 to use normalizePath(path) for consistency with the rest of the class.
🤖 Prompt for AI Agents
In
liquibase-standard/src/main/java/liquibase/sdk/resource/MockResourceAccessor.java
around lines 29 to 31 (and specifically the search() call at line 45), the
search() method currently normalizes paths with path.replace("\\", "/") which is
inconsistent with getAll() that uses normalizePath(); update search() to call
normalizePath(path) instead so nulls and protocol prefixes (e.g., classpath:)
are handled consistently; ensure you replace the simple replace call with
normalizePath(path) and use the returned normalized value for matching.
3 participants
Impact
Description
Starting
include@file,includeAll@path,property@filewith.setsrelativeToChangelogFiletrue by defaultImproved error handling for theses attributes:
missing
file,valueornamefor 'property' resulted inNullPointerException: Cannot invoke "String.equalsIgnoreCase(String)" because the return value of "liquibase.changelog.ChangeLogParameters$ChangeLogParameter.getKey()" is null.new error message:
Either 'file' or 'name' + 'value' must be set for 'property'Warning if 'file' is set for 'property', then 'value' and 'name' are ignored
nameattribute forpropertycreated a property with no name silently, which of course could not be used. Now results error message:'value' is required for 'property' when 'name' is setfileattribute forincludeandpropertyresults a nice error message'file' is empty for 'include'instead of a cryptic error listing all files on the search path'file' is empty for 'include'Things to be aware of
The same
ValidationErrorsis used inDatabaseChangeLogas everywhere else.Things to worry about
Additional Context
Summary by CodeRabbit
Bug Fixes
Chores