Fix for working with Firebird 2 and newer versions. Fixes nullables and boolean type. #2277
Conversation
|
Thanks @mngsgoncalves for taking the time to look into this and propose the fix. We are reviewing. |
|
I like the removal of the Firebird3 class since that is not really our standard way of working. However, if anyone has extensions that depend on Firebird3 that will break them, so we may need to re-introduce it as a deprectated class to be safe. I'm currently working on a big improvement to the integration test system, and am going to leave this open for a little bit since it will be a good test of it as I add firebird support in. Should be able to get to it soon, though. |
…ves/liquibase into mngsgoncalves-fix-firebird-booleans # Conflicts: # liquibase-core/src/main/resources/META-INF/services/liquibase.database.Database # liquibase-integration-tests/src/test/java/liquibase/statementexecute/UnlockDatabaseChangeLogExecuteTest.java
…tMajorVersion() call
There was a problem hiding this comment.
I switched the custom isVersion2() logic for the standard getDatabaseMajorVersion check, with an assumption it's 3+ since 2.x versions are no longer supported by firebird.
Otherwise looks good, and the test-harness tests should catch any issues with firebird 3 or 4
nvoxland
added
the
SafeToBuild
|
The failing functional test is unrelated to the changes |
| @@ -19,11 +20,18 @@ public class BooleanType extends LiquibaseDataType { | |||
| @Override | |||
| public DatabaseDataType toDatabaseDataType(Database database) { | |||
| String originalDefinition = StringUtil.trimToEmpty(getRawDefinition()); | |||
| if ((database instanceof Firebird3Database)) { | |||
| if ((database instanceof FirebirdDatabase)) { | |||
| try { | |||
There was a problem hiding this comment.
As a non-developer, I find the use of a try/catch instead of if/else interesting. If anyone has time, can you please explain the advantages of this approach? I'm always happy to learn something new. :)
There was a problem hiding this comment.
@XDelphiGrl try/catch is how Java does the exception handling, use of try/catch blocks segregates error-handling code and program code making it easier to identify the logical flow of a program. The logic in the program code does not include details of the actions to be performed when an exception occurs. Such details are present in the catch blocks.
There was a problem hiding this comment.
@nvoxland Do we want to log the exception??
There was a problem hiding this comment.
Thank you for the explanation, @suryaaki2!
There was a problem hiding this comment.
I added a log of the exception
There was a problem hiding this comment.
This change replaces the Firebird3Database class with more streamlined logic to determine Firebird version. Depending on the Firebird version, different SQL is generated for both setting the boolean datatype and setting the nullable state of a column.
- Fix specific to the Firebird database.
- Existing unit tests updated to reflect removal of the Firebird3Database class.
- No additional testing required.
APPROVED
Functional Tests
Passing Test Harness Execution
NOTE: Test failures in the functional test suite are due to mismatch in expected console output and actual console output for the checks feature. These are unrelated to the changes in this PR and can be disregarded.
| @@ -19,11 +20,18 @@ public class BooleanType extends LiquibaseDataType { | |||
| @Override | |||
| public DatabaseDataType toDatabaseDataType(Database database) { | |||
| String originalDefinition = StringUtil.trimToEmpty(getRawDefinition()); | |||
| if ((database instanceof Firebird3Database)) { | |||
| if ((database instanceof FirebirdDatabase)) { | |||
| try { | |||
There was a problem hiding this comment.
@XDelphiGrl try/catch is how Java does the exception handling, use of try/catch blocks segregates error-handling code and program code making it easier to identify the logical flow of a program. The logic in the program code does not include details of the actions to be performed when an exception occurs. Such details are present in the catch blocks.
| @@ -19,11 +20,18 @@ public class BooleanType extends LiquibaseDataType { | |||
| @Override | |||
| public DatabaseDataType toDatabaseDataType(Database database) { | |||
| String originalDefinition = StringUtil.trimToEmpty(getRawDefinition()); | |||
| if ((database instanceof Firebird3Database)) { | |||
| if ((database instanceof FirebirdDatabase)) { | |||
| try { | |||
There was a problem hiding this comment.
@nvoxland Do we want to log the exception??
Fix for working with Firebird 2 and newer versions. Fixes nullables and boolean type.
Current implementation does not handle correctly working on Firebird 2.5 and Firebird 3 databases. It always assumes a Firebird 3 database, which incurs in invalid SQL statements.
The proposed fix eliminates the Firebird3Database class and instead includes a "version2" attribute in the FirebirdDatabase class. This attribute is populated when the connection is set.
I've fixed the tests and impacted classes (BooleanType and SetNullableGenerator) for this implementation.