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> 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>> 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.DatabaseSessionsession,
> > SchemaManager schemaManager) {
> > dropTables(session, schemaManager, false);
> > }
> >
> > /**
> > * Drop the tables from the database.
> > */
> > public void
> > dropTables(oracle.toplink.essentials.sessions.DatabaseSessionsession,
> > 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
> > > >
> > >
> >
> >
>