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

Allow multi-line rollback statements in formatted sql changefile. #334

Open
wants to merge 6 commits into
base: master
from

Conversation

@pmendelson
Copy link

commented Dec 1, 2014

Currently, it is awkward to have a long block of rollback sql code in a formatted sql changelog file.

@weters

This comment has been minimized.

Copy link

commented Oct 2, 2015

It appears that liquibase does support multiline rollbacks, but you have to prefix each line with --rollback.

--changeset me:1

CREATE TABLE bar (
        id INTEGER PRIMARY KEY,
        name TEXT
);

--rollback DROP TABLE
--rollback bar;
@jcfandino

This comment has been minimized.

Copy link

commented Sep 5, 2016

@weters : where did you find that?
That isn't working for me and I found this jira: https://liquibase.jira.com/browse/CORE-1984
So it seems it isn't supported yet, can anyone confirm if this will be added in the future?

@weters

This comment has been minimized.

Copy link

commented Sep 6, 2016

@jcfandino - Liquibase supports multiline rollbacks, but it doesn't support disabling splitStatements for your rollback SQL.

This is supported:

--rollback DROP TABLE
--rollback foo;

This is not supported yet (because liquibase will split the statements on every semicolon):

--rollback CREATE OR REPLACE FUNCTION foo() RETURNS bool
--rollback LANGUAGE plpgsql
--rollback AS $$
--rollback BEGIN
--rollback   RETURN true;
--rollback END;
--rollback $$;
@jcfandino

This comment has been minimized.

Copy link

commented Sep 6, 2016

Thanks @weters for the response.
I was getting the same error as the jira bug when using the maven plugin, but I tested it again today and it's working as expected.
It must have been another problem then.
Thanks, this is much better now!

@nvoxland

This comment has been minimized.

Copy link
Member

commented Sep 8, 2016

Fun fact: I just added support splitting rollback statements for the upcoming 3.5.2 release. Just add rollbackSplitStatements:true|false and/or rollbackEndDelimiter:/ to your --changeset header

pmendelson and others added 4 commits Nov 2, 2016
modified:   liquibase-core/src/test/java/liquibase/parser/core/formattedsql/FormattedSqlChangeLogParserTest.java
@pmendelson

This comment has been minimized.

Copy link
Author

commented on 9fbdc57 Nov 2, 2016

Making ChangeWithColumns extend Change would fit well with all known implementations (they extend AbstractChange).

This would benefit someone who is trying to write helper classes that service extensions to CreateTableChange and AddColumnChange.

@hello-josh

This comment has been minimized.

Copy link

commented Apr 11, 2017

@nvoxland rollbackSplitStatements:false saved my life. I was in CREATE OR REPLACE FUNCTION hell just now.

@Saravanan21

This comment has been minimized.

Copy link

commented Jul 1, 2017

@pmendelson Do you still plan to support multiline rollback without having lots of "--rollback"s? Brief scan of your initial commit seems to resemble something like what I'd want, curious to know why you reverted those commits after.

@Gamesh

This comment has been minimized.

Copy link

commented Aug 8, 2017

placing rollback sql in comment while it's multi-line or not just is ugly and no syntax highlighting from the IDE, can't we just use --rollback to mark the start of the rollback and place the rollback sql(s) on the next line like a regular migrations?

-- rollback
DROP TABLE foo;
delete from bar where 1=1;
@Saravanan21

This comment has been minimized.

Copy link

commented Aug 8, 2017

I'm not sure why this PR has a commit that was then rolled back, but I believe I have a simpler solution to this: #695

@fellipecm

This comment has been minimized.

Copy link

commented Mar 7, 2018

Hi @nvoxland, apart from your comment, is there any documentation on multi-line rollback statements, preferably with an example? Is #695 even being considered? Thanks!

@holandaleo

This comment has been minimized.

Copy link

commented Feb 27, 2019

I believe the reason to leave rollback statements as part of a comment would be for cases when the sql formatted changefile repository is shared to an external team who has no idea what liquibase is. They won't run the .sql using liquibase, instead they are used to their green phosphor terminal where they will copy&paste the script without a clue on what it's doing. Believe me, there is still people like that, even working as DBAs. Better safe than sorry.

@Gamesh

This comment has been minimized.

Copy link

commented Mar 1, 2019

@holandaleo i would consider that an edge case, most don't have such a use case and we shouldn't build tools to solve 5% uses cases

@holandaleo

This comment has been minimized.

Copy link

commented Mar 1, 2019

I agree it would be 5% of the cases for liquibase users, but not 5% of the users who adopted the .sql format. I, personally, don't even like the .Sql format for liquibase. But, from what I've seen out there, is that teams who make the choice to adopt the .sql format, the most common reason for the choice is do that they can copy and paste fragments of the file to a sql terminal. In the other hand I'm still partially on your side that this is a bad practice and the tool design should discourage bad practices. However:

  1. Current behavior of the .sql format is to have rollback statements within comments. Future solutions should be backward compatible and still allow rollback blocks entirely in comments despite accepting statements below the comment line making the beginning of the block.
  2. Rollback statements are usually destructive, so, for .sql format, leaving them behind comments is a good safe-guard to ensure they are only seen buy the build tool.
  3. Leaving 5% of the cases out of the solution is not a good argument. Liquibase is a great tool and very popular so that 5% of a lot is still a lot. The goal should be robustness and achieve 100% cases covered.
@Saravanan21

This comment has been minimized.

Copy link

commented Mar 1, 2019

@holandaleo if you check out #695 I implemented this with backwards compatibility - you can use --rollback for single line, but --changesetrollback for multiline. So those who want .sql and copy and paste can use the existing --rollback system, and those who want .sql but multiline rollback can use --changesetrollback as well.

@jcrand

This comment has been minimized.

Copy link

commented May 17, 2019

@holandaleo @Saravanan21 Bear with me - I'm new to liquibase and I find both of your arguments have merit. Would it make sense to support multi-line rollbacks inside the standard delimiters for multi-line comments?

/* rollback
  DROP TABLE foo;
  delete from bar where 1=1;
*/
@Gamesh

This comment has been minimized.

Copy link

commented May 24, 2019

@jcrand it has some cons, for example it just means that for tools we write in, it will be harder to understand this syntax and provide us with feedback and check the syntax. Tools will have to add support for it. The comment delimiter works great -- rollback because it does not cause such issues the tools which don't support it will treat it as a comment while still providing autocompletion and syntax checking for the SQL that follows.

@holandaleo the 5% was just a pick from the top of my head, i don't think any of us have any numbers to support it, i just have a hunch that its a rare case. Haven't encountered any at least, so i'm not willing to bet on it.

  1. i agree partly, major versions can introduce breaking changes to move forward
  2. safeguard from what? that also means that tools like mentioned before, IDEs, editors for example will not provide us with assistance to write it in the first place. I hope we want better developer experience than that.
  3. Again fictional numbers, but it's a good practice to follow 80/20 rule (Pareto principle), to reduce bloat and we can't accommodate everyone's needs, so 100% is unachievable. The only real way to measure this would be to do a survey or add anonymous usage stats to liquibase

i personally would be for #695 seems like best of both worlds

@ghunteranderson

This comment has been minimized.

Copy link

commented Jun 19, 2019

My team was just talking about needing a feature like #695 and I suspect our use case isn't uncommon. We need to add rollback scripts for stored procedures, views, and functions that may span hundreds of lines. Adding -- rollback before each line is technically possible but seems unnecessary and really ruins the developer experience. I believe the syntax introduced by #695 would be a really good addition.

@hello-josh

This comment has been minimized.

Copy link

commented Jun 19, 2019

The problem with the style of rollback in #695 @ghunteranderson is that it breaks the ability to execute the SQL script directly without it executing the DDL/DML and ALSO the rollback. Something like /* */ style multi-line comments would be best and not break backwards compat with executing the SQL scripts directly

@ghunteranderson

This comment has been minimized.

Copy link

commented Jun 19, 2019

@TRII I agree we probably don't want to break that if people are using it. Two quick thoughts.

  1. Often times (with MySQL at least) we cannot execute the files directly on the server anyways because of limitations around how the DELIMITER syntax is handled. (Perhaps a separate issue that needs addressing)
  2. The --rollback syntax allows users to keep writing rollbacks as commented out sql but the new --changesetRollback would give users the option for cleaner syntax with the trade off of not being able to execute the file directly on the server any more. How do we feel about providing the option?

One last after thought. I'm not sure if I understand the use case of running change logs directly on the server especially with features like the updateSql command that is intended to generate the SQL that needs to be ran on the server. That being said, I also have a pretty narrow view of how people use Liquibase in the wild.

@hello-josh

This comment has been minimized.

Copy link

commented Jun 19, 2019

I'm not a maintainer of the project @ghunteranderson, just a user, but I execute the SQL files directly via psql during development on my locally running instance to make sure they are correct. I think the solution described by @jcrand #334 (comment) would be a nice compromise.

@ghunteranderson

This comment has been minimized.

Copy link

commented Jun 19, 2019

@TRII The solution here #334 (comment) isn't my first pick but I could see it being a good compromise. It offers simpler syntax (compared to prefixing every line of the rollback) and a smaller set of keywords to remember the difference between.

@ali88-gg

This comment has been minimized.

Copy link

commented Aug 28, 2019

has this great feature "allow mutli-line rollback statements" been released? currently I'm using liquibase version 3.3.2 which asks me to put all rollback statements in one line as comment. This is ridiculous!!!
We have lots of cases for working on stored procedures, views and triggers etc. all are multiple lines, some of them have more than 1000 lines, how can I put them in such one line comment?!
I want to continue using liquibase but looks like I have to give up because of this rollback issue.

Stephen

@ali88-gg

This comment has been minimized.

Copy link

commented Aug 28, 2019

@holandaleo if you check out #695 I implemented this with backwards compatibility - you can use --rollback for single line, but --changesetrollback for multiline. So those who want .sql and copy and paste can use the existing --rollback system, and those who want .sql but multiline rollback can use --changesetrollback as well.

Hi,
which version includes this multiline rollback via --changesetrollback ?. this looks awesome and will save my life.

Thanks
Stephen

@hsahay1970

This comment has been minimized.

Copy link

commented Oct 7, 2019

Hi
I am new to liquibase and we are using "sql formatted liquibase". I am trying to figure out a way to rollback stored procs.
I have got a multi line rollback working by prefixing each line of the old stored proc with the string "--rollback"
Example -

--liquibase formatted sql
--changeset authorname:ID_2 endDelimiter:/ rollbackEndDelimiter:/
Create or replace procedure test_proc is
.........new version of proc code
.........................
end;
/
--rollback Create or replace procedure test_proc is
--rollback .........old version of proc code
--rollback .........................
--rollback end;
--rollback /

But this requires developers to continually keep cutting and pasting old code and then manually adding the string --rollback to every line.

From the sparse documentation at the datical site, it seems i can use the changeSetId property to refer to an older changeset to rollback too ....That would make it really easy and readable. for example if the old change set ID is authorname:ID_1 I will have to write the new change set as below -

--liquibase formatted sql
--changeset authorname:ID_2 endDelimiter:/ rollbackEndDelimiter:/
Create or replace procedure test_proc is
.........new version of proc code
.........................
end;
/
--rollback changeSetId authorname:ID_2

Thing is - I don't know what the exact syntax is to specify a changeset to rollback to using sql formatted liquibase file. I could not find any example or any documentation. Is this even possible using sql formatted Liquibase ?

I know that in XML it is supposed to work as below -

...... ....
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.