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

migration succeeds even if compiling procedures failed #46

Closed
jmodi1982 opened this Issue May 6, 2016 · 22 comments

Comments

Projects
None yet
3 participants
@jmodi1982

jmodi1982 commented May 6, 2016

hi,

we are using MyBatis for applying DB changes and in 1 of the script there was a procedure which we were creating. MyBatis removed the comments while performing migraitons and that lead to procedure not compiling successfully. However Mybatis migraiton did not fail the execution but it provided Success status.

When we went to Database to check the procedure, it was broken.

Please can you confirm if MyBatis migrations can be used to Create/Replace Procedures,Functions, Packages in Oracle database.

Thanks

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 6, 2016

Member

Please provide sample scripts to reproduce the problem.

Member

harawata commented May 6, 2016

Please provide sample scripts to reproduce the problem.

@jmodi1982

This comment has been minimized.

Show comment
Hide comment
@jmodi1982

jmodi1982 May 6, 2016

All the comments from the below script gets removed, I don't want the comments inside stored procedure to be removed. Please find below a sample script -

--*****************************************************************************
--Request                       : 1548
--Date                          : 26/04/2016
--Onsite Anchor                 : John March
--******************************************************************************/

CREATE OR REPLACE PROCEDURE CheckPassword
 --**********************************************************************
 -- TYPE               : Procedure
 -- Orginal Procedure  : inet_pwrup_qgpn_price
 -- NAME               : DP_inet_pwrup_qgpn_price
 -- INPUT Parameters   : None
 -- PURPOSE            :This procedure is used to populate the quasi generic
                         part price and other details
 -- ------ ---------- ---   ---------------------------------------------
    @username VARCHAR(20),
    @password varchar(20)
AS
BEGIN

SET NOCOUNT ON

IF EXISTS(SELECT * FROM usertable WHERE username = @username AND password = @password)
    SELECT 'true' AS UserExists
ELSE
    SELECT 'false' AS UserExists

END

jmodi1982 commented May 6, 2016

All the comments from the below script gets removed, I don't want the comments inside stored procedure to be removed. Please find below a sample script -

--*****************************************************************************
--Request                       : 1548
--Date                          : 26/04/2016
--Onsite Anchor                 : John March
--******************************************************************************/

CREATE OR REPLACE PROCEDURE CheckPassword
 --**********************************************************************
 -- TYPE               : Procedure
 -- Orginal Procedure  : inet_pwrup_qgpn_price
 -- NAME               : DP_inet_pwrup_qgpn_price
 -- INPUT Parameters   : None
 -- PURPOSE            :This procedure is used to populate the quasi generic
                         part price and other details
 -- ------ ---------- ---   ---------------------------------------------
    @username VARCHAR(20),
    @password varchar(20)
AS
BEGIN

SET NOCOUNT ON

IF EXISTS(SELECT * FROM usertable WHERE username = @username AND password = @password)
    SELECT 'true' AS UserExists
ELSE
    SELECT 'false' AS UserExists

END
@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 7, 2016

Member

I couldn't create that procedure even in SQL*Plus.
The below script worked on Oracle 11g (ojdbc7 driver version 12.1.0.2).

-- // First migration.
-- Migration SQL that makes the change goes here.

-- @DELIMITER |
CREATE OR REPLACE PROCEDURE myproc IS
/*
Comments
Comments
Comments
*/
BEGIN
    INSERT INTO person (id, name1) VALUES (999, 'John');
END;
|
-- @DELIMITER ;

-- See if the procedure has an error.
SELECT * FROM SYS.USER_ERRORS WHERE name='MYPROC' AND type='PROCEDURE';

-- //@UNDO
-- SQL to undo the change goes here.

DROP PROCEDURE myproc;

Some comments:

  • Even when the procedure has compile error, Oracle driver creates the procedure and throws no exception. So, to Migrations, it's still SUCCESS.
    In the above script, I added a SELECT statement to check the error. It might be better than nothing.
  • To leave comments in procedure, you have to use multi-line format /* .... */.
  • The @DELIMITER syntax requires MyBatis 3.3.0 or later.

Here is the environment properties:

time_zone=GMT+0:00

driver=oracle.jdbc.driver.OracleDriver
url=jdbc:oracle:thin:@192.168.57.101:1521:ORCL
username=xxx
password=xxx

send_full_script=false
delimiter=;
full_line_delimiter=false
auto_commit=false
changelog=CHANGELOG

I close this issue because creating procedure works :-)

Member

harawata commented May 7, 2016

I couldn't create that procedure even in SQL*Plus.
The below script worked on Oracle 11g (ojdbc7 driver version 12.1.0.2).

-- // First migration.
-- Migration SQL that makes the change goes here.

-- @DELIMITER |
CREATE OR REPLACE PROCEDURE myproc IS
/*
Comments
Comments
Comments
*/
BEGIN
    INSERT INTO person (id, name1) VALUES (999, 'John');
END;
|
-- @DELIMITER ;

-- See if the procedure has an error.
SELECT * FROM SYS.USER_ERRORS WHERE name='MYPROC' AND type='PROCEDURE';

-- //@UNDO
-- SQL to undo the change goes here.

DROP PROCEDURE myproc;

Some comments:

  • Even when the procedure has compile error, Oracle driver creates the procedure and throws no exception. So, to Migrations, it's still SUCCESS.
    In the above script, I added a SELECT statement to check the error. It might be better than nothing.
  • To leave comments in procedure, you have to use multi-line format /* .... */.
  • The @DELIMITER syntax requires MyBatis 3.3.0 or later.

Here is the environment properties:

time_zone=GMT+0:00

driver=oracle.jdbc.driver.OracleDriver
url=jdbc:oracle:thin:@192.168.57.101:1521:ORCL
username=xxx
password=xxx

send_full_script=false
delimiter=;
full_line_delimiter=false
auto_commit=false
changelog=CHANGELOG

I close this issue because creating procedure works :-)

@harawata harawata closed this May 7, 2016

@jmodi1982

This comment has been minimized.

Show comment
Hide comment
@jmodi1982

jmodi1982 May 9, 2016

Thanks for the info, we also changed source code to use ## instead of -- as script comments as SQL. Everything looks good for now, will keep you posted if i come across any new issues

jmodi1982 commented May 9, 2016

Thanks for the info, we also changed source code to use ## instead of -- as script comments as SQL. Everything looks good for now, will keep you posted if i come across any new issues

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 10, 2016

Member

Good to know that ## works, too.
Thanks for the update!

Member

harawata commented May 10, 2016

Good to know that ## works, too.
Thanks for the update!

@jmodi1982

This comment has been minimized.

Show comment
Hide comment
@jmodi1982

jmodi1982 May 11, 2016

Hi,

I see a big issue and I need some immediate remediation based on your experience.

Even when the procedure has compile error, Oracle driver creates the procedure and throws no exception. So, to Migrations, it's still SUCCESS. Due to this the changelog table is updated and I cannot run the script again. So I have to create another script with correct procedure code and then again rerun it. Is there a way, using user_errors table we can fail the script so that the Changelog table is not updated with success. The developer can then go back correct the procedure code and rerun the same script file via mybatis. I agree the database however would be having errored stored procedure, but I want to avoid creating new files for every error in my pl/sql codes.

Looking forward for your guidance on this issue.

Thanks

jmodi1982 commented May 11, 2016

Hi,

I see a big issue and I need some immediate remediation based on your experience.

Even when the procedure has compile error, Oracle driver creates the procedure and throws no exception. So, to Migrations, it's still SUCCESS. Due to this the changelog table is updated and I cannot run the script again. So I have to create another script with correct procedure code and then again rerun it. Is there a way, using user_errors table we can fail the script so that the Changelog table is not updated with success. The developer can then go back correct the procedure code and rerun the same script file via mybatis. I agree the database however would be having errored stored procedure, but I want to avoid creating new files for every error in my pl/sql codes.

Looking forward for your guidance on this issue.

Thanks

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 11, 2016

Member

Hi,

After the incorrectly succeeded 'up' command, you just need to execute the 'down' command.
Of course, you need to write DROP PROCEDURE in the 'UNDO' part (see my sample script above).
The 'down' command will remove the row from the changelog table as well.

I had expected that Oracle has some kind of configuration option for this behavior, but couldn't find it.
There seems to be some trick to achieve this in SQL*Plus, but it wouldn't work with JDBC (even if it did, I wouldn't recommend it).

Hope this helps,
Iwao

Member

harawata commented May 11, 2016

Hi,

After the incorrectly succeeded 'up' command, you just need to execute the 'down' command.
Of course, you need to write DROP PROCEDURE in the 'UNDO' part (see my sample script above).
The 'down' command will remove the row from the changelog table as well.

I had expected that Oracle has some kind of configuration option for this behavior, but couldn't find it.
There seems to be some trick to achieve this in SQL*Plus, but it wouldn't work with JDBC (even if it did, I wouldn't recommend it).

Hope this helps,
Iwao

@jmodi1982

This comment has been minimized.

Show comment
Hide comment
@jmodi1982

jmodi1982 May 11, 2016

Hi,

Please do let us know if you find any oracle configuration which can solve this issue. At least we should get the compilation error and the script should be exited since we usually run a large no of scripts and a non-compiled procedure could cause serious dB inconsistency

Also DBAY don't like the idea of going to sql+ to see if the stored procedure has thrown any compilation errors. They want to see it automatically after procedure is updated and exit the script if there is ano error.

Thanks.

jmodi1982 commented May 11, 2016

Hi,

Please do let us know if you find any oracle configuration which can solve this issue. At least we should get the compilation error and the script should be exited since we usually run a large no of scripts and a non-compiled procedure could cause serious dB inconsistency

Also DBAY don't like the idea of going to sql+ to see if the stored procedure has thrown any compilation errors. They want to see it automatically after procedure is updated and exit the script if there is ano error.

Thanks.

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 11, 2016

Member

I totally understand what you and your team want. Strange implementation indeed 😕

Anyway, I think I have found a solution to this problem.
I will open a new issue and let you know.

Member

harawata commented May 11, 2016

I totally understand what you and your team want. Strange implementation indeed 😕

Anyway, I think I have found a solution to this problem.
I will open a new issue and let you know.

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 11, 2016

Member

Well, I should just reopen this one.

Member

harawata commented May 11, 2016

Well, I should just reopen this one.

@harawata harawata reopened this May 11, 2016

@harawata harawata added the bug label May 11, 2016

@harawata harawata self-assigned this May 11, 2016

harawata added a commit to mybatis/mybatis-3 that referenced this issue May 11, 2016

@harawata harawata closed this in 50f2742 May 11, 2016

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 11, 2016

Member

OK, I have fixed it.
migrate up command should now fail if the procedure has a compile error.

To try it, just clone this repository and execute mvn clean package.
The .zip bundle will be generated in the target directory.
Please let me know if it works.

Thank you!
Iwao

Member

harawata commented May 11, 2016

OK, I have fixed it.
migrate up command should now fail if the procedure has a compile error.

To try it, just clone this repository and execute mvn clean package.
The .zip bundle will be generated in the target directory.
Please let me know if it works.

Thank you!
Iwao

@harawata harawata added this to the 3.2.1 milestone May 11, 2016

@jmodi1982

This comment has been minimized.

Show comment
Hide comment
@jmodi1982

jmodi1982 May 11, 2016

Thanks for the fix. appreciated.

Will it print the compilation error as well ?

jmodi1982 commented May 11, 2016

Thanks for the fix. appreciated.

Will it print the compilation error as well ?

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 11, 2016

Member

I don't think so. The only message I see in the warning is that the statement execution completed with an error.

Member

harawata commented May 11, 2016

I don't think so. The only message I see in the warning is that the statement execution completed with an error.

@jmodi1982

This comment has been minimized.

Show comment
Hide comment
@jmodi1982

jmodi1982 May 11, 2016

Is there any configuration or code by which we can enable the error message as well?

jmodi1982 commented May 11, 2016

Is there any configuration or code by which we can enable the error message as well?

@jmodi1982

This comment has been minimized.

Show comment
Hide comment
@jmodi1982

jmodi1982 May 12, 2016

Hi,

Is it possible to provide the error message from show errors table ?

Thanks,
Jeemish

jmodi1982 commented May 12, 2016

Hi,

Is it possible to provide the error message from show errors table ?

Thanks,
Jeemish

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 12, 2016

Member

How about this?

You create a special script file 'onabort.sql' in the scripts directory.
The content would look as follows.

-- Reset delimiter in case creating procedure was aborted
-- @DELIMITER ;
-- Output error. As this is a global file, you cannot specify the procedure name.
SELECT * FROM SYS.USER_ERRORS;

Whenever migration up is aborted by SQLException, Migrations executes this script (only if the file exists, of course).

For example, here is the result of migrate up if I removed the semicolon at the end of the INSERT statement of the above script.

------------------------------------------------------------------------
-- MyBatis Migrations - up
------------------------------------------------------------------------
========== Applying: 20160506152242_first_migration.sql ========================
--  First migration.
-- Migration SQL that makes the change goes here.
CREATE OR REPLACE PROCEDURE myproc IS
/*
Comments
Comments
Comments
*/
BEGIN
        INSERT INTO person (id, name1) VALUES (999, 'John')
END;


Error executing: CREATE OR REPLACE PROCEDURE myproc IS
/*
Comments
Comments
Comments
*/
BEGIN
        INSERT INTO person (id, name1) VALUES (999, 'John')
END;

.  Cause: java.sql.SQLWarning: Warning: execution completed with warning

========== Executing onabort.sql script. =======================================
-- reset delimiter in case creating procedure was aborted.
SELECT * FROM SYS.USER_ERRORS WHERE type='PROCEDURE'

NAME    TYPE    SEQUENCE        LINE    POSITION        TEXT    ATTRIBUTE       MESSAGE_NUMBER  
MYPROC  PROCEDURE       3       11      0       PLS-00103: Encountered the symbol "end-of-file" when expecting one of the following:

   ( begin case declare end exception exit for goto if loop mod
   null pragma raise return select update while with
   <an identifier> <a double-quoted delimited-identifier>
   <a bind variable> << continue close current delete fetch lock
   insert open rollback savepoint set sql execute commit forall
   merge pipe purge
        ERROR   103
MYPROC  PROCEDURE       2       8       2       PL/SQL: SQL Statement ignored   ERROR   0
MYPROC  PROCEDURE       1       8       54      PL/SQL: ORA-00933: SQL command not properly ended       ERROR   0

------------------------------------------------------------------------
-- MyBatis Migrations FAILURE
-- Total time: 0s
-- Finished at: Thu May 12 23:43:12 JST 2016
-- Final Memory: 25M/479M
------------------------------------------------------------------------

Does this meet your requirement?

Member

harawata commented May 12, 2016

How about this?

You create a special script file 'onabort.sql' in the scripts directory.
The content would look as follows.

-- Reset delimiter in case creating procedure was aborted
-- @DELIMITER ;
-- Output error. As this is a global file, you cannot specify the procedure name.
SELECT * FROM SYS.USER_ERRORS;

Whenever migration up is aborted by SQLException, Migrations executes this script (only if the file exists, of course).

For example, here is the result of migrate up if I removed the semicolon at the end of the INSERT statement of the above script.

------------------------------------------------------------------------
-- MyBatis Migrations - up
------------------------------------------------------------------------
========== Applying: 20160506152242_first_migration.sql ========================
--  First migration.
-- Migration SQL that makes the change goes here.
CREATE OR REPLACE PROCEDURE myproc IS
/*
Comments
Comments
Comments
*/
BEGIN
        INSERT INTO person (id, name1) VALUES (999, 'John')
END;


Error executing: CREATE OR REPLACE PROCEDURE myproc IS
/*
Comments
Comments
Comments
*/
BEGIN
        INSERT INTO person (id, name1) VALUES (999, 'John')
END;

.  Cause: java.sql.SQLWarning: Warning: execution completed with warning

========== Executing onabort.sql script. =======================================
-- reset delimiter in case creating procedure was aborted.
SELECT * FROM SYS.USER_ERRORS WHERE type='PROCEDURE'

NAME    TYPE    SEQUENCE        LINE    POSITION        TEXT    ATTRIBUTE       MESSAGE_NUMBER  
MYPROC  PROCEDURE       3       11      0       PLS-00103: Encountered the symbol "end-of-file" when expecting one of the following:

   ( begin case declare end exception exit for goto if loop mod
   null pragma raise return select update while with
   <an identifier> <a double-quoted delimited-identifier>
   <a bind variable> << continue close current delete fetch lock
   insert open rollback savepoint set sql execute commit forall
   merge pipe purge
        ERROR   103
MYPROC  PROCEDURE       2       8       2       PL/SQL: SQL Statement ignored   ERROR   0
MYPROC  PROCEDURE       1       8       54      PL/SQL: ORA-00933: SQL command not properly ended       ERROR   0

------------------------------------------------------------------------
-- MyBatis Migrations FAILURE
-- Total time: 0s
-- Finished at: Thu May 12 23:43:12 JST 2016
-- Final Memory: 25M/479M
------------------------------------------------------------------------

Does this meet your requirement?

@jmodi1982

This comment has been minimized.

Show comment
Hide comment
@jmodi1982

jmodi1982 May 12, 2016

Yes, this would meet our requirements, however we would have the SQL command to show errors for Procedure, Functions, Packages, Triggers as well. That should work isn't it ?

Is this change already implemented in the source code ?

Thanks,
Jeemish

jmodi1982 commented May 12, 2016

Yes, this would meet our requirements, however we would have the SQL command to show errors for Procedure, Functions, Packages, Triggers as well. That should work isn't it ?

Is this change already implemented in the source code ?

Thanks,
Jeemish

@jmodi1982

This comment has been minimized.

Show comment
Hide comment
@jmodi1982

jmodi1982 May 12, 2016

Just wanted to confirm MyBatis UP command will fail and throw a similar warning if there is a compilation error in Functions, Packages, Triggers as well.

jmodi1982 commented May 12, 2016

Just wanted to confirm MyBatis UP command will fail and throw a similar warning if there is a compilation error in Functions, Packages, Triggers as well.

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 12, 2016

Member

Just wanted to confirm MyBatis command will fail and throw a similar warning if there is a compilation error in Functions, Packages, Triggers as well.

I would expect so. You should be able to verify this behavior with the current snapshot if you want to be 100% sure.

I need to clean up the code a little bit before pushing the change.

Member

harawata commented May 12, 2016

Just wanted to confirm MyBatis command will fail and throw a similar warning if there is a compilation error in Functions, Packages, Triggers as well.

I would expect so. You should be able to verify this behavior with the current snapshot if you want to be 100% sure.

I need to clean up the code a little bit before pushing the change.

harawata added a commit to harawata/migrations that referenced this issue May 14, 2016

Add OnAbort script executed when migration failed. Mainly to display …
…error when creating procedure etc. failed on Oracle (see #46).

harawata added a commit that referenced this issue May 14, 2016

Add OnAbort script executed when migration failed. Mainly to display …
…error when creating procedure etc. failed on Oracle (see #46). (#47)
@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 14, 2016

Member

@jmodi1982 The onabort.sql script should work with the latest 3.3.0-SNAPSHOT.

Member

harawata commented May 14, 2016

@jmodi1982 The onabort.sql script should work with the latest 3.3.0-SNAPSHOT.

@harawata harawata modified the milestones: 3.2.1, 3.3.0 Jun 11, 2016

@svscorp

This comment has been minimized.

Show comment
Hide comment
@svscorp

svscorp Oct 25, 2016

@harawata latest snapshot doesn't start (compiled, but errors out on run).

"could not find or load main class org.apache.ibatis.migration.Migrator".

Same issue (as the topic) exists with creating triggers - trying to get a working version of 3.3.0-snapshot (do you have one for download?).

svscorp commented Oct 25, 2016

@harawata latest snapshot doesn't start (compiled, but errors out on run).

"could not find or load main class org.apache.ibatis.migration.Migrator".

Same issue (as the topic) exists with creating triggers - trying to get a working version of 3.3.0-snapshot (do you have one for download?).

@harawata harawata changed the title from MyBatis migration not compiling procedures to migration succeeds even if compiling procedures failed Apr 25, 2017

harawata added a commit that referenced this issue Oct 16, 2017

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata Oct 16, 2017

Member

Hi @jmodi1982 ,

In 3.3.2, a new per-environment option ignore_warnings is added and its default value is true.
So, by default, warnings do not stop migrations.
For your use case, you may have to add ignore_warnings=false to your environment properties files.

We've got several reports about migration being aborted by warnings unexpectedly and decided to change the default behavior.
I apologize for the confusing move and hope you support our decision.

Thank you!

Member

harawata commented Oct 16, 2017

Hi @jmodi1982 ,

In 3.3.2, a new per-environment option ignore_warnings is added and its default value is true.
So, by default, warnings do not stop migrations.
For your use case, you may have to add ignore_warnings=false to your environment properties files.

We've got several reports about migration being aborted by warnings unexpectedly and decided to change the default behavior.
I apologize for the confusing move and hope you support our decision.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment