hibernate integration + diff bug

General Discussion
1

hi !
first off i’d like to say i’m really glad i found liquibase, the idea and features are great.
I was planning to use hbm2ddl to incrementally update the database after changes to my hibernate mappings. Unfortunately i had to find out that hbm2ddl either does not remove dropped columns/tables or drops and recreates everything if you force it to take care of that issue. Because i need to keep the data that is already stored i was going to make my own evil dirty version of hbm2ddl that should do the trick, but lucky me, i found liquibase.

i use Diff with a HibernateDatabase created with my hibernate.cfg.xml file and a MySQLDatabase.
The change-sets are written like that:

    Diff diff = new Diff(hDB, mySQLDB); diff.addStatusListener(new LogDiffStatusListener()); DiffResult diffResult = diff.compare();         logger.debug("writing diffLog file: " + changeLogFile); diffResult.printChangeLog(changeLogFile, mySQLDB);

it works very well most of the time but i found a couple of things that i consider to be bugs:
(Version 1.9.3)

1. changes of datatypes are not detected (deliberately ignored)
sourcefile: HibernateDatabaseSnapshot.java

    #82 column.setCertainDataType(false);
in my case i tried to change an integer to a double, but because every column in a HibernateDatabaseSnapshot is set to have no "certain datatype" the method isDataTypeDifferent of Column ignores the difference in line #273
    #272 public boolean isDataTypeDifferent(Column otherColumn) { #273 if (!this.isCertainDataType() || !otherColumn.isCertainDataType()) { #274 return false; #275 ...
during the diff.compare() process.

2. invalid schema for drop column change-set
sourcefile: DiffResult.java

    #633 change.setSchemaName(column.getTable().getName());
In my opinion this is a copy paste typo and i wonder why it has not been detected by now. It should instead be column.getTable().getSchema(). Therefore it currently leads to a com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: Table 'tablename.tablename doesn't exist exception if the exported change-set is used to update the database.

I will try to fix these small things and see if they are the only issues that prevent me from successful ddl update with liquibase. Hopefully more people will use this tool with hibernate so it will mature in quality and features :slight_smile:

2

If you could improve them, that would be great.  You can either send me a patch, or commit the change to the svn directly and review it then. 

My envisioned use case for the hibernate integration is:

  1. Make a change to your hibernate mapping
  2. Run the diff between your database and hibernate
  3. Check the generated changeset to see if it is really doing what you want it to, or if it should be changed (rename instead of drop/add, fix data types, etc.)
  4. Run liquibase update.

So there is that manual step 3 which is necessary to have, otherwise you are back to hbm2ddl.  The smoother we can make step 3, however, the better.

Nathan

3

Hi Nathan!

I totally agree with you that changes to a database, that is used in a live application, should be reviewed before they are applied.
Unfortunately i am in the situation of being forced to automatically update the database DDL during application runtime. Its a very dirty thing to do, and i’d rather not do it but i have to. Nevertheless i think liquibase is the best candidate for the job, much better than hbm2ddl. So i will stick to it, and hope you forgive me for abusing your tool.

I dug into the liquibase source and made changes necessary to make it work for me. However i ran into troubles (as i expected) with the data types. To fix these troubles i had to make changes which affect more than just the hibernate part of the code. Therefore i did not send any patches, i can’t guarantee that it wont break the other use cases of liquibase. I’ll try to summarize the problem:

In the HibernateDatabaseSnapshot class are quite a lot of commented lines of code and //todo parts so i knew there was a reason why column.setCertainDataType was set to false.
Hibernate has the displeasing manner of setting predefined default values for things like precision, scale, length and so on, no matter what datatype the column has. Liquibase Columns (in my case coming from a MysqlDatabaseSnapshot) have a default of 0 for all data-type-related properties. (a varchar has no precision or scale, an integer has no length, etc.)
So just setting setCertainDataType to true has the effect that all columns are detected as having changed, even tough they are the same and just differ in the unused default values. How to avoid this ? My approach was “just set the properties (scale, precision, length, etc.) on the snapshot columns that are appropriate for the datatype of the hibernate column”. To do this, you need to know which properties are relevant for which types. Currently this information/logic is in the Column class inside the getDataTypeString method (or something like that, sorry if i don’t remember correctly, i don’t have any code atm). Since i needed this information, and i think this information should be more accessible to other classes, i moved it to the SQLUtil class.
Another approach would be to change the isDataTypeDifferent Method of the column to only check the difference of the properties relevant to the type but i thought it would be cleaner to only set the values if they make sense and keep the logic of isDataTypeDifferent simple.
After i made those and other small changes i also ran into the recently reported MySQL-Double-Bug, and unfortunately a couple more MySQL specific type-parameter issues. I had to change the lists of those noParens, oneParam, twoParams lists and bloat the Columns getDataTypeString method with a some more MySQL specific special cases. It works now after a lot of tweaks but it is not a sustainable solution.
It seems like it would be better to move that part into the database specific classes, i.e. the database knows if it wants one or two parameters for a type and the syntax of the type instead of having the Column maintaining lists of types and parameters and exceptions from those lists and special syntax exceptions and lots of database instanceof SomeSpecificDatabase . . .

I don’t know all differences of all the databases and versions and SQL standards, and most important, the big picture of liquibase so i don’t feel confident to make such decisions.
You’re the expert on this topic and its a decision that only you can make, thats why i did not commit or send anything.

But who knows what you have come up with in 2.0, maybe all these things have changed anyway.
Well anyway, thanks for reading and i hope i can make it to the chat tomorrow/today :wink:

4

You make some very good points.  I think that some of them are cleared up a bit in 2.0, but not all.  If your changes are in a state that you could easily generate a patch, I could apply it and look over what you changed and incorporate what looks good and either modify or disregard code that may break other areas.  The hibernate diff is definitely something we would like improved.

Hope to see you at the chat

Nathan