Tom,
Please review my second fix.
As your comments I modified addForeignKeyConstraint() to consider candidate
keys and tested the case with newly-added models.
Here is the summary which I modified after the previous fix.
* When making ForeignKeyConstraint the keys are reordered using primary keys
or candidate keys of the target table. If target keys are incorrect(neither
primary keys nor unique keys), it doesn't reorder keys and underlying
DatabaseException will be thrown.
* Fixed issue 1419 - Unique constraints should be made before foreign key
constraints. In the test of relationships using candidate(unique) keys, I
found that adding foreign key constraints failed because unique constraints
are made late.
I fixed 1419 also because it's required to fix this(1072).
New tests are added.
* New models and tests are in ~.testing.models.cmp3.ddlgeneration and
~.testing.tests.ddlgeneration
* New models have several kinds of composite foreign key relationships
including one using candidate keys.
All unit tests have passed.
Thanks
-Wonseok
On 11/3/06, Tom Ware <tom.ware_at_oracle.com> wrote:
>
> Hi Wonsoek,
>
> I think your mapping from Source to Target should work (with the
> assumption that ddl generation is successful)
>
> I agree with your plan for getForeignKeys()/setForeignKeys().
>
> -Tom
>
> Wonseok Kim wrote:
>
> > Tom,
> >
> > I will try an experiment of ForeignReferenceMapping which references
> > unique keys.
> > Probably, in the following code if CODE1, CODE2 are unique keys but
> > not primary keys, it could be such a case.
> > Will it work well in current runtime code (supposing ddl generation
> > were successful)?
> >
> > public class Source {
> >
> > @OneToOne
> > @JoinColumns({
> > @JoinColumn(name="TARGET_CODE1", referencedColumnName =
> "CODE1"),
> > @JoinColumn(name="TARGET_CODE2", referencedColumnName = "CODE2")
> > })
> > Target target;
> > ...
> > }
> >
> > @Table(name="TARGET",
> > uniqueConstraints={_at_UniqueConstraint(columnNames={"CODE1", "CODE2"})})
> > public class Target {
> > @Id
> > Long id;
> >
> > @Column(name="CODE1", nullable = false)
> > String code1;
> >
> > @Column(name="CODE2", nullable = false)
> > String code2;
> > ...
> > }
> >
> > As to getForeignKeys()/setForeignKeys(), I will keep the signature and
> > add a javadoc which says it's PUBLIC.
> > But I won't make new getForeignKeyMap()/getForeignKeyMap() methods
> > public, because it exposes the internal structure and make it harder
> > to change the structure later.
> >
> > Thanks
> > -Wonseok
> >
> > On 11/2/06, *Tom Ware* <tom.ware_at_oracle.com
> > <mailto:tom.ware_at_oracle.com>> wrote:
> >
> > 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>
> > > <mailto: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>>>
> > > > > <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 <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>> >>
> > > > > > <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 <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
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > >
> > > >
> > >
> > >
> >
>