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

Fix #2133 and preserve CORE-2971 (Liquibase endDelimiter does not work after upgrading from 3.10.x to 4.x) #2135

Merged

Conversation

Saucistophe
Copy link
Contributor

@Saucistophe Saucistophe commented Oct 1, 2021

Environment

Liquibase Version: Latest (4.5.1?)

Liquibase Integration & Version: core

Pull Request Type

  • Bug fix (non-breaking change which fixes an issue.)
  • Enhancement/New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Fixes #2133 while preserving (and testing) CORE-2971.
It's a tad less aggressive regarding end delimiters.

Steps To Reproduce

All steps are clearly listed in the Github issue.

Fast Track PR Acceptance Checklist:


Dev Handoff Notes (Internal Use)

Links

Testing

Dev Verification

Reviewed code and verified that automated tests now check the described problem

┆Issue is synchronized with this Jira Bug by Unito

@Saucistophe Saucistophe changed the title Fix #854 and preserve CORE-2971 Fix #2133 and preserve CORE-2971 Oct 1, 2021
@mkobylarz
Copy link

Hello, I would like to upvote for this merge request, because it solves breaking change for me. @Saucistophe do you think we could ask someone for review?

@Saucistophe
Copy link
Contributor Author

Hello, I would like to upvote for this merge request, because it solves breaking change for me. @Saucistophe do you think we could ask someone for review?

Sure, I'm just not sure who/how to call out?

@mkobylarz
Copy link

Hello, I would like to upvote for this merge request, because it solves breaking change for me. @Saucistophe do you think we could ask someone for review?

Sure, I'm just not sure who/how to call out?

Don't exactly know either, but maybe there is an option to assign a reviewer? For example look at the history of #2140. There is a log entry: wwillard7800 requested review from nvoxland and suryaaki2 2 days ago therefore I suppose there must be such button somewhere. Can you see anything like that? Users: @nvoxland and @suryaaki2 were recently active in approving merge request.

Nevertheless, I am very grateful for your help. This gives hope to solve this issue, thanks a lot.

@Saucistophe
Copy link
Contributor Author

Looks like it's for registered collaborators only !

@nvoxland or @suryaaki2, I'm not sure about how to raise attention on a PR, do you think you could check this one?
Thanks!

@molivasdat molivasdat added this to To Do in Conditioning++ via automation Oct 12, 2021
…com/Saucistophe/liquibase into Saucistophe-handle_slash_delimiter_and_comments

# Conflicts:
#	liquibase-core/src/test/groovy/liquibase/parser/core/formattedsql/FormattedSqlChangeLogParserTest.groovy
@nvoxland nvoxland changed the base branch from master to 1_9 December 9, 2021 19:53
@nvoxland nvoxland changed the base branch from 1_9 to master December 9, 2021 19:53
Copy link
Contributor

@nvoxland nvoxland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, thanks.

I pushed a merge from master to your fork to clean up the merge conflicts

@nvoxland nvoxland moved this from To Do to Code Review in Conditioning++ Dec 9, 2021
@suryaaki2 suryaaki2 self-requested a review December 13, 2021 21:10
@suryaaki2 suryaaki2 moved this from Code Review to Ready for Handoff (In JIRA) in Conditioning++ Dec 13, 2021
@sync-by-unito sync-by-unito bot changed the title Fix #2133 and preserve CORE-2971 Fix #2133 and preserve CORE-2971 (Liquibase endDelimiter does not work after upgrading from 3.10.x to 4.x) Dec 20, 2021
@kataggart
Copy link
Contributor

hey @nvoxland can you take a look at this one? I think it was all set to go but never actually got merged; I put it in Ready to Merge and assigned to you, if that isn't right could you adjust? Thanks!

@XDelphiGrl XDelphiGrl self-assigned this Mar 3, 2022
@XDelphiGrl
Copy link
Contributor

This PR addresses two issues. The first issue is that endDelimiter was not being respected during liquibase update. The second issue is to ensure the fix for endDelimiter does not break handling of trailing comments in SQL statements; i.e. /*comment*/.


  • Verify changesets with endDelimiter deploy without error.

    • formatted SQL changelog PASS
    • JSON changelog PASS
    • YAML changelog PASS
    • XML changelog Mostly PASS
      • The createProcedure changetype does not support the endDelimiter property. However, Liquibase still tries to split statements based on an end delimiter and does not do it correctly (truncates the last statement in the SQL). If you use an XML changelog, I suggest using either the sql or sqlFile changetypes to explicitly set an end delimeter and split statement properties.
  • Verify with a trailing comment deploy without error.

    • formatted SQL changelog PASS
    • JSON changelog PASS
    • YAML changelog PASS
    • XML changelog PASS

Test Files

Formatted SQL

--liquibase formatted sql

-- changeset pr2135:endDelim runOnChange:true endDelimiter:/
CREATE OR REPLACE PROCEDURE proschema.pr2135_proc is
BEGIN
    SELECT * FROM DATABASECHANGELOGLOCK;
END pr2135_proc;
/

grant execute on pr2135_proc to CUKE_ADMIN/
grant execute on pr2135_proc to CUKE_ADMIN/
grant execute on pr2135_proc to CUKE_ADMIN/
-- rollback drop PROCEDURE pr2135_proc/

--changeset pr2135:trailingcomment stripComments:false

CREATE VIEW proschema.v_test AS

SELECT 1 x FROM dual /*test view*/

JSON

{ "databaseChangeLog": [
  {
    "changeSet": {
      "id": "1646318526286-1",
      "author": "erz (generated)",
      "changes": [
        {
          "createView": {
            "fullDefinition": true,
            "selectQuery": "CREATE OR REPLACE FORCE VIEW \"V_TEST\" (\"X\") AS SELECT 1 x FROM dual /*test view*/",
            "viewName": "V_TEST"
          }
        }]
      
    }
  },
  
  {
    "changeSet": {
      "id": "1646318526286-2",
      "author": "erz (generated)",
      "changes": [
        {
          "createProcedure": {
            "path": "objects/storedprocedure/PR2135_PROC.sql",
            "procedureName": "PR2135_PROC",
            "relativeToChangelogFile": true,
            "endDelimiter": "/"
          }
        }]
      
    }
  }
  
]}

YML

databaseChangeLog:
- changeSet:
    id: 1646318590509-1
    author: erz (generated)
    changes:
    - createView:
        fullDefinition: true
        selectQuery: CREATE OR REPLACE FORCE VIEW "V_TEST" ("X") AS SELECT 1 x FROM
          dual /*test view*/
        viewName: V_TEST
- changeSet:
    id: 1646318590509-2
    author: erz (generated)
    changes:
    - createProcedure:
        path: objects/storedprocedure/PR2135_PROC.sql
        procedureName: PR2135_PROC
        relativeToChangelogFile: true
        endDelimiter: /

XML

<?xml version="1.1" encoding="UTF-8" standalone="no"?>
<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog" xmlns:ext="http://www.liquibase.org/xml/ns/dbchangelog-ext" xmlns:pro="http://www.liquibase.org/xml/ns/pro" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog-ext http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd http://www.liquibase.org/xml/ns/pro http://www.liquibase.org/xml/ns/pro/liquibase-pro-4.1.xsd http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.1.xsd">
    <changeSet author="erz (generated)" id="1646318372076-1">
        <createView fullDefinition="true" viewName="V_TEST">CREATE OR REPLACE FORCE VIEW "V_TEST" ("X") AS SELECT 1 x FROM dual /*test view*/</createView>
    </changeSet>
    <changeSet author="erz (generated)" id="1646318372076-2">
        <createProcedure path="objects/storedprocedure/PR2135_PROC.sql" procedureName="PR2135_PROC" relativeToChangelogFile="true"/>
    </changeSet>
</databaseChangeLog>

SQL File PR2135_PROC.sql

CREATE OR REPLACE PROCEDURE proschema.pr2135_proc is
BEGIN
    SELECT * FROM DATABASECHANGELOGLOCK;
END pr2135_proc;/

grant execute on pr2135_proc to CUKE_ADMIN/
grant execute on pr2135_proc to CUKE_ADMIN/
grant execute on pr2135_proc to CUKE_ADMIN/

Test Environment
Liquibase Core: handle_slash_delimiter_and_comments/957/b949f0, Pro: master/216/2fa6e0
Oracle 12.2.1
Passing Functional Tests

  • Note: The Oracle proxy tests did not run due to an environmental issue; I deem this acceptable due to the limited scope of those tests and their lack of relation to the current fix.

@nvoxland nvoxland merged commit e314058 into liquibase:master Mar 4, 2022
Conditioning++ automation moved this from Ready for Handoff (In JIRA) to Done Mar 4, 2022
@kataggart kataggart modified the milestones: On Deck, NEXT Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Liquibase endDelimiter does not work after upgrading from 3.10.x to 4.x
7 participants