Hi Wonseok,
I am ok with keeping shouldIgnoreDatabaseException a property of
SchemaManager. If we do that, I think we to make a couple of changes:
1. Also make it a property of TableCreator and pass it in when we
instantiate a TableCreator. In the TableCreator methods check the local
property instead of the SchemaManager property.
2. Ensure SchemaManager does not have code that changes the value of
shouldIgnoreDatabaseException in the middle of a method and then changes
it back at the end of the method like this:
// ** store the variable to reset it later
boolean shouldIgnoreDatabaseException =
shouldIgnoreDatabaseException();
// ** change the value
setIgnoreDatabaseException(true);
try {
TableCreator tableCreator = getDefaultTableCreator();
tableCreator.createTables(session, this);
} catch (DatabaseException ex) {
// Ignore error
} finally {
// ** change the value back
setIgnoreDatabaseException(shouldIgnoreDatabaseException);
getSession().getSessionLog().setShouldLogExceptionStackTrace(shouldLogExceptionStackTrace);
}
Instead, allow the variable to be set by the user of SchemaManager and
then just use that setting.
In my opinion, those changes would make the code more readable.
What do you think?
-Tom
Wonseok Kim wrote:
> Hi Tom,
>
> Good point! My intention of introducing ignoreDatabaseException
> property is that it can be used more in the future.
> This time it may be appeared to make little sense. Frankly I'm
> thinking this property can be used to fix issue 1346 "some exceptions
> found by SchemaManager should be reported to the user"
> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1346
>
> As Marina commented there, with that flag, I think, it may be made to
> ignore or collect all exceptions thrown by all queries. I'm not
> thinking it deeply now and it should be discussed later, but it would
> require this kind of property. An parameter can be used instead as you
> suggest in that situation, but then we maybe should add the parameter
> to possibly all methods...
>
> I'm thinking also that shouldIgnoreDatabaseException() and
> setIgnoreDatabaseException() should be package-private access until it
> make more sense.
>
> What do you think?
> -Wonseok
>
> On 11/22/06, *Tom Ware* <tom.ware_at_oracle.com
> <mailto:tom.ware_at_oracle.com>> wrote:
>
> 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>
> > <mailto: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>
> > <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
> > >
> >
>
>