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

JDBCStateRepository not schema aware #38

Closed
tdtappe opened this Issue Apr 25, 2013 · 15 comments

Comments

Projects
None yet
2 participants
@tdtappe
Contributor

tdtappe commented Apr 25, 2013

It seems as if the JDBCStateRepository is not schema aware. At least I ran into problems when I tried to let Togglz create and migrate the metadata. It didn't add the strategy columns. And I noticed that the SchemaUpdater provides "null" for the schema pattern when fiddling with the metadata.

Version 2.0.0.Beta1, PostgreSQL

@chkal

This comment has been minimized.

Member

chkal commented Apr 30, 2013

Hi,

first of all: Sorry, for my delayed response. I was traveling last week, so I wasn't able to answer earlier.

is this perhaps related to #39? Looks like the same issue to me.

The JDBC spec says regarding the schema parameter:

null means that the schema name should not be used to narrow the search

I guess you are right. Passing null here is not a good idea. Although I would expect that Postgres returns the table (and maybe more located in other schemas) when passing null.

To be honest, I'm a bit unsure how to handle this case consistently across all databases.

The schema is important in two places:

  • The database migration when calling stuff like getTables()
  • In the SQL queries itself.

Any thought on this?

Christian

@tdtappe

This comment has been minimized.

Contributor

tdtappe commented Apr 30, 2013

For the delayed response: no problem at all. I could work around the problem quite easily.
In my own config class I check for migration before returning the JDBCStateRepository.
I copied the migrateSchema method with my own implementation of the SchemaUpdater.
In my own implementation I just modified two places: metaData.getTables and metaData.getColumns and provided the correct schema which is available as a system property in my case :-)

The SQL queries itself are not problematic (in my case) as they will use the default schema/user.

So it might be a good idea to provide the JDBCStateRepository with another optional argument for the schema to be used - especially for the metadata queries. What do you think?

--Heiko

@chkal

This comment has been minimized.

Member

chkal commented May 2, 2013

Yeah, basically you are right. But actually I don't think we should force the user to enter a schema name. It should be optional. But in this case it's difficult to decide what should be the default.

@tdtappe

This comment has been minimized.

Contributor

tdtappe commented May 2, 2013

Yes, the schema name should definitely be optional. And maybe let null be the default so that it works like before. Or you could try to get the current user name and use it as the schema name!?

@chkal

This comment has been minimized.

Member

chkal commented May 2, 2013

I also thought of using the user name, but this isn't the default for MSSQL which uses "dbo" as the default schema.

I'll play a bit with different approaches. Could you perhaps help me by testing the new approach against Postgres as soon as it is done? That would be awesome. :)

@tdtappe

This comment has been minimized.

Contributor

tdtappe commented May 3, 2013

Oh, really? I didn't know that about MSSQL.
And yes, of course - I can do some testing against Postgres. No problem.

@ghost

This comment has been minimized.

ghost commented May 6, 2013

Hi, is there any progress in regarding this issue? I can also test againsts Postgres

@chkal

This comment has been minimized.

Member

chkal commented May 6, 2013

No, sorry, but I'll hope to find some time tomorrow to work on this.

The recommended way to work around this issue is to do the following:

  1. Create the database table manually with the structure described in the javadocs
  2. Set createTable=false in the JDBCStateRepository constructor.

See the java docs for JDBCStateRepository for details:

http://www.togglz.org/apidocs/2.0.0.Beta1/org/togglz/core/repository/jdbc/JDBCStateRepository.html

@ghost

This comment has been minimized.

ghost commented May 6, 2013

Thanks for your quick reply, i used this quick "hack" since i keep creating new environments:

            if (!updater.doesTableExist()) {
                updater.migrateToVersion1();
                updater.migrateToVersion2();//quick fix
            }

            if (updater.isSchemaVersion1()) {
                updater.migrateToVersion2();
            }

throws an exception on startup but works great until the patch will comeout :)

@chkal

This comment has been minimized.

Member

chkal commented May 6, 2013

OK, great! I hope I'll be able to fix this issue soon.

@chkal

This comment has been minimized.

Member

chkal commented May 6, 2013

Hey all,

I think this issue should be fixed now. I deployed new snapshots for you to test. If you want to give it a try, you have to add this snapshot repository to your pom.xml:

https://oss.sonatype.org/content/repositories/snapshots/

Then set the Togglz version to 2.0.0-SNAPSHOT.

Let me know if you need more detailed instructions on how to use the snapshots.

Looking forward to your feedback. :)

Christian

@tdtappe

This comment has been minimized.

Contributor

tdtappe commented May 7, 2013

Great! Seems to work. At least for me using Postgres.
Just out of curiosity: How did you solve the problem?
And when do you think you will publish a new (non-snapshot) release? But of course - no hurry! I am fine with what I have :-)

@chkal

This comment has been minimized.

Member

chkal commented May 7, 2013

Nice to hear that it's working now.

I had a look at the Liquibase source (a popular tool for database schema migration) to find out how they do it. Actually they are using database dependent classes who can answer questions like "what is the default schema for this db?". However IMHO this is an overkill for Togglz.

So I decided to go with a very naive way of determining the current version of the database schema. Actually I perform multiple queries which read the table and/or specific column. If specific queries work and others not, I can identify which version of the table is currently in the DB and send the corresponding migration commands.

See:

https://github.com/togglz/togglz/blob/master/core/src/main/java/org/togglz/core/repository/jdbc/SchemaUpdater.java#L34

I know, this is very dirty. But I see no other easy way to solve this issue. And actually it works fine. :)

I think I'll publish a new beta release by the end of this week or early next week. So stay tuned. :)

@ghost

This comment has been minimized.

ghost commented May 9, 2013

looking forward to the release :)

@chkal

This comment has been minimized.

Member

chkal commented May 10, 2013

I just pushed 2.0.0.Beta2 out. It should sync to central in a few hours.

@chkal chkal closed this May 10, 2013

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