Adding support for schema/catalog/table arguments #16

sunnymoon · 2013-05-07T17:06:47Z

Implemented FORGEPLUGINS-112

gastaldi · 2013-05-07T17:16:34Z

.../java/org/hibernate/forge/generate/DefaultReverseEngineeringStrategyWithSchemaSelection.java

That means only one schema will be parsed ? What about the substring match ?

final String SCHEMA_HELP = "Schema pattern to include. 'PRODUCTION' for specific schema, 'PR.*' for substring match and '.*' for all (the default)";

It works for many schemas also... Just check the test with the schemas
TEST1 and TEST2...

JP

On Tue, May 7, 2013 at 6:16 PM, George Gastaldi [email protected]:

In
src/main/java/org/hibernate/forge/generate/DefaultReverseEngineeringStrategyWithSchemaSelection.java:

* @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) {

SchemaSelection schemaSelection = new SchemaSelection(catalogs,

schemas, tables);

schemaSelections = Collections.singletonList(schemaSelection);

That means only one schema will be parsed ? What about the substring match
?

final String SCHEMA_HELP = "Schema pattern to include. 'PRODUCTION' for specific schema, 'PR.' for substring match and '.' for all (the default)";

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/16/files#r4117932
.

[image: Linkare TI - http://www.linkare.com]

» José Pedro Pereira | CTO
» Email/Gtalk: [email protected]
» M: +351 918 140 992 | T: +351 213 590 623 | F: +351 213 590 624
**» Web Site: http://www.linkare.com

Not needed. Just force push in your same branch and the pull request will be automatically updated

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?

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 ?

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 %.

Yes., that would be much better ! Thank you !

Great job on that!

gastaldi · 2013-05-07T17:36:21Z

Hi @sunnymoon,

I reviewed your pull request and added some comments on it. Let me know if you have any further questions, I'll be glad to help.

…enting a ReverseEngineeringStrategy that uses SchemaSelection - FORGEPLUGINS-112

gastaldi · 2013-05-07T18:21:04Z

src/test/java/org/hibernate/forge/generate/GenerateEntitiesPluginTest.java

Actually the expected is the first parameter, after the message, but that's ok

Lol. It really doesnt matter, does it? Because mathematically, "equals" is
commutative and transitive, right? ;)
On 7 May 2013 19:21, "George Gastaldi" [email protected] wrote:

In
src/test/java/org/hibernate/forge/generate/GenerateEntitiesPluginTest.java:

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(

Actually the expected is the first parameter, after the message, but
that's ok

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/16/files#r4119477
.

koentsje · 2013-05-07T18:27:01Z

Wow, this is awesome! Man do I love open source ;-)

gastaldi · 2013-05-07T18:35:51Z

I think it's ok now, @koentsje you can merge it anytime 👍

Thanks @sunnymoon !

maxandersen · 2013-05-07T18:39:02Z

btw. FORGEPLUGINS-112 should not happen if hibernate tools autodetect and uses the OracleMetaDataDialect...something must be messing with that.

sunnymoon · 2013-05-07T18:39:12Z

Pleasure.
On 7 May 2013 19:35, "George Gastaldi" [email protected] wrote:

I think it's ok now, @koentsje https://github.com/koentsje you can
merge it anytime [image: 👍]

Thanks @sunnymoon https://github.com/sunnymoon !

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/16#issuecomment-17561282
.

maxandersen · 2013-05-07T18:48:00Z

src/main/java/org/hibernate/forge/generate/GenerateEntitiesPlugin.java

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.

Inline....

On 7 May 2013 19:48, "Max Rydahl Andersen" [email protected] wrote:

In src/main/java/org/hibernate/forge/generate/GenerateEntitiesPlugin.java:

@@ -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);

this looks a tad problematic to me.

It actually doesn't seem at all problematic, since if not specified, the
defaults actually will be

Catalog=%
Schema=%
Table=%

This means all schemas will be scanned by default no matter what
hibernate tools metadatadialects states.

As I've seen from the code, this is not true, as the
DatabaseMetadata.getTables methods are then filtered immediately by these
limitations.

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.

I think that this would require a much deeper refactoring of the forge
plugin... probably then you would have to choose between the direct
arguments catalog/schema/table and use only a revenge.xml file...

This strategy should only be enable if the user has explicitly set one of
catalogs, schemas or tables parameters.

This is totally irrelevant, as if the schema selection is null or the
defaults as defined by this implementation the behaviour inside hibernate
tools code is actually the same...

—
Reply to this email directly or view it on GitHub.

maxandersen · 2013-05-07T18:48:58Z

I think we should get this fixed better before applying - the idea is great though!

maxandersen · 2013-05-08T07:24:55Z

.../java/org/hibernate/forge/generate/DefaultReverseEngineeringStrategyWithSchemaSelection.java

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.

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:

In
src/main/java/org/hibernate/forge/generate/DefaultReverseEngineeringStrategyWithSchemaSelection.java:

* @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, "*", "%");

this should also be ".*", "%" to follow the syntax supported by
hibernatetools - I see no reason why we would introduce a different one
here.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/16/files#r4129427
.

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.

gastaldi reviewed May 7, 2013
View reviewed changes

Adding support for schema/catalog/table arguments correctly by implem…

595bd49

…enting a ReverseEngineeringStrategy that uses SchemaSelection - FORGEPLUGINS-112

gastaldi reviewed May 7, 2013
View reviewed changes

maxandersen reviewed May 7, 2013
View reviewed changes

maxandersen reviewed May 8, 2013
View reviewed changes

Uh oh!

Adding support for schema/catalog/table arguments #16

Are you sure you want to change the base?

Adding support for schema/catalog/table arguments #16

Conversation

sunnymoon commented May 7, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gastaldi commented May 7, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koentsje commented May 7, 2013

Uh oh!

gastaldi commented May 7, 2013

Uh oh!

maxandersen commented May 7, 2013

Uh oh!

sunnymoon commented May 7, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxandersen commented May 7, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants