From: Tom Ware <tom.ware_at_oracle.com>
Date: Thu, 23 Nov 2006 09:40:36 -0500

Hi Wonseok,

  The changes look good.

-Tom

Wonseok Kim wrote:

> Hi Tom,
>
> I agree, in current fix it is natural that TableCreator has the
> ignoreDatabaseException property. SchemaManager is not using it
> anywhere now so I think SchemaManager don't need to have it if
> TableCreator have the property. Thus, I moved the property from
> SchemaManager to TableCreator.
>
> I attached the modified version, please let me know if there is any
> other concern.
>
> Thanks,
> -Wonseok
>
> On 11/23/06, *Tom Ware* < tom.ware_at_oracle.com
> <mailto:tom.ware_at_oracle.com>> wrote:
>
> 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>
> > <mailto: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>>
> > > <mailto: 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
> > > >
> > >
> >
> >
>
>