Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changeset with relative path deployed with liquibase 4.6.0 to 4.23.1 are replayed with liquibase > 4.23.1 #5724

Closed
2 tasks done
jgarec opened this issue Mar 22, 2024 · 4 comments · Fixed by #5727
Closed
2 tasks done
Assignees

Comments

@jgarec
Copy link
Contributor

jgarec commented Mar 22, 2024

Search first

  • I searched and no similar issues were found

Description

We have changeset with relativePath. They were deployed with liquibase 4.8.0 (but all versions between 4.6.0 and 4.23.0 are concerned).
When we tried to upgrade to liquibase 4.24.0 (same result with 4.25 & 4.26), these changesets were replayed.

Steps To Reproduce

I use docker for this :

# Start a postgres database
docker run -d --rm -it --name postgres -e POSTGRES_USER=myusername -e POSTGRES_PASSWORD=mypassword -p 5412:5432 postgres:12
# execute liquibase with any version from to 4.23.0
docker run --rm -it -v ${PWD}:/liquibase/changelog liquibase/liquibase:4.8.0 --changelogFile=changelog-master.yaml --classpath=/liquibase/changelog --url=jdbc:postgresql://host.docker.internal:5412/postgres\?user=myusername\&password=mypassword update
# check the databasechangelog table
postgres=# select id,filename from databasechangelog;
            id             |          filename
---------------------------+----------------------------
 model-create-table-person | model/changelog-table.yaml
(1 row)
# execute liquibase with any version from 4.23.1 to 4.26.0
docker run --rm -it -v ${PWD}:/liquibase/changelog liquibase/liquibase:4.26.0 --changelogFile=changelog-master.yaml --classpath=/liquibase/changelog --url=jdbc:postgresql://host.docker.internal:5412/postgres\?user=myusername\&password=mypassword update

...
Running Changeset: model/foo/../bar/../changelog-table.yaml::model-create-table-person::12345

UPDATE SUMMARY
Run:                          1
Previously run:               0
Filtered out:                 0
-------------------------------
Total change sets:            1

ERROR: Exception Details
ERROR: Exception Primary Class:  PSQLException
ERROR: Exception Primary Reason: ERROR: relation "tb_person" already exists
ERROR: Exception Primary Source: PostgreSQL 12.18 (Debian 12.18-1.pgdg120+2)

Expected/Desired Behavior

we expect :

UPDATE SUMMARY
Run:                          0
Previously run:               1
Filtered out:                 0
-------------------------------
Total change sets:            1

Liquibase Version

= 4.23.2

Database Vendor & Version

offline, postgresql

Liquibase Integration

cli, docker, build with source code

Liquibase Extensions

No response

OS and/or Infrastructure Type/Provider

linux

Additional Context

create changelog-master.yaml file

databaseChangeLog:
- include:
    file: model/foo/../bar/../changelog-table.yaml
    relativeToChangelogFile: true

create a model directory and add changelog-table.yaml file

databaseChangeLog:
- changeSet:
    id: model-create-table-person
    author: "12345"
    changes:
    - createTable:
        tableName: TB_PERSON
        remarks: Person Table.
        columns:
        - column:
            constraints:
              primaryKey: true
              primaryKeyName: PK_PERSON
            name: PERSON_ID
            type: BIGINT
            remarks: PK

Are you willing to submit a PR?

  • I'm willing to submit a PR (Thank you!)
@jgarec
Copy link
Contributor Author

jgarec commented Mar 22, 2024

With an empty database and liquibase 4.26.0, the databasechangelog is :

select id,filename from databasechangelog;
            id             |                 filename
---------------------------+------------------------------------------
 model-create-table-person | model/foo/../bar/../changelog-table.yaml
(1 row)

For me, this PR is the root cause : #5141

I can submit a PR with this change :
add this line at the end of the static method DatabaseChangeLog.normalizePath :

        filePath = Paths.get(filePath).normalize().toString().replace("\\", "/");

        return filePath;
    }

revert isSamePath in RunChangeSet like this :

private boolean isSamePath(String filePath) {
        String normalizedFilePath = DatabaseChangeLog.normalizePath(this.getChangeLog());
        return normalizedFilePath.equalsIgnoreCase(DatabaseChangeLog.normalizePath(filePath));
    }

@filipelautert does it make sense to you ?
I've made some tests and it's ok. All existing tests are ok except this one :

def "relative paths for changelog include are not resolved to their full path"() {
given:
def changelog = new DatabaseChangeLog("com/example/root1.xml")
def resourceAccessor = new MockResourceAccessor(["com/example/root1.xml" : test1Xml,
"com/example/../../path/changelog.xml": test2Xml])
when:
boolean result = changelog.include("../../path/changelog.xml", true, false,
resourceAccessor, null, new Labels(), false, DatabaseChangeLog.OnUnknownFileFormat.WARN)
then:
result
}

@filipelautert filipelautert self-assigned this Mar 23, 2024
@filipelautert
Copy link
Collaborator

Hello @jgarec ! The idea behind method isSamePath was to add all possible variations there to make sure that we are keeping compatibility with new and older versions. Do you think it would be possible to add another or in that method that matches your case?

@filipelautert
Copy link
Collaborator

Btw, there is another PR going around the same piece of code: #5682 , I believe it will be good to handle those 2 problems together or we may face conflicts.

@jgarec
Copy link
Contributor Author

jgarec commented Mar 24, 2024

thx for your feedback @filipelautert, I've check the other PR.
I've published a PR to illustrate how I think this issue have to be handled and we will see how to handle both issues

filipelautert added a commit that referenced this issue Apr 8, 2024
* fixes #5724: compare normalized file paths

* fix: windows path should be corrected after calling OS aware Paths method

* chore: copy test to validate issue 5681

* chore: we really need to replace it twice....

---------

Co-authored-by: filipe <flautert@liquibase.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants