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