Comment 1 Michael OBrien 2009-11-10 16:09:50 EST

>For both of the related DI 98 issues in bug# 294765 and bug# 294811 - part of the context is the fact that the keyMapping only exists on the MappedKeyMapContainerPolicy and not on the other two MapContainerPolicy and DirectMapContainerPolicy because of a constraint on the map key type.
 ContainerPolicy (A)
    +=== InterfaceContainerPolicy (A)
             +=== DirectMapContainerPolicy
             +=== MapContainerPolicy (use keyField or PK class)
                      +=== MappedKeyMapContainerPolicy (use keyMapping.mapKeyTargetType or attributeClassification)

Comment 2 Michael OBrien 2009-11-12 07:43:56 EST

>This issue is not dependent or blocks bug # 266912 - however when a fix is performed - the workaround in the currently single implementor in MapAttribute should be removed because it will then be redundant.

Comment 3 Gordon Yorke 2009-11-12 10:35:36 EST

MapContainerPolicy.keyName is the attribute name of the Map's element class that stores the key.  Getting that mapping will give you the key type.

There is no need to access the CMP3Policy.

The code in MapAttributeImpl needs to be updated.  
Also why wouldn't you just add a getKeyClass() method to the MapContainerPolicy that returned the information that you needed?  That method would be overloaded in MappedKeyMapContainerPolicy

Comment 4 Michael OBrien 2009-11-12 15:10:18 EST

>This bug is valid. I did try using keyName - in UC12 it is also null even with an eager map as you can see, I already discussed this with another senior member of the JPA team and we agreed it was an issue.
policy	MapContainerPolicy  (id=49)	
	cloneMethod	null	
	constructor	Constructor<T>  (id=87)	
	containerClass	Class<T> (org.eclipse.persistence.indirection.IndirectMap) (id=91)	
	containerClassName	"org.eclipse.persistence.indirection.IndirectMap" (id=95)	
	elementClass	Class<T> (org.eclipse.persistence.testing.models.jpa.metamodel.GalacticPosition) (id=97)	
	elementClassName	"org.eclipse.persistence.testing.models.jpa.metamodel.GalacticPosition" (id=99)	
	elementDescriptor	RelationalDescriptor  (id=76)	
	keyField	null	
	keyMethod	null	
	keyName	null	

"Also why wouldn't you just add a getKeyClass() method to the MapContainerPolicy"
>This is the whole point of this bug - to offload an issue with Maps that has nothing to do with the metamodel API. I am not fixing MapContainerPolicy in core JPA at this time because work outside of the scope of the Metamodel API is not currently scheduled becaue we need to close 266912 as you know.

Comment 5 Gordon Yorke 2009-11-12 16:43:27 EST

If both keyName and getKeyType return null for certain use cases this is a bigger issue then just the MetaModel API.  What you and this other "senior member of the JPA team" failed to see is that the key() and entry() expressions will also fail for these usecases.
I will update the summary to better reflect the impact of this issue.

Also if the core had a deficiency in functionality that a developer's feature needed it would be a similar amount of work and produce a much higher quality of code to add the functionality to the core instead of hacking together a workaround.  Committers should only "workaround" functionality when there is no other choice.

Comment 6 Michael OBrien 2009-11-12 19:07:44 EST

Thanks for the update on the deeper issue - appreciated.
While i would like to spend more time researching a full fix for all workarounds (as I am known) - I am in reality limited by time constraints and triaged this issue just enough to hand it off so I could "focus" as instructed to do.
"Committers should only "workaround" functionality when there is no
other choice" - I fully agree with this in principal - but in practice the API needs to go out asap (like yesterday) and this 2nd last issue was holding it up.

Comment 7 Michael OBrien 2009-11-13 09:31:53 EST

>Problem Summary 
=====================
When the name attribute is not defined for a @MapKey annotation on a Map<K,V>  for a non-Basic (POJO or @EmbeddedId) type as key - the resulting MapContainerPolicy has all 3 fields (keyField, keyMethod and keyName uninitialized as null.
=====================

- My summary was not fully clear originally - see 3 use cases on Computer.class (#10,12,13) - not #9 on Manufacturer.class


>Code
---------------------
In the following call the initializeKey() fails to set either the keyField or the keyMethod 

MapContainerPolicy
    public Object getKeyType(){
-->     initializeKey();
        if (keyField != null){
            return keyField.getType();
        } else if (keyMethod != null){
            return keyMethod.getReturnType();
        }
-->     return null;
    }

Comment 8 Gordon Yorke 2009-11-13 13:03:22 EST

all the bug needs is getKeyType to add 3 line to:

    public Object getKeyType(){
        initializeKey();
        if (keyField != null){
            return keyField.getType();
        } else if (keyMethod != null){
            return keyMethod.getReturnType();
        }else{                                                            //<-
            return getElementDescriptor().getCMPPolicy().getPKClass();    //<-
       }                                                                  //<-
    }

Comment 9 Michael OBrien 2009-11-13 16:00:45 EST

>See post rev# 5793 test cases that can be used to verify this fix - comment out MapAttribute.java:183 to verify
TEST NAME: testMapAttribute_getKeyType_294811_UC10_DI86_Embeddable_IdClass_keyType_Method(org.eclipse.persistence.testing.tests.jpa.metamodel.MetamodelMetamodelTest)
TEST NAME: testMapAttribute_getKeyType_294811_UC12_DI86_Embedded_keyType_Method(org.eclipse.persistence.testing.tests.jpa.metamodel.MetamodelMetamodelTest)
TEST NAME: testMapAttribute_getKeyType_294811_UC13_DI86_Embedded_keyType_Method(org.eclipse.persistence.testing.tests.jpa.metamodel.MetamodelMetamodelTest)

Comment 10 Tom Ware 2009-11-17 12:43:03 EST

Created attachment 152410 [details]
proposed changes

Attaching changes that remove the workaround specified in this bug and also fix the following issues:

- JPQL KEY() notation with a composite PK that has derived ID as part of it
- Reading of Object with a composite PK that has derived ID as part of it

Comment 11 Tom Ware 2009-11-17 16:26:37 EST

- clean up code for metamodel generation and maps
- fix JPQL KEY() notation with a composite PK that has derived ID as part of it
- fix Reading of Object with a composite PK that has derived ID as part of it

Parts reviewed by Michael O'Brien, Guy Pelletier and Gordon Yorke

Added test to JPQL test model and derived id model and ran full testing for JPA and core.

Comment 12 Eclipse Webmaster 2022-06-09 10:20:22 EDT

The Eclipselink project has moved to Github: https://github.com/eclipse-ee4j/eclipselink