Improved Sybase ASE Support #665

carlos940513 · 2017-03-16T21:50:58Z

Changes made on this PR are listed below:

Improved view snapshot support: modified SybaseDatabase.java class, so that delete the words CREATE VIEW name_view as, to did not send error when applied to another DB.
Improved data type handling: modified the BigIntType.java, IntType.java, SmallIntType.java, TinyIntType.java classes, the Sybase instance was added so that it did not write the size of these data types in the ChangeLog.
Improved system-managed table ignoring
Added unit tests

ro-rah · 2020-05-13T20:23:51Z

Hi @carlos940513

Thanks for your pull request!

Here’s what happens next:

A member of the Liquibase team will take a look at your contribution and may suggest:

Changes based on guidelines for PR submission
Additional tests (unit and/or integration)
Provide other feedback, like requesting changes.

The PR will be prioritized according to our internal development and testing capacity.

We’ll let you know when it’s ready to move to the next step or if any changes are needed.

ro-rah · 2020-05-13T20:24:37Z

Hi @carlos940513 , would you please resolve the branch conflicts and consider adding tests?

Thanks,

Ronak

github-actions · 2022-10-05T05:34:11Z

Unit Test Results

  4 740 files ±  0   4 740 suites ±0 39m 29s ⏱️ + 6m 42s
  4 695 tests +  3   4 459 ✔️ +  3   236 💤 ±0 0 ❌ ±0
55 560 runs +36 50 252 ✔️ +36 5 308 💤 ±0 0 ❌ ±0

Results for commit 9f802cc. ± Comparison against base commit b88ce92.

♻️ This comment has been updated with latest results.

nvoxland

Code review and test results:

After resolving conflicts, the list of things still needing fixed by this PR are:

Fixed view definition snapshot logic to remove "create view as" if the database returns it
Fixed handling of bigint/int/smallint/tinyint when arguments are specified (sybase does not support them)
Better ignore columns that start with "sybfi" which are system-managed
Better handling of foreign key onUpdate/onDelete (sybase does not support them)
Better handling of numbers being used as booleans that have trailing non-number content in them

Things to be aware of:

Changes are limited to sybase

Things to worry about:

Not good test automation around sybase to see and test that the changes work. But they all appear to make sense to me and as we add sybase testing support, it will catch regressions if any of these changes get undone

liquibase-core/src/main/java/liquibase/database/core/SybaseDatabase.java

XDelphiGrl · 2022-10-11T16:26:39Z

liquibase-core/src/main/java/liquibase/datatype/core/BigIntType.java

@@ -70,6 +70,9 @@ public DatabaseDataType toDatabaseDataType(Database database) {
                }
            }
        }
+        if (database instanceof SybaseDatabase) {


Can the SybaseASADatabase and SybaseDatabase "ifs" be included as an "or" in the "if" at line 56?

We were actually already checking for sybase up on line 44 so I just moved SybaseASA up there.

It doesn't super matter, I had just left it as it came though in the merge

XDelphiGrl · 2022-10-11T16:29:22Z

liquibase-core/src/main/java/liquibase/datatype/core/IntType.java

@@ -72,6 +72,9 @@ public DatabaseDataType toDatabaseDataType(Database database) {
        if (database instanceof SybaseASADatabase) {
            return new DatabaseDataType("INTEGER");
        }
+        if (database instanceof SybaseDatabase) {


Similar question to the one in BigIntType.java. Can this be moved to be an "or" in the "if" at line 65?

It can but doesn't really matter. I moved it since I was taking a couple things out otherwise.

XDelphiGrl · 2022-10-11T16:32:11Z

liquibase-core/src/main/java/liquibase/datatype/core/SmallIntType.java

@@ -61,7 +61,9 @@ public DatabaseDataType toDatabaseDataType(Database database) {
            }
            return new DatabaseDataType("SMALLINT"); //always smallint regardless of parameters passed
        }
-
+        if (database instanceof SybaseDatabase) {


Should the SybaseASADatabase also be included in this logic? Same question for TinyIntType.java.

I'm not sure about smallint vs. tinyint. I didn't go hunting through their docs on what datatypes they support and was just reviewing this PR for the fixes it specifically had.

XDelphiGrl · 2022-10-11T16:36:19Z

liquibase-core/src/main/java/liquibase/snapshot/jvm/ForeignKeySnapshotGenerator.java

 import liquibase.database.jvm.JdbcConnection;
-import liquibase.database.core.MariaDBDatabase;


Were these imports for DB2Database, MSSQLDatabase and MariaDBDatabase removed by accident?

No accidentally. With the merge from master, I did a "fix code layout" which also removes unneeded imports. If they were needed, the compile would have failed.

XDelphiGrl · 2022-10-11T16:40:10Z

liquibase-core/src/main/java/liquibase/sqlgenerator/core/AddForeignKeyConstraintGenerator.java

@@ -80,6 +82,8 @@ public Sql[] generateSql(AddForeignKeyConstraintStatement statement, Database da
                // see "REFERENCES Clause" in manual
            } else if ((database instanceof FirebirdDatabase) && "RESTRICT".equalsIgnoreCase(statement.getOnDelete())) {
                //don't use
+            } else if (database instanceof SybaseDatabase) {


Should SybaseASADatabase be here, too?

I'm not sure. This PR was just for sybase support, not sybase asa so didn't look at that.

XDelphiGrl

@nvoxland, hello - Will you please take a look at my comments?

CHANGES REQUESTED

…40513/liquibase into carlos940513-Liquibase-sybase-changes

…inor refactorings

MalloD12 · 2022-10-31T13:36:11Z

@nvoxland, hello - Will you please take a look at my comments?

CHANGES REQUESTED

Hey @XDelphiGrl ,

Could you please take a look to this PR when you have a chance? I have addressed I think most of the comments you have made here?

Thanks,
Daniel.

…ase-sybase-changes # Conflicts: # liquibase-core/src/main/java/liquibase/datatype/core/BigIntType.java # liquibase-core/src/main/java/liquibase/datatype/core/IntType.java # liquibase-core/src/main/java/liquibase/datatype/core/SmallIntType.java

MalloD12

Review and testing results:

Overall changes look ok to me. The only things I did here were:

Fixed unit tests failing.
Update some IF blocks from Data Types classes as suggested in some of the review comments.

Things to be aware of:

These changes have only an impact on Sybase and SybaseASADatabase DBs.

Things to worry about:

Nothing

XDelphiGrl

Several improvements for Sybase ASE handling are introduced in this PR. See this comment for a comprehensive list.

New tests added for numeric datatype handling.
Automated functional and test harness executions passing.
No additional testing required.

APPROVED

carlos940513 added 6 commits March 16, 2017 14:09

delete the words CREATE VIEW name_view as

02c2002

delete the words CREATE VIEW [name_view] as

e4f2755

re-write pom.xml

9aefc6c

POM:XML import Eclipse

0683cf8

Remove columns with the name sybfi

fe65086

Remove tags onDelete and onUpdate if the database is Sybase

29816f9

datical-jenkins added the Status:Discovery label Mar 3, 2020

datical-jenkins changed the title ~~Liquibase sybase changes~~ LB-107 ⁃ Liquibase sybase changes Mar 4, 2020

datical-jenkins changed the title ~~LB-107 ⁃ Liquibase sybase changes~~ Liquibase sybase changes Mar 5, 2020

datical-jenkins added Status:Discovery and removed Status:Discovery labels Mar 25, 2020

kataggart removed the StatusDiscovery label May 23, 2022

kataggart added the DBSybase label Aug 11, 2022

nvoxland added 3 commits October 4, 2022 23:26

Merged code from master

bdfcad6

Cleaned up code changes

8143a01

Added tests

e4d9cf5

nvoxland self-requested a review as a code owner October 5, 2022 05:09

nvoxland changed the base branch from master to 1_9 October 5, 2022 05:09

nvoxland changed the base branch from 1_9 to master October 5, 2022 05:09

Reverting formatting-only changes

1f2e35b

nvoxland added the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label Oct 5, 2022

Removed unneded if statements

fb7ea9d

nvoxland added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Oct 6, 2022

nvoxland approved these changes Oct 6, 2022

View reviewed changes

nvoxland added the autocandidate label Oct 6, 2022

XDelphiGrl reviewed Oct 11, 2022

View reviewed changes

liquibase-core/src/main/java/liquibase/database/core/SybaseDatabase.java Outdated Show resolved Hide resolved

XDelphiGrl reviewed Oct 11, 2022

View reviewed changes

liquibase-core/src/main/java/liquibase/database/core/SybaseDatabase.java Outdated Show resolved Hide resolved

XDelphiGrl reviewed Oct 11, 2022

View reviewed changes

XDelphiGrl suggested changes Oct 11, 2022

View reviewed changes

Removed checks for super-old drivers from sybase

6f2dbe2

nvoxland added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Oct 11, 2022

Merge branch 'Liquibase-sybase-changes' of https://github.com/carlos9…

cdd4d5d

…40513/liquibase into carlos940513-Liquibase-sybase-changes

MalloD12 self-assigned this Oct 27, 2022

testGetDatabaseVersionWhenNotImplemented unit test fixed, plus some m…

70a52e4

…inor refactorings

MalloD12 changed the base branch from master to 1_9 October 28, 2022 13:33

MalloD12 changed the base branch from 1_9 to master October 28, 2022 13:34

MalloD12 added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Nov 1, 2022

MalloD12 added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Nov 8, 2022

MalloD12 changed the title ~~Liquibase sybase changes~~ Sybase improvements for datatype handling, snapshotting views, ignoring system-managed tables, and a foreign key bug fix Nov 9, 2022

MalloD12 self-requested a review November 9, 2022 19:38

MalloD12 approved these changes Nov 9, 2022

View reviewed changes

MalloD12 changed the title ~~Sybase improvements for datatype handling, snapshotting views, ignoring system-managed tables, and a foreign key bug fix~~ Improved Sybase ASE Support Nov 9, 2022

XDelphiGrl approved these changes Nov 15, 2022

View reviewed changes

filipelautert merged commit 2d333a2 into liquibase:master Nov 16, 2022

filipelautert added this to the 4.18.0 milestone Dec 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved Sybase ASE Support #665

Improved Sybase ASE Support #665

carlos940513 commented Mar 16, 2017 •

edited by MalloD12

ro-rah commented May 13, 2020

ro-rah commented May 13, 2020

github-actions bot commented Oct 5, 2022 •

edited

nvoxland left a comment

XDelphiGrl Oct 11, 2022

nvoxland Oct 11, 2022

XDelphiGrl Oct 11, 2022

nvoxland Oct 11, 2022

XDelphiGrl Oct 11, 2022 •

edited

nvoxland Oct 11, 2022

XDelphiGrl Oct 11, 2022

nvoxland Oct 11, 2022

XDelphiGrl Oct 11, 2022

nvoxland Oct 11, 2022

XDelphiGrl left a comment

MalloD12 commented Oct 31, 2022

MalloD12 left a comment

XDelphiGrl left a comment

		import liquibase.database.jvm.JdbcConnection;
		import liquibase.database.core.MariaDBDatabase;

Improved Sybase ASE Support #665

Improved Sybase ASE Support #665

Conversation

carlos940513 commented Mar 16, 2017 • edited by MalloD12

ro-rah commented May 13, 2020

ro-rah commented May 13, 2020

github-actions bot commented Oct 5, 2022 • edited

Unit Test Results

nvoxland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XDelphiGrl Oct 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XDelphiGrl left a comment

Choose a reason for hiding this comment

MalloD12 commented Oct 31, 2022

MalloD12 left a comment

Choose a reason for hiding this comment

Review and testing results:

Things to be aware of:

Things to worry about:

XDelphiGrl left a comment

Choose a reason for hiding this comment

carlos940513 commented Mar 16, 2017 •

edited by MalloD12

github-actions bot commented Oct 5, 2022 •

edited

XDelphiGrl Oct 11, 2022 •

edited