-
Notifications
You must be signed in to change notification settings - Fork 10
Adding support for schema/catalog/table arguments #16
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| package org.hibernate.forge.generate; | ||
|
|
||
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| import org.hibernate.cfg.reveng.DefaultReverseEngineeringStrategy; | ||
| import org.hibernate.cfg.reveng.ReverseEngineeringStrategy; | ||
| import org.hibernate.cfg.reveng.SchemaSelection; | ||
| import org.hibernate.forge.common.Constants; | ||
| import org.hibernate.util.StringHelper; | ||
|
|
||
| /** | ||
| * Defines a {@link SchemaSelection} limitation on reverse engineering strategy. | ||
| * | ||
| * @author jpereira - Linkare TI | ||
| * | ||
| */ | ||
| public class DefaultReverseEngineeringStrategyWithSchemaSelection extends | ||
| DefaultReverseEngineeringStrategy { | ||
|
|
||
| private List<SchemaSelection> schemaSelections; | ||
|
|
||
| /** | ||
| * Creates a new {@link ReverseEngineeringStrategy} with additional | ||
| * limitations by catalog, schema and table | ||
| * | ||
| * @param catalogs | ||
| * The catalogs to use ( FOO* matches any catalog named FOOX, | ||
| * FOOBAR, FOO, etc) | ||
| * @param schemas | ||
| * The schemas to use ( FOO* matches any schema named FOOX, | ||
| * FOOBAR, FOO, etc) | ||
| * @param tables | ||
| * The tables to use ( FOO* matches any table named FOOX, FOOBAR, | ||
| * FOO, etc) | ||
| */ | ||
| public DefaultReverseEngineeringStrategyWithSchemaSelection( | ||
| String catalogs, String schemas, String tables) { | ||
|
|
||
| String catalog = catalogs == null ? Constants.CATALOG_DEFAULT | ||
| : catalogs; | ||
| String schema = schemas == null ? Constants.SCHEMA_DEFAULT : schemas; | ||
| String table = tables == null ? Constants.TABLE_DEFAULT : tables; | ||
|
|
||
| catalog = StringHelper.replace(catalog, "*", "%"); | ||
| schema = StringHelper.replace(schema, "*", "%"); | ||
| table = StringHelper.replace(table, "*", "%"); | ||
|
|
||
| SchemaSelection schemaSelection = new SchemaSelection(catalog, schema, | ||
| table); | ||
| schemaSelections = Collections.singletonList(schemaSelection); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That means only one schema will be parsed ? What about the substring match ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It works for many schemas also... Just check the test with the schemas JP On Tue, May 7, 2013 at 6:16 PM, George Gastaldi [email protected]:
[image: Linkare TI - http://www.linkare.com] » José Pedro Pereira | CTO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not needed. Just force push in your same branch and the pull request will be automatically updated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll do it just now... it might take a minute or two! But you did understand that it works for many schemas, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I see it works in your test case. As I commented earlier, is it possible to keep the original behavior ? Using TEST* for TEST1 and TEST2 instead of TEST.* as your test suggests ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be quite feasible... I was taking advantage of the hibernate-tools code that replaces .* by %... But I think I can replace it myself (* by %) and then hibernate-tools code would just not change it... Another way would be to let the user specify % directly (TEST% instead of TEST*)... As we are not talking about entities but rather, table names and alike, it should definitely make more sense... The beauty of what I'll try to implement is that it will support both * and %. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes., that would be much better ! Thank you ! Great job on that! |
||
| } | ||
|
|
||
| /** | ||
| * Obtains the list of schema selections (built at construct time) | ||
| * | ||
| * @return The schema limitations for this reverse engineering strategy | ||
| */ | ||
| @Override | ||
| public List getSchemaSelections() { | ||
| return schemaSelections; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| import org.hibernate.cfg.reveng.DefaultReverseEngineeringStrategy; | ||
| import org.hibernate.cfg.reveng.ReverseEngineeringSettings; | ||
| import org.hibernate.cfg.reveng.ReverseEngineeringStrategy; | ||
| import org.hibernate.cfg.reveng.TableIdentifier; | ||
| import org.hibernate.forge.common.Constants; | ||
| import org.hibernate.forge.common.UrlClassLoaderExecutor; | ||
| import org.hibernate.forge.connections.ConnectionProfile; | ||
|
|
@@ -73,7 +74,7 @@ public void generateEntities( | |
| { | ||
| ConnectionProfile connectionProfile = buildConnectionProfile(connectionProfileName, url, user, password, dialect, driver, path); | ||
| JDBCMetaDataConfiguration jmdc = configureMetaData(connectionProfile); | ||
| jmdc.setReverseEngineeringStrategy(createReverseEngineeringStrategy(packageName, manyToMany, oneToOne, optimisticLock)); | ||
| jmdc.setReverseEngineeringStrategy(createReverseEngineeringStrategy(packageName, manyToMany, oneToOne, optimisticLock, catalogs,schemas, tables)); | ||
| try { | ||
| doReverseEngineering(connectionProfile.driver, connectionProfile.path, jmdc); | ||
| } catch (Throwable t) { | ||
|
|
@@ -127,8 +128,8 @@ private ReverseEngineeringStrategy createReverseEngineeringStrategy( | |
| String packageName, | ||
| Boolean manyToMany, | ||
| Boolean oneToOne, | ||
| Boolean optimisticLock) { | ||
| ReverseEngineeringStrategy strategy = new DefaultReverseEngineeringStrategy(); | ||
| Boolean optimisticLock, String catalogs, String schemas, String tables) { | ||
| ReverseEngineeringStrategy strategy = new DefaultReverseEngineeringStrategyWithSchemaSelection(catalogs,schemas,tables); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks a tad problematic to me. This means all schemas will be scanned by default no matter what hibernate tools metadatadialects states. I also means if we at some point add support for passing in reveng.xml or some other configuration that will be delegating to this default strategy it will overrule any configuration from there. This strategy should only be enable if the user has explicitly set one of catalogs, schemas or tables parameters. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inline.... On 7 May 2013 19:48, "Max Rydahl Andersen" [email protected] wrote:
It actually doesn't seem at all problematic, since if not specified, the Catalog=%
As I've seen from the code, this is not true, as the
I think that this would require a much deeper refactoring of the forge
This is totally irrelevant, as if the schema selection is null or the
|
||
| ReverseEngineeringSettings revengsettings = | ||
| new ReverseEngineeringSettings(strategy) | ||
| .setDefaultPackageName(determinePackageName(packageName)) | ||
|
|
@@ -176,6 +177,7 @@ public void run() { | |
| Thread.currentThread().getContextClassLoader()) | ||
| .newInstance(); | ||
| DriverManager.registerDriver(new DelegatingDriver(jdbcDriver)); | ||
|
|
||
| jmdc.readFromJDBC(); | ||
| jmdc.buildMappings(); | ||
| } catch (Exception e) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,10 +3,17 @@ | |
| import java.io.File; | ||
| import java.sql.Connection; | ||
| import java.sql.DriverManager; | ||
| import java.sql.SQLException; | ||
| import java.util.List; | ||
|
|
||
| import junit.framework.Assert; | ||
|
|
||
| import org.h2.tools.Server; | ||
| import org.hibernate.forge.connections.ConnectionProfile; | ||
| import org.jboss.arquillian.container.test.api.Deployment; | ||
| import org.jboss.forge.project.Project; | ||
| import org.jboss.forge.resources.DirectoryResource; | ||
| import org.jboss.forge.resources.Resource; | ||
| import org.jboss.forge.test.AbstractShellTest; | ||
| import org.jboss.forge.test.MavenArtifactResolver; | ||
| import org.jboss.shrinkwrap.api.spec.JavaArchive; | ||
|
|
@@ -44,13 +51,183 @@ public void testH2() throws Exception { | |
| + " --pathToDriver " + resolveH2DriverJarPath() | ||
| + " --url jdbc:h2:tcp://localhost/mem:test" | ||
| + " --dialect org.hibernate.dialect.H2Dialect" | ||
| + " --user foo" //+ " --password bar" | ||
| + " --user foo" // + " --password bar" | ||
| + " --entityPackage com.test.model"); | ||
|
|
||
| server.stop(); | ||
|
|
||
| } | ||
|
|
||
| @Test | ||
| public void testSchemaTestH2() throws Exception { | ||
|
|
||
| Server server = Server.createTcpServer().start(); | ||
| Class.forName("org.h2.Driver"); | ||
| Connection conn = DriverManager | ||
| .getConnection("jdbc:h2:tcp://localhost/mem:test;USER=foo;PASSWORD=bar"); | ||
| createSampleSchemasAndTables(conn); | ||
|
|
||
| Project javaProject = initializeJavaProject(); | ||
| queueInputLines(""); | ||
| getShell().execute("project install-facet forge.spec.jpa"); | ||
| queueInputLines("bar"); | ||
| getShell().execute( | ||
| "generate-entities" + " --driver org.h2.Driver" | ||
| + " --pathToDriver " + resolveH2DriverJarPath() | ||
| + " --url jdbc:h2:tcp://localhost/mem:test" | ||
| + " --dialect org.hibernate.dialect.H2Dialect" | ||
| + " --user foo" // + " --password bar" | ||
| + " --entityPackage com.test.model --schema TEST*"); | ||
|
|
||
| DirectoryResource srcModelDir = javaProject.getProjectRoot() | ||
| .getChildDirectory("src/main/java/com/test/model"); | ||
| List<Resource<?>> generatedSrcFiles = srcModelDir.listResources(); | ||
| StringBuilder entities = new StringBuilder(); | ||
| for (Resource<?> resource : generatedSrcFiles) { | ||
| entities.append(resource.getName()).append(","); | ||
| } | ||
|
|
||
| Assert.assertEquals( | ||
| "Generation was wrong: 5 entities were expected - current: '" | ||
| + entities.toString() + "'", generatedSrcFiles.size(), | ||
| 5); | ||
|
|
||
| for (Resource<?> resource : generatedSrcFiles) { | ||
| Assert.assertTrue("Generation was wrong: " + resource.getName() | ||
| + " was not expected!", | ||
| resource.getName().startsWith("Client") | ||
| || resource.getName().startsWith("Customer") | ||
| || resource.getName().startsWith("Batatas")); | ||
|
|
||
| } | ||
|
|
||
| server.stop(); | ||
|
|
||
| } | ||
|
|
||
| @Test | ||
| public void testSchemaTest1H2() throws Exception { | ||
|
|
||
| Server server = Server.createTcpServer().start(); | ||
| Class.forName("org.h2.Driver"); | ||
| Connection conn = DriverManager | ||
| .getConnection("jdbc:h2:tcp://localhost/mem:test;USER=foo;PASSWORD=bar"); | ||
| createSampleSchemasAndTables(conn); | ||
|
|
||
| Project javaProject = initializeJavaProject(); | ||
| queueInputLines(""); | ||
| getShell().execute("project install-facet forge.spec.jpa"); | ||
| queueInputLines("bar"); | ||
| getShell().execute( | ||
| "generate-entities" + " --driver org.h2.Driver" | ||
| + " --pathToDriver " + resolveH2DriverJarPath() | ||
| + " --url jdbc:h2:tcp://localhost/mem:test" | ||
| + " --dialect org.hibernate.dialect.H2Dialect" | ||
| + " --user foo" // + " --password bar" | ||
| + " --entityPackage com.test.model --schema TEST2"); | ||
|
|
||
| DirectoryResource srcModelDir = javaProject.getProjectRoot() | ||
| .getChildDirectory("src/main/java/com/test/model"); | ||
| List<Resource<?>> generatedSrcFiles = srcModelDir.listResources(); | ||
|
|
||
| StringBuilder entities = new StringBuilder(); | ||
| for (Resource<?> resource : generatedSrcFiles) { | ||
| entities.append(resource.getName()).append(","); | ||
| } | ||
| Assert.assertEquals( | ||
| "Generation was wrong: 1 entity was expected - current: '" | ||
| + entities.toString() + "'", generatedSrcFiles.size(), | ||
| 1); | ||
| for (Resource<?> resource : generatedSrcFiles) { | ||
| Assert.assertTrue("Generation was wrong: " + resource.getName() | ||
| + " was not expected!", | ||
| resource.getName().equals("Batatas.java")); | ||
| } | ||
|
|
||
| server.stop(); | ||
|
|
||
| } | ||
|
|
||
| @Test | ||
| public void testTableNamesH2() throws Exception { | ||
|
|
||
| Server server = Server.createTcpServer().start(); | ||
| Class.forName("org.h2.Driver"); | ||
| Connection conn = DriverManager | ||
| .getConnection("jdbc:h2:tcp://localhost/mem:test;USER=foo;PASSWORD=bar"); | ||
| createSampleSchemasAndTables(conn); | ||
|
|
||
| Project javaProject = initializeJavaProject(); | ||
| queueInputLines(""); | ||
| getShell().execute("project install-facet forge.spec.jpa"); | ||
| queueInputLines("bar"); | ||
| getShell().execute( | ||
| "generate-entities" + " --driver org.h2.Driver" | ||
| + " --pathToDriver " + resolveH2DriverJarPath() | ||
| + " --url jdbc:h2:tcp://localhost/mem:test" | ||
| + " --dialect org.hibernate.dialect.H2Dialect" | ||
| + " --user foo" // + " --password bar" | ||
| + " --entityPackage com.test.model --table CUSTOMER*"); | ||
|
|
||
| DirectoryResource srcModelDir = javaProject.getProjectRoot() | ||
| .getChildDirectory("src/main/java/com/test/model"); | ||
| List<Resource<?>> generatedSrcFiles = srcModelDir.listResources(); | ||
| StringBuilder entities = new StringBuilder(); | ||
| for (Resource<?> resource : generatedSrcFiles) { | ||
| entities.append(resource.getName()).append(","); | ||
| } | ||
| Assert.assertEquals( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually the expected is the first parameter, after the message, but that's ok There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lol. It really doesnt matter, does it? Because mathematically, "equals" is
|
||
| "Generation was wrong: 2 entities were expected - current: '" | ||
| + entities.toString() + "'", generatedSrcFiles.size(), | ||
| 2); | ||
| for (Resource<?> resource : generatedSrcFiles) { | ||
| Assert.assertTrue("Generation was wrong: " + resource.getName() | ||
| + " was not expected!", | ||
| resource.getName().startsWith("Customer")); | ||
|
|
||
| } | ||
|
|
||
| server.stop(); | ||
|
|
||
| } | ||
|
|
||
| private void createSampleSchemasAndTables(Connection conn) | ||
| throws SQLException { | ||
| conn.createStatement().execute("CREATE SCHEMA TEST1"); | ||
| conn.createStatement().execute("CREATE SCHEMA TEST2"); | ||
| conn.createStatement().execute("CREATE SCHEMA ANOTHERTEST"); | ||
| conn.commit(); | ||
|
|
||
| conn.createStatement().execute( | ||
| "CREATE TABLE TEST1.customer1(" + " id INTEGER PRIMARY KEY," | ||
| + " first_name VARCHAR(256)," | ||
| + " last_name VARCHAR(256))"); | ||
| conn.createStatement().execute( | ||
| "CREATE TABLE TEST1.customer2(" + " id INTEGER PRIMARY KEY," | ||
| + " first_name VARCHAR(256)," | ||
| + " last_name VARCHAR(256))"); | ||
| conn.createStatement().execute( | ||
| "CREATE TABLE TEST1.client1(" + " id INTEGER PRIMARY KEY," | ||
| + " first_name VARCHAR(256)," | ||
| + " last_name VARCHAR(256))"); | ||
| conn.createStatement().execute( | ||
| "CREATE TABLE TEST1.client2(" + " id INTEGER PRIMARY KEY," | ||
| + " first_name VARCHAR(256)," | ||
| + " last_name VARCHAR(256))"); | ||
|
|
||
| conn.createStatement().execute( | ||
| "CREATE TABLE TEST2.batatas(" + " id INTEGER PRIMARY KEY," | ||
| + " first_name VARCHAR(256)," | ||
| + " last_name VARCHAR(256))"); | ||
|
|
||
| conn.createStatement().execute( | ||
| "CREATE TABLE ANOTHERTEST.foo(" + " id INTEGER PRIMARY KEY," | ||
| + " first_name VARCHAR(256)," | ||
| + " last_name VARCHAR(256))"); | ||
|
|
||
| conn.commit(); | ||
| } | ||
|
|
||
| @Test | ||
| public void test() throws Exception { | ||
| initializeJavaProject(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should also be ".*", "%" for all replacements to follow the syntax supported by hibernatetools - I see no reason why we would introduce a different syntax/semantics here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That one was my first implementation... although I do agree that on a
commandline the proper syntax should be the current one. Let me even mildly
suggest that hibernate tools shouldn't use the regular syntax approach
because it might trick users into using more complex expressions wich are
not actually supported.
Best regards
José
On 8 May 2013 08:24, "Max Rydahl Andersen" [email protected] wrote:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was deliberate to use .* since you can actually extend hibernate tools to support it; if you just want * behaviour then use % directly, no need to introduce yet another format IMO.