Hi Wonseok,
Sorry about the long turn around.
The changes look good to me. Go ahead and check-in.
-Tom
Wonseok Kim wrote:
> Hi Tom,
>
> Please review my fix (attached) for the issue 1391.
>
> Here is the summary of this fix:
> 1. The ObjectBuilder.mappingsByField of Aggregate class descriptor now
> contains the mapping for the primary key join (secondary) fields
> * This also fixes the GF#1153 elegantly, so removed the
> previously-added fix code for 1153 in ObjectBuilder
> * The types of primary key join DatabaseFields are set properly now
> in ClassDescriptor post-initialize phase.
> 2. DefaultTableGenerator
> * For primary key join fields, the type of FieldDefinition is copied
> from the original primary key field to ensure that same type of field
> is generated
> * Contains some refactoring to remove duplicate code
> 3. APPLY GENERICS to collection members of ClassDescriptor,
> ObjectBuilder, DatabaseMapping(and its subclasses) and
> DefaultTableGenerator for code readability
>
> Please see ClassDescriptor, ObjectBuilder, AggregateObjectMapping,
> DefaultTableGenerator classes for this fix and other classes are
> updated just to apply generics.
>
> Also added some unit tests for GF#1153 and GF#1391. This fix is tested
> with entity-persistence-tests.
>
> Thanks!
> - Wonseok
>
> On 12/8/06, *Tom Ware * <tom.ware_at_oracle.com
> <mailto:tom.ware_at_oracle.com>> wrote:
>
> Hi Wonseok,
>
> I would definitely take a look at how the AggregateObjectMapping's
> reference descriptor gets initialized.
> (AggregateObjectMapping.initialize() is a good start) I suspect
> that if
> any information about fields is incorrect or missing, that is the
> best
> place to start looking for a fix.
>
> I'd also look at how the TableCreator for DDL_TARGET gets generated.
> Confirm it is looking at the right DatabaseField (and isn't just
> working
> because the join fields have the same name) - it probably is, but I'd
> confirm. When that is confirmed, it should be a matter of determining
> how those fields get their type and ensuring it comes from the
> right place.
>
> I'd be careful about introducing any new knowledge about Primary
> Key
> information to AggregateObjectMapping or it's reference descriptor
> unless absolutely necessary.
>
> -Tom
>
> Wonseok Kim wrote:
>
> > Hi Tom,
> >
> > On 12/7/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,
> >
> > PR 381 is an issue that was fixed some time in the mid
> '90s when
> > TopLink was still owned by The Object People (2 companies
> ago), so
> > I am
> > a little short on detail about the actual bug. Here is what I
> > understand about the isAggregateMapping check. We do not
> set the type
> > for AggregateMappings in ClassDescriptor because that job is
> meant
> > to be
> > done in the Descriptor representing the actual aggregate
> > object. Note:
> > AggregateDescriptors are treated differently by
> TopLink. The original
> > descriptor stored in the project is cloned for each time the
> Aggregate
> > is used in a mapping and that clone is the actual descriptor
> that is
> > used (instead of the initial instance)
> >
> >
> > Then, I think, there is no reason that the 'type' values of
> > AggregateObjectMapping fields should be remained null. Now I need to
> > investigate how to set the 'type' values correctly.
> >
> > My initial thoughts on your suggested fix is that although
> it would
> > likely work, it is more of a fix of the symptoms then a fix of
> > root of
> > the problem.
> >
> > I believe the real bug is that we do not get the correct value
> > for the
> > type at initialization time and that the DDL generation
> simply shows
> > that bug.
> >
> > If I were investigating this bug, I would see if it was
> possible to
> > get the type right during the initialization process. I
> would take a
> > look at how Aggregates are initialized and see if I could plug
> > into that
> > process somehow.
> >
> >
> > I agree. We need to tackle down the root cause.
> > I think this issue is related to previously fixed issue 1153
> > <https://glassfish.dev.java.net/issues/show_bug.cgi?id=1153
> <https://glassfish.dev.java.net/issues/show_bug.cgi?id=1153>>
> > Even if AggregateObjectMapping is primary key in Joined strategy, it
> > contains only the fields of root entity class. Subclass has also
> > primary key join columns(secondary pk fields) corresponding to the
> > ones of root entity class. In some cases such as this issue and
> 1153,
> > the pk join columns are used but the AggregateObjectMapping(and its
> > reference descriptor) has no info about the pk join columns, so
> issues
> > like this occur. IMHO, What we need is adding such info to
> > AggregateObjectMapping and taking advantage of it. I need to do more
> > work for the detail about this, but if you have idea let me know.
> >
> > Thanks,
> > -Wonseok
> >
> > -Tom
> >
> > Wonseok Kim wrote:
> >
> > > Hi Tom,
> > >
> > > I'm investigating the issue 1391 - Java2DB generates Wrong
> > primary key
> > > type in JOINED strategy
> > > https://glassfish.dev.java.net/issues/show_bug.cgi?id=1391
> > < https://glassfish.dev.java.net/issues/show_bug.cgi?id=1391>
> > > < https://glassfish.dev.java.net/issues/show_bug.cgi?id=1391>
> > >
> > > The root cause of this is that the 'type' value of
> > DatabaseFields for
> > > subclass primary key fields is null.
> > > We don't call setType() for DatabaseFields which come from
> > > AggregateObjectMapping like below.
> > >
> > > ClassDescriptor:2155
> > > // Index and classify fields and primary key.
> > > // This is in post because it needs field
> classification defined
> > > in initializeMapping
> > > // this can come through a 1:1 so requires all
> descriptors to be
> > > initialized (mappings).
> > > // May 02, 2000 - Jon D.
> > > // Added mapping.isAggregateMapping() check for fix to
> pr 381
> > > for (int index = 0; index < getFields().size(); index++) {
> > > DatabaseField field =
> > > (DatabaseField)getFields().elementAt(index);
> > > DatabaseMapping mapping =
> > > getObjectBuilder().getMappingForField(field);
> > > if ((mapping != null) &&
> > (!(mapping.isAggregateMapping()))) {
> > > field.setType(mapping.getFieldClassification
> (field));
> > > }
> > > field.setIndex(index);
> > > }
> > >
> > > Of course, even if I comment "!(mapping.isAggregateMapping()"
> > out, the
> > > type is still null for subclass primary key fields because
> > > AggregateObjectMapping.getFieldClassification (field) returns
> > null for
> > > fields of subclass table. So I should take another approach.
> > >
> > > But I'm curious what "pr 381" is and why the type of
> > > AggregateObjectMapping fields should be remained null. Tom,
> > could you
> > > check this?
> > >
> > > As you see below logs, if the type of DatabaseField is
> null it is
> > > defaulted to String in DDL generation and it produces this
> issue.
> > >
> > > [TopLink Finest]: 2006.12.06
> > > 09:50:57.746--Thread(Thread[main,5,main])--The default table
> > generator
> > > could not locate or convert a java type (null) into a
> database type
> > > for database field (DDL_TARGET.ID2). The generator uses
> > > java.lang.String as default java type for the field.
> > > [TopLink Finest]: 2006.12.06
> > > 09:50:57.747--Thread(Thread[main,5,main])--The default table
> > generator
> > > could not locate or convert a java type (null) into a
> database type
> > > for database field (DDL_TARGET.ID1). The generator uses
> > > java.lang.String as default java type for the field.
> > >
> > > My solution for this is setting the type of subclass
> primary key
> > > FieldDefinition same to the type of superclass pk
> > FieldDefinition in
> > > DefaultTableGenerator.addJoinColumnsFkConstraint (). Similar
> > thing is
> > > being done in addForeignKeyFieldToFieldDefinition() for
> relationship
> > > fields like below.
> > >
> > > FieldDefinition srcFldDef =
> > (FieldDefinition)fieldMap.get(dbSrcField);
> > > FieldDefinition trgFldDef =
> > > (FieldDefinition)fieldMap.get(dbTrgField);
> > >
> > > if(srcFldDef != null && trgFldDef != null) {
> > > // Also ensure that the type, size and subsize of the
> > foreign
> > > key field is
> > > // same as that of the original field.
> > > srcFldDef.setType (trgFldDef.getType());
> > > srcFldDef.setSize(trgFldDef.getSize());
> > > srcFldDef.setSubSize (trgFldDef.getSubSize());
> > > }
> > >
> > > It seems to work correctly. Do you see any problem in this
> approach?
> > >
> > > Regards,
> > > -Wonseok
> > >
> >
>