Hi Wonsoek,
Just a couple of comments:
- TableDefinition, line 120 - our coding convention requires braces
around all blocks even if they are only one line
- TableDefinition.addForeignKeyConstraint(), do you think we should warn
users or throw an exception if they are adding a constraint that already
exists?
Other than that, 'looks good.
Thanks,
Tom
Wonseok Kim wrote:
> 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
> <mailto: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>
> > <mailto: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>>
> > > <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 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
> <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>>>>
> > > > > <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 <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>>> >>
> > > > > > <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 <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