Hi Wonseok,
I think the way the code works is good. I just have concern.
I am wondering if shouldIgnoreDatabaseException really makes sense as
a property of SchemaManager? It seems to be only used in 3 methods in
TableCreator and never in SchemaManager. In fact, it seems to only ever
be temporarily set when those methods are called and then returned to
whatever value it was before in a finally block. That indicates to me
that it might make more sense as a method argument. Would it make sense
to add additional implementations of the the affected method. Here is
an example for dropTables():
/**
* Drop the tables from the database. By default, we will allow
database exceptions to be thrown.
*/
public void
dropTables(oracle.toplink.essentials.sessions.DatabaseSession session,
SchemaManager schemaManager) {
dropTables(session, schemaManager, false);
}
/**
* Drop the tables from the database.
*/
public void
dropTables(oracle.toplink.essentials.sessions.DatabaseSession session,
SchemaManager schemaManager, boolean shouldIgnoreDatabaseException) {
for (Enumeration enumtr = getTableDefinitions().elements();
enumtr.hasMoreElements();) {
schemaManager.buildFieldTypes((TableDefinition)enumtr.nextElement());
}
for (Enumeration enumtr = getTableDefinitions().elements();
enumtr.hasMoreElements();) {
try {
schemaManager.dropConstraints((TableDefinition)enumtr.nextElement());
} catch
(oracle.toplink.essentials.exceptions.DatabaseException dbE) {
//ignore
}
}
String sequenceTableName = getSequenceTableName(session);
for (Enumeration enumtr = getTableDefinitions().elements();
enumtr.hasMoreElements();) {
// Must not create sequence table as done in createSequences.
TableDefinition table = (TableDefinition)enumtr.nextElement();
if (!table.getName().equals(sequenceTableName)) {
try {
schemaManager.dropObject(table);
} catch (DatabaseException ex) {
if (!shouldIgnoreDatabaseException) {
throw ex;
}
}
}
}
}
-Tom
Wonseok Kim wrote:
> Tom, Thanks for reviewing the fix.
>
> Yes, you're right. createDefaultTables() code is duplicate mostly. So
> I fixed more as your suggestion.
> Please review this(attached) fix.
>
> Additional Summary of this fix:
> * SchemaManager.createDefaultTables() now just calls
> TableCreator.createTables() with ignoreDatabaseException flag on.
> * Introduce ignoreDatabaseException flag in SchemaManager. This flag
> is used by createDefaultTables() and replaceDefaultTables() to ignore
> exceptions and continue to execute DDLs.
> * This fixes also the issue that DDL generation with
> "drop-and-create-tables" stop if there is an error in executing CREATE
> TABLE or ALTER TABLE xxx ADD CONSTRAINT.
>
> Thanks,
> -Wonseok
>
> On 11/22/06, *Tom Ware* <tom.ware_at_oracle.com
> <mailto:tom.ware_at_oracle.com>> wrote:
>
> Hi Wonseok,
>
> My initial instinct for fixing this bug would be to try to make
> SchemaManager.createDefaultTables() make use of the code in
> TableCreator.createTables(). That appears to be what we are doing
> for
> table replacement. It looks like there is some code that is fairly
> close to duplicated. Is it possible to extend the
> TableCreator.createTables() functionality a bit and allow this bug
> to be
> fixed and backwards compatibility to be maintained?
>
> -Tom
>
> Wonseok Kim wrote:
>
> > Hi Tom,
> >
> > Please review this fix(attached) of the following issue.
> > https://glassfish.dev.java.net/issues/show_bug.cgi?id=1365
> <https://glassfish.dev.java.net/issues/show_bug.cgi?id=1365>
> >
> > Summary of this fix:
> > * Add creating constraints logic to
> SchemaManager.createDefaultTables()
> > * Reuse the default TableCreator instance in SchemaManager. If
> > toplink.ddl-generation.output-mode is 'both' or run in SE mode, same
> > SchemaManager was called twice for both files and database output.
> > Each call created repetitively the same default TableCreator which
> > takes time in initializing - processing all ClassDescriptors.
> >
> > Tested with entity-persistence-tests and quick look tests.
> >
> > Here is the diff:
> > Index:
> >
> src/java/oracle/toplink/essentials/tools/schemaframework/SchemaManager.java
> >
> > ===================================================================
> > RCS file:
> >
> /cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/tools/schemaframework/SchemaManager.java,v
> > retrieving revision 1.12
> > diff -c -w -r1.12 SchemaManager.java
> > ***
> >
> src/java/oracle/toplink/essentials/tools/schemaframework/SchemaManager.java
> > 7 Nov 2006 20:35:26 -0000 1.12
> > ---
> >
> src/java/oracle/toplink/essentials/tools/schemaframework/SchemaManager.java
>
> > 21 Nov 2006 07:13:55 -0000
> > ***************
> > *** 56,61 ****
> > --- 56,62 ----
> > protected Writer createSchemaWriter;
> > protected Writer dropSchemaWriter;
> > protected boolean createSQLFiles = true; //if true, schema
> > writer will add terminator string.
> > + protected TableCreator defaultTableCreator;
> >
> > public SchemaManager(DatabaseSessionImpl session) {
> > this.session = session;
> > ***************
> > *** 778,805 ****
> > }
> >
> > /**
> > * Create the default table schema for the TopLink project this
> > session associated with.
> > */
> > public void createDefaultTables() {
> > //Create each table w/o throwing exception and/or exit if
> > some of them are already existed in the db.
> > //If a table is already existed, skip the creation.
> > - Iterator tblDefIter = new
> >
> DefaultTableGenerator(session.getProject()).generateDefaultTableCreator().getTableDefinitions().iterator();
> >
> > ! while (tblDefIter.hasNext()) {
> > ! TableDefinition tblDef =
> (TableDefinition)tblDefIter.next();
> > boolean shouldLogExceptionStackTrace =
> > getSession().getSessionLog().shouldLogExceptionStackTrace();
> >
> > getSession().getSessionLog().setShouldLogExceptionStackTrace(false);
> >
> > try {
> > createObject(tblDef);
> > session.getSessionLog().log(SessionLog.FINEST,
> > "default_tables_created", tblDef.getFullName());
> > } catch (DatabaseException exception) {
> > // Ignore the exception, table already created
> > session.getSessionLog().log(SessionLog.FINEST,
> > "default_tables_already_existed", tblDef.getFullName ());
> > } finally {
> >
> >
> getSession().getSessionLog().setShouldLogExceptionStackTrace(shouldLogExceptionStackTrace);
> > }
> > - }
> >
> > //create sequence table
> > createSequences();
> > --- 779,840 ----
> > }
> >
> > /**
> > + * Construct the default TableCreator.
> > + * If the default TableCreator is already created, just
> returns it.
> > + */
> > + protected TableCreator getDefaultTableCreator() {
> > + if(defaultTableCreator == null) {
> > + defaultTableCreator = new
> > DefaultTableGenerator(session.getProject
> ()).generateDefaultTableCreator();
> >
> > + }
> > + return defaultTableCreator;
> > + }
> > +
> > + /**
> > * Create the default table schema for the TopLink project
> this
> > session associated with.
> > */
> > public void createDefaultTables() {
> > //Create each table w/o throwing exception and/or exit if
> > some of them are already existed in the db.
> > //If a table is already existed, skip the creation.
> >
> > ! TableCreator tableCreator = getDefaultTableCreator();
> > ! Vector tableDefinitions =
> tableCreator.getTableDefinitions ();
> > !
> > boolean shouldLogExceptionStackTrace =
> > getSession().getSessionLog().shouldLogExceptionStackTrace();
> >
> >
> getSession().getSessionLog().setShouldLogExceptionStackTrace(false);
> >
> > try {
> > + for (Iterator tblDefIter =
> tableDefinitions.iterator();
> > tblDefIter.hasNext();) {
> > + TableDefinition tblDef =
> > (TableDefinition)tblDefIter.next();
> > +
> > + try {
> > createObject(tblDef);
> > session.getSessionLog().log(SessionLog.FINEST,
> > "default_tables_created", tblDef.getFullName ());
> > } catch (DatabaseException exception) {
> > // Ignore the exception, table already created
> >
> session.getSessionLog().log(SessionLog.FINEST ,
> > "default_tables_already_existed", tblDef.getFullName());
> > + }
> > + }
> > +
> > + // Unique constraints should be generated before
> foreign
> > key constraints,
> > + // because foreign key constraints can reference
> unique
> > constraints
> > + for (Iterator tblDefIter =
> tableDefinitions.iterator();
> > tblDefIter.hasNext ();) {
> > + try {
> > +
> > createUniqueConstraints((TableDefinition)tblDefIter.next());
> > + } catch(DatabaseException exception) {
> > + // Ignore the exception
> > + }
> > + }
> > +
> > + for (Iterator tblDefIter =
> tableDefinitions.iterator();
> > tblDefIter.hasNext();) {
> > + try {
> > +
> > createForeignConstraints((TableDefinition)tblDefIter.next());
> > + } catch(DatabaseException exception) {
> > + // Ignore the exception
> > + }
> > + }
> > } finally {
> >
> >
> getSession().getSessionLog().setShouldLogExceptionStackTrace(shouldLogExceptionStackTrace);
>
> > }
> >
> > //create sequence table
> > createSequences();
> > ***************
> > *** 813,820 ****
> >
> >
> getSession().getSessionLog().setShouldLogExceptionStackTrace(false);
> >
> > try {
> > ! DefaultTableGenerator gen = new DefaultTableGenerator(
> > session.getProject());
> > !
> gen.generateDefaultTableCreator().replaceTables(session,
> > this);
> > } catch (DatabaseException exception) {
> > // Ignore error
> > } finally {
> > --- 848,855 ----
> >
> >
> getSession().getSessionLog().setShouldLogExceptionStackTrace(false);
> >
> > try {
> > ! TableCreator tableCreator = getDefaultTableCreator();
> > ! tableCreator.replaceTables(session, this);
> > } catch (DatabaseException exception) {
> > // Ignore error
> > } finally {
> > ***************
> > *** 830,837 ****
> >
> > getSession().getSessionLog().setShouldLogExceptionStackTrace(false);
> >
> > try {
> > ! DefaultTableGenerator gen = new
> > DefaultTableGenerator(session.getProject());
> > ! gen.generateDefaultTableCreator
> > ().replaceTables(session, this, keepSequenceTables);
> > } catch (DatabaseException exception) {
> > // Ignore error
> > } finally {
> > --- 865,872 ----
> >
> >
> getSession().getSessionLog().setShouldLogExceptionStackTrace(false);
> >
> > try {
> > ! TableCreator tableCreator = getDefaultTableCreator();
> > ! tableCreator.replaceTables(session, this,
> > keepSequenceTables);
> > } catch (DatabaseException exception) {
> > // Ignore error
> > } finally {
> >
> > Thanks,
> > -Wonseok
> >
>