Hi Wonsoek,
Answer inline:
Wonseok Kim wrote:
> Hi Tom,
> Please see my comments inline.
>
> On 11/2/06, *Tom Ware* <tom.ware_at_oracle.com
> <mailto:tom.ware_at_oracle.com>> wrote:
>
> Hi Wonsoek,
>
> Answers inline.
>
> <snip>
>
> >
> > - Will the addForeignKeyConstraint() method work if the foreign
> > keys do
> > not map exactly to the primary keys of the target
> table? Will it work
> > if they, instead map to some candidate keys, or if they map to
> > only part
> > of the primary key table? Perhaps I am missing something
> here and I
> > just need some explantation.
> >
> >
> > Foreign key and primary key list come from
> ForeignReferenceMapping. So
> > I assumed that they are correct - fkFields map exactly to
> pkFields and
> > pkFields contain all composite primary keys of the target table.
> >
> > Is is possible that they are not? Is there any case that composite
> > foreign keys do not map to the primary keys of taget table? Or the
> > part of composite primary keys are referenced? I couldn't think of
> > such a case. Could you tell me an example if it's possible?
> >
> > If a user specify incorrect join columns, it might lead to such a
> > situation. Validation may be required, but I think schema
> generation
> > is not good place to do it, because in that case
> > ForeignReferenceMappings are already invalid and the problem will
> > occur in runtime too if schema generation is not used. Without
> > validation an incorrect ForeignKeyConstraint may cause underlying
> > DatabaseException.
> >
> > What do you mean by candidate keys? The method will be called in
> > postInitTableSchema() phase after initTableSchema() construct
> primary
> > keys. So it assume that targetTableDefinition contain complete
> primary
> > keys.
>
> Here's an example on Oracle. I am wondering if we can support
> creating
> constraints on tables that are defined as follows?
>
> CREATE TABLE A_TABLE (ID INTEGER NOT NULL, B_NAME VARCHAR(20), B_TYPE
> INTEGER, PRIMARY KEY (ID))
>
> CREATE TABLE B_TABLE (ID INTEGER NOT NULL, NAME VARCHAR(20), TYPE
> INTEGER, PRIMARY KEY (ID), CONSTRAINT NAME_TYPE UNIQUE (NAME, TYPE))
>
> In this case, the combination of NAME and TYPE is a candidate
> key. The
> constraint would look something like the following:
>
> ALTER TABLE A_TABLE ADD CONSTRAINT B_CONSTRAINT FOREIGN KEY (B_NAME,
> B_TYPE) REFERENCES B_TABLE (NAME, TYPE)
>
> I realize that JPA probably will not currently create the Unique
> Constraint on TABLE_B, but I think it should be possible to create it
> using base-TopLink API. (TableDefinition.addUniqueKeyConstraint())
>
> Again, Perhaps I am missing something fundmental, but if possible I
> would like to avoid disallowing creating tables of that kind.
>
>
> In that case, TableDefinition.addForeignKeyConstraint() can be used.
> The method we are discussing is default foreign key constraints
> generator based on ForeignReferenceMappings. Can
> ForeignReferenceMapping reference candidate keys instead of primary
> keys of the target entity?
Yes, ForeignReferenceMapping subclasses (for instance OneToOneMapping)
can use either candidate keys or primary keys. As long as your code
change works with a ForeignReferenceMapping that uses either, it should
be ok.
-Tom
>
> >
> >
> > > * Deprecated FieldDefinition.foreginKeyFieldName .
> > > * When making ForeignKeyConstraint the key order is
> reordered as the
> > > primary key order of target table - if the order is different,
> > > databases don't accept it.
> > > * Changed the data structure of foreign keys in
> TableDefinition
> > from
> > > Vector to Map to avoid same foreign key constraint being
> added -
> > this
> > > occurs for bidirectional relationships.
> >
> > I am concerned about this change. It does not appear to be
> completely
> > implemented. TableDefinition.getForeignKeys() and
> > TableDefinition.setForeignKeys() still operate on
> vectors. I think we
> > need to choose. Either this data structure is dealt with as a
> > Vector or
> > as a HashMap.
> >
> >
> > First of all I would like to know whether getForeignKeys() and
> > setForeignKeys() are PUBLIC or INTERNAL method. I thought they are
> > PUBLIC methods so I didn't modify their signature because they
> may be
> > being used by existing code. If they are INTERNAL methods, I will
> > modify their signature to use Map. How about others such as
> > get/setFields() and get/setUniqueKeys()? Are they all INTERNAL
> methods?
>
>
> Good point about the public use of these methods. Go ahead and change
> the methods as you have suggested.
>
>
> Do you mean I can treat them as INTERNAL methods or not? Actually I
> don't see any reason they are PUBLIC methods, because there is already
> addForeignKeyConstraint(). If required, we can add "Collection
> getForeignKeyConstraints()" for public getter.
I think you should leave your code the way it was when you sent it. You
made a good argument about these methods being accessible for the
public. Although there are APIs available that will accomplish the same
thing, for backwards compatibility, I think it is better not to change
the signatures. Perhaps a comment that indicates why the setter takes a
Vector and converts it to a map would be a good idea.
We could add setForeignKeyConstraint(Map) and Map
getForeignKeyConstraint(), but I am not sure they would really get used.
-Tom
>
> -Wonseok
>
> -Tom
>
> >
> > Thanks,
> > -Wonseok
> >
> > >
> > > This contains also a fix for issue 1392 - Wrong primary key
> > constraint
> > > if one-to-one and many-to-many relationships have same
> join column
> > > name. It is included because it is simple fix. See
> > > DefaultTableGenerator.resolveDatabaseField().
> > > https://glassfish.dev.java.net/issues/show_bug.cgi?id=1392
> > >
> > > For tests,
> > > I would like to add separate models for ddl generation
> purpose.
> > > * Added a new models in new
> > >
> oracle.toplink.essentials.testing.models.cmp3.ddlgeneration package.
> > > * Added a new test DDLGenerationJUnitTestSuite which now
> contains a
> > > test for issue 1392.
> > > * DDL generation doesn't throw exceptions, so I checked logs
> > manually.
> > > * Tested single field foreign key constraint by running ddl
> > generation
> > > for entity-persistence-tests models. There was no failure
> during ddl
> > > generation.
> > >
> > > Updating TableCreators to use ForeignKeyConstraint object
> instead of
> > > deprecated foreignKeyFieldName will be done after review,
> how about
> > > making this another enhancement issue?
> > >
> > > Regards,
> > > -Wonseok
> > >
> > > On 10/31/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,
> > >
> > > Sorry for not replying sooner. I was out of the office
> > for the last
> > > few days.
> > >
> > > First, a brief explanation:
> > >
> > > There are some historical reasons in TopLink why both
> > > foreignKeyFieldName and the ForeignKeyConstraint object
> > exist. The
> > > ForeignKeyConstraintObject is the newer way of doing
> things,
> > but
> > > due to
> > > backwards compatibility requirements we have chosen to
> keep both
> > > ways of
> > > doing things. In the past, some of the tooling that
> shipped
> > with
> > > TopLink relied on the foreignKeyFieldName. I am in the
> > process of
> > > determining if that is still the case and I will let you
> > know the
> > > result.
> > >
> > > I think the ideal approach to this problem is as
> follows:
> > >
> > > 1. deprecate setForeignKeyFieldName and
> > getForeignKeyFieldName on
> > > FieldDefinition and update DefaultTableGenerator to
> make use of
> > > ForeignKeyConstraint instead.
> > > 2. Make the changes you suggest below to allow both
> types of
> > foreign
> > > keys to be used TableDefinition.buildForeignFieldTypes ().
> > > 3. Update the entity-persistence-tests to use the
> non-deprecated
> > > API. I
> > > realize this is a bit tedious, but I think we need to make
> > that change
> > > in order to completely fix the issue.
> > >
> > > I assume there are more changes required to fix
> issue 1072.
> > >
> > > As for the use of Generics in the schema generation
> code,
> > the simple
> > > answer is that you may now use generics in that code. For
> > those that
> > > have been working on the TopLink Essentials code for some
> > time, a
> > > bit of
> > > further explanation is required. Until fairly
> recently, we
> > only
> > > allowed
> > > Java 5-specific code in a few of our packages due to a
> > requirement to
> > > support core TopLink functionality on JDK 1.4. That
> > requirement no
> > > longer exists and as a result, Java 5 code can now
> appear in
> > our core
> > > packages.
> > >
> > > -Tom
> > >
> > > Wonseok Kim wrote:
> > >
> > > > TopLink Team, please shed light on this so I can go
> ahead.
> > > > I added some comments in the below,
> > > >
> > > > On 10/26/06, *Wonseok Kim* < guruwons_at_gmail.com
> <mailto:guruwons_at_gmail.com>
> > <mailto: guruwons_at_gmail.com <mailto:guruwons_at_gmail.com>>
> > > <mailto:guruwons_at_gmail.com <mailto:guruwons_at_gmail.com>
> <mailto:guruwons_at_gmail.com <mailto:guruwons_at_gmail.com> >>
> > > > <mailto: guruwons_at_gmail.com
> <mailto:guruwons_at_gmail.com> <mailto:guruwons_at_gmail.com
> <mailto:guruwons_at_gmail.com>>
> > <mailto: guruwons_at_gmail.com <mailto:guruwons_at_gmail.com>
> <mailto:guruwons_at_gmail.com <mailto:guruwons_at_gmail.com>>>>> wrote:
> > > >
> > > > Hi Tom,
> > > >
> > > > I'm trying to fix issue 1072 - ddl generation of
> > composite
> > > foreign
> > > > key constraints.
> > > > I think the ForeignKeyConstraints class is the right
> > one to
> > > use if
> > > > composite foreign key, but there is another
> foreign key
> > > setting in
> > > > FieldDefinition.
> > > >
> > > > public class FieldDefinition implements
> Serializable,
> > > Cloneable {
> > > > ...
> > > > protected String
> > foreignKeyFieldName;//fully-qualified
> > > foreign
> > > > key field name
> > > >
> > > > But current code shows that if above
> > foreignKeyFieldName is set,
> > > > it ignores the TableDefinition.foreignKeys
> > > > (ForeignKeyConstraints). See below.
> > > >
> > > > TableDefinition:
> > > > private void buildForeignFieldTypes(final
> > AbstractSession
> > > > session) {
> > > > Hashtable fieldTypes =
> > > session.getPlatform().getClassTypes();
> > > > FieldDefinition field = null;
> > > >
> > > > Vector foreignKeysClone =
> > > > (Vector)getForeignKeys().clone();
> > > > setForeignKeys(new Vector());
> > > > for (Enumeration enumtr =
> getFields().elements();
> > > > enumtr.hasMoreElements ();) {
> > > > field =
> (FieldDefinition)enumtr.nextElement();
> > > > if (field.getForeignKeyFieldName ()
> != null) {
> > > >
> > > >
> > addForeignKeyConstraint(buildForeignKeyConstraint(field, this,
> > > > session.getPlatform()));
> > > >
> > > > }
> > > > if (field.getType() == null) {
> > > > field.setType
> ((Class)fieldTypes.get(
> > > > field.getTypeName ()));
> > > > if ( field.getType() != null) {
> > > > field.setTypeName(null);
> > > > }
> > > > }
> > > > }
> > > >
> > > > /* bug #2997188 constraints not getting
> > generated when
> > > > creating/replacing tables via TableCreator
> > > > had to add the next few lines instead of
> > removing the
> > > > above code for backwards compatibility */
> > > > if (getForeignKeys().isEmpty()) {
> > > > //if foreignKeys is empty then we
> know we are
> > > not in 2.5
> > > > setForeignKeys(foreignKeysClone);
> > > > }
> > > > }
> > > >
> > > > I'm curious, should we use either
> foreignKeyFieldName or
> > > > foreignKeys (ForeignKeyConstraints)? We could
> not mix
> > both of
> > > > them? Is there any reason for not allowing this?
> > > > entity-persistence-tests table creators are
> using only
> > > > foreignKeyFieldName now, but If I would like to add
> > composite
> > > > foreign key constraints, how can I mix them?
> > > >
> > > >
> > > > If there is no problem, I would like to support for both
> > > > ForeignKeyConstraint object and foreignKeyFieldName by
> > removing
> > > > setForeignKeys(new Vector()) line and below lines.
> > > >
> > > > /* bug #2997188 constraints not getting
> generated
> > when
> > > > creating/replacing tables via TableCreator
> > > > had to add the next few lines instead of
> > removing the
> > > above
> > > > code for backwards compatibility */
> > > > if (getForeignKeys().isEmpty()) {
> > > > //if foreignKeys is empty then we know
> we are
> > not in 2.5
> > > > setForeignKeys(foreignKeysClone);
> > > > }
> > > >
> > > > Then we can use both
> FieldDefinition.foreignKeyFieldName
> > (for simple
> > > > foreign key) and addForeignKeyConstraint (for composite
> > foreign
> > > key).
> > > > What do you think?
> > > >
> > > > Also, I don't know there is any policy about
> code, but
> > could
> > > I use
> > > > Java SE 5 Generics in schema generation codes? This
> > makes us
> > > avoid
> > > > type-mismatch mistakes and easy to read code.
> > > >
> > > > Regards,
> > > > - Wonseok
> > > >
> > > >
> > >
> > >
> >
> >
>
>
--
Tom Ware
Principal Software Engineer
Oracle Canada Inc.
Direct: (613) 783-4598
Email: tom.ware_at_oracle.com