@GenerateValue (AUTO) doesn't work with Property level access

• Key: OPENJPA-272
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Kevin Sutter
• Reporter: Kevin Sutter
• Component/s: kernel
• Affects Version/s: 0.9.7
• Fix Version/s: 1.0.0

Description

The @GenerateValue annotation doesn't work correctly when applied to via the Property level access (getter method) when using the wrapper classes for the primitive types. Something like this:

    private Long id;

    @Id
    @GeneratedValue(strategy=GenerationType.AUTO)
public Long getId() {
return id;
}

public void setId(Long id) {
this.id = id;
}

With this type of Entity definition, we hit a problem when checking for the "default value":

    public boolean isDefaultValue() {
        return dblval == 0 && longval == 0
            && (objval == null || "".equals(objval));
    }

For this scenario, objval is not null and it's not of type String, so we fail this test and return false. Upon returning the value of false, the calling code skips the call that would have assigned the generated value to the field (in ApplicationIds):

    private static boolean assign(OpenJPAStateManager sm, StoreManager store,
        FieldMetaData[] pks, boolean preFlush) {
        for (int i = 0; i < pks.length; i++)
            if (pks[i].getValueStrategy() != ValueStrategies.NONE
                && sm.isDefaultValue(pks[i].getIndex())
                && !store.assignField(sm, pks[i].getIndex(), preFlush))
                return false;
        return true;
    }

I haven't figured out the exact fix yet, but there are two workarounds available:

1. Use field level annotations instead of property, or...
2. Don't use the primitive wrapper types (use long instead of Long).

In either of these cases, objval is left as null and we are eventually allowed to call store.assignField() which gets the generated value assigned to the field in question (id in this case).

I will keep digging, but if anyone knows the history of the isDefaultValue() method, it would help with getting a quick answer to this Issue. Since we're dealing with generated values, I'm not clear why we care if values are already assigned to this field or not. It would seem that we would want to just override what's there. But, like I said, I need to dive into this a bit. I just wanted to get the Issue on the books with the information I discovered thus far.

Kevin

Comments

[Kevin Sutter]

The result of this type of scenario is that you get duplicate key exceptions because the id generation never takes place. Thus, you end up with attempting to store multiple rows with the same key of 0:

Caused by: <0.0.0 nonfatal store error> org.apache.openjpa.util.StoreException: The statement was aborted because it would have caused a duplicate key value in a unique or primary key constraint or unique index identified by 'SQL070629094257460' defined on 'GASPURCHASE'. {prepstmnt 429791646 INSERT INTO GASPURCHASE (id, DAYNUMBER, GRADENUMBER, PUMPNUMBER, QUANTITY) VALUES (?, ?, ?, ?, ?) [params=(long) 0, (int) 5, (int) 6, (int) 7, (int) 8]} [code=20000, state=23505]
FailedObject: com.ibm.ws.jpa.entities.GasPurchaseProperty@4e144e14

[Kevin Sutter]

New information...

My original posting wasn't quite accurate. You need to mix the use of primitive and wrapper types to get this to fail:

    private long id;

    @Id
    @GeneratedValue(strategy=GenerationType.AUTO)
    public Long getId() {
        return id;
    }

    public void setId(Long id) {
        this.id = id;
    }

In this case, it seems that the autoboxing feature of Java 5 kicks in and when getId() gets called, the default value of "long id" (0) is returned as an autoboxed Long. This returned Long object then throws off the conditional code mentioned above and we don't set the generated value appropriately.

So, now I go back to my question of why the invocation of sm.isDefaultValue() necessary? Do we care there's a default value when we're generating one anyway?

Kevin

[Patrick Linskey]

Man. Another place where property access rules are annoying.

It seems like one possible solution would be to just expand what is considered a default value to include auto-boxed values, when running in a JDK1.5 or higher environment.

[Kevin Sutter]

It's turning out that this is more than just autoboxing. The setting of initial values for fields annotated with @GeneratedValue is causing several problems.

For example, even if I have the same types through the Entity definition (where autoboxing does not come into play)...

    private Long id = new Long(5);

    @Id
    @GeneratedValue(strategy=GenerationType.AUTO)
    public Long getId() {
        return id;
    }

    public void setId(Long id) {
        this.id = id;
    }


.. I still get the duplicate key exceptions because our runtime logic doesn't know the difference between the initial value and somebody setting a value. As I pointed out in a separate mail thread, the invocation of the setter method was also being allowed to override the @GeneratedValue annotation. And, once you have set your own id, how do you tell OpenJPA "okay, I'm done now... go ahead and generate the rest of the ids now..."?

It just seems like we're opening a can of worms attempting to support both ways. I think the @GeneratedValue annotation should take precedence.

Kevin

[Kevin Sutter]

I just committed the changes to resolve this issue. Per the discussion on our dev mailing list (http://www.nabble.com/Allow-overrides-of-%40GeneratedValue--tf4031013.html#a11450606), I decided to go with the more direct response as Patrick and others suggested. That is, if somebody has @GeneratedValue on a field (id or otherwise) and then attempts to set a value either via an initializer or a setter method, then an InvalidStateException will be thrown. This will immediately let the user know that something isn't quite right. At first, I thought this was too drastic and was leaning towards a warning or error message. But, due to data integrity concerns, I decided to go with an exception to signal the problem. This will force the user to do something about the situation instead of blindly running with it until s/he notices the error message.

The exception thrown has a localizer message that indicates the problem and suggested actions to resolve it.

I also provided a new testcase to test for this new condition and the exception processing.

I also had to update the TestSharedMappedSuperclassIdValue testcase since it was incorrectly relying on this "incorrect" behavior.

Kevin

[Kevin Sutter]

Resolved via revision 562987.