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> 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>> 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>
> > 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
> > >
> >
> > --
> > Tom Ware
> > Principal Software Engineer
> > Oracle Canada Inc.
> >
> > Direct: (613) 783-4598
> > Email: tom.ware_at_oracle.com <mailto:tom.ware_at_oracle.com>
> >
> >
>
> --
> Tom Ware
> Principal Software Engineer
> Oracle Canada Inc.
>
> Direct: (613) 783-4598
> Email: tom.ware_at_oracle.com
>