Bug 76 : .equals() enhancement ... not right for primitive id types (int and long)
Priority 
High
Reported Version 
 
Logged By 
Rob
Status 
Fixed
Fixed Version 
1.1.0
Assigned To 
 
Product 
Ebean - core
Duplicate Of 
 
Created 
24/02/2009
Updated 
24/02/2009
Type 
Bug
 
Attachments 
No attachments

Tests:

package main;

import app.data.PrimEntity;

import com.avaje.ebean.bean.EntityBean;

public class TestPrimEquals {

public static void main(String[] args) {

PrimEntity t0 = new PrimEntity();

if (t0 instanceof EntityBean){
// only interesting in testing
// enhanced bean (with primitive id)
testOne();



} else {
throw new RuntimeException("Not enhanced");
}
}

private static void testOne() {


PrimEntity a0 = new PrimEntity();
PrimEntity a1 = new PrimEntity();
assert !a0.equals(a1);

PrimEntity b0 = new PrimEntity();
b0.setId(5);
PrimEntity b1 = new PrimEntity();
b1.setId(5);
assert b1.equals(b0);

PrimEntity dummy = new PrimEntity();

PrimEntity c0 = new PrimEntity();
c0.equals(dummy);
c0.setId(4);

PrimEntity c1 = new PrimEntity();
c1.setId(4);

assert !c0.equals(c1);

PrimEntity e0 = new PrimEntity();
e0.equals(dummy);
e0.setId(4);

PrimEntity e1 = new PrimEntity();
e1.equals(dummy);
e1.setId(4);

assert !e0.equals(e1);


System.out.println("done");
}

}

 
Rob 24 Feb 09:54
The problem...

The problem was in the _ebean_getIdentity() generated method. It was checking for null to test if the id property had been set. For primitives it instead needs to test for id != 0 (0 being the default value for int and long).

Rob 24 Feb 09:54
I have fixed this.

... just need to commit it up.

Rob 24 Feb 10:23
Mods for future reference

--- ebean/trunk/src/com/avaje/ebean/enhance/agent/FieldMeta.java 2009-02-23 22:24:19 UTC (rev 142)
+++ ebean/trunk/src/com/avaje/ebean/enhance/agent/FieldMeta.java 2009-02-24 10:01:43 UTC (rev 143)
@@ -243,6 +243,22 @@
|| annotations.contains("Lcom/avaje/ebean/annotation/EmbeddedColumns;");
}

+ public void appendGetField(MethodVisitor mv, ClassMeta classMeta) {
+ mv.visitFieldInsn(GETFIELD, classMeta.getClassName(), fieldName , fieldDesc);
+ }
+
+ public void appendValueOf(MethodVisitor mv, ClassMeta classMeta) {
+ if (primativeType) {
+ // use valueOf methods to return primitives as objects
+ Type objectWrapperType = PrimitiveHelper.getObjectWrapper(asmType);
+
+ String objDesc = objectWrapperType.getInternalName();
+ String primDesc = asmType.getDescriptor();
+
+ mv.visitMethodInsn(Opcodes.INVOKESTATIC, objDesc, "valueOf", "(" + primDesc + ")L" + objDesc + ";");
+ }
+ }
+
/**
* As part of the switch statement to read the fields generate the get code.
*/

Modified: ebean/trunk/src/com/avaje/ebean/enhance/agent/IndexFieldWeaver.java
===================================================================
--- ebean/trunk/src/com/avaje/ebean/enhance/agent/IndexFieldWeaver.java 2009-02-23 22:24:19 UTC (rev 142)
+++ ebean/trunk/src/com/avaje/ebean/enhance/agent/IndexFieldWeaver.java 2009-02-24 10:01:43 UTC (rev 143)
@@ -102,7 +102,7 @@

} else {
// add the _ebean_getIdentity(), equals() and hashCode() methods
- MethodEquals.addMethods(cv, classMeta, idIndex, idFieldMeta.getName());
+ MethodEquals.addMethods(cv, classMeta, idIndex, idFieldMeta);
}
}


Modified: ebean/trunk/src/com/avaje/ebean/enhance/agent/MethodEquals.java
===================================================================
--- ebean/trunk/src/com/avaje/ebean/enhance/agent/MethodEquals.java 2009-02-23 22:24:19 UTC (rev 142)
+++ ebean/trunk/src/com/avaje/ebean/enhance/agent/MethodEquals.java 2009-02-24 10:01:43 UTC (rev 143)
@@ -46,7 +46,7 @@
* @param idFieldIndex
* the index of the id field
*/
- public static void addMethods(ClassVisitor cv, ClassMeta meta, int idFieldIndex, String fieldName) {
+ public static void addMethods(ClassVisitor cv, ClassMeta meta, int idFieldIndex, FieldMeta idFieldMeta) {

if (meta.hasEqualsOrHashCode()) {
// already has a equals or hashcode method...
@@ -57,9 +57,13 @@
} else {
if (meta.isLog(2)) {
meta.log("adding equals() hashCode() and _ebean_getIdentity() with Id field "
- + fieldName+ " index:" + idFieldIndex);
+ + idFieldMeta.getName()+ " index:" + idFieldIndex+" primative:"+idFieldMeta.primativeType);
}
- addGetIdentity(cv, meta, idFieldIndex);
+ if (idFieldMeta.primativeType){
+ addGetIdentityPrimitive(cv, meta, idFieldIndex, idFieldMeta);
+ } else {
+ addGetIdentityObject(cv, meta, idFieldIndex);
+ }
addEquals(cv, meta);
addHashCode(cv, meta);
}
@@ -77,7 +81,13 @@
}

/**
- * Generate the _ebean_getIdentity method for used with equals().
+ * Generate the _ebean_getIdentity method for primitive id types.
+ *


+ * For primitives we need to check for == 0 rather than null.
+ *


+ *


+ * This is used for implementing equals().
+ *


*
*

* private Object _ebean_getIdentity() {
@@ -85,27 +95,27 @@
    *         if (_ebean_identity != null) {
    *             return _ebean_identity;
    *         }
-    *         Object id = getId();
-    *         if (id != null) {
-    *             return id;
+    *        
+    *         if (id != 0) {
+    *             _ebean_identity = Integer.valueOf(id);
+    *         } else {
+    *             _ebean_identity = new Object();
    *         }
    *
-    *         if (_ebean_identity == null) {
-    *             _ebean_identity = new Object();
-    *         }
    *         return _ebean_identity;
    *     }
    * }
    *

*/
- private static void addGetIdentity(ClassVisitor cv, ClassMeta classMeta, int idFieldIndex) {
-
+ private static void addGetIdentityPrimitive(ClassVisitor cv, ClassMeta classMeta, int idFieldIndex, FieldMeta idFieldMeta) {
+
String className = classMeta.getClassName();

MethodVisitor mv;

mv = cv.visitMethod(ACC_PRIVATE, _EBEAN_GET_IDENTITY, "()Ljava/lang/Object;", null, null);
mv.visitCode();
+
Label l0 = new Label();
Label l1 = new Label();
Label l2 = new Label();
@@ -114,15 +124,117 @@
Label l4 = new Label();
mv.visitTryCatchBlock(l3, l4, l2, null);
Label l5 = new Label();
+ mv.visitTryCatchBlock(l2, l5, l2, null);
Label l6 = new Label();
- mv.visitTryCatchBlock(l5, l6, l2, null);
+ mv.visitLabel(l6);
+ mv.visitLineNumber(1, l6);
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitInsn(DUP);
+ mv.visitVarInsn(ASTORE, 1);
+ mv.visitInsn(MONITORENTER);
+ mv.visitLabel(l0);
+ mv.visitLineNumber(1, l0);
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitFieldInsn(GETFIELD, className, IDENTITY_FIELD, "Ljava/lang/Object;");
+ mv.visitJumpInsn(IFNULL, l3);
Label l7 = new Label();
- mv.visitTryCatchBlock(l2, l7, l2, null);
+ mv.visitLabel(l7);
+ mv.visitLineNumber(1, l7);
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitFieldInsn(GETFIELD, className, IDENTITY_FIELD, "Ljava/lang/Object;");
+ mv.visitVarInsn(ALOAD, 1);
+ mv.visitInsn(MONITOREXIT);
+ mv.visitLabel(l1);
+ mv.visitInsn(ARETURN);
+ mv.visitLabel(l3);
+ mv.visitLineNumber(1, l3);
+ mv.visitVarInsn(ALOAD, 0);
+ idFieldMeta.appendGetField(mv, classMeta);
Label l8 = new Label();
+ mv.visitJumpInsn(IFEQ, l8);
+ Label l9 = new Label();
+ mv.visitLabel(l9);
+ mv.visitLineNumber(1, l9);
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitVarInsn(ALOAD, 0);
+ idFieldMeta.appendGetField(mv, classMeta);
+ idFieldMeta.appendValueOf(mv, classMeta);
+ mv.visitFieldInsn(PUTFIELD, className, IDENTITY_FIELD, "Ljava/lang/Object;");
+ Label l10 = new Label();
+ mv.visitJumpInsn(GOTO, l10);
mv.visitLabel(l8);
mv.visitLineNumber(1, l8);
mv.visitVarInsn(ALOAD, 0);
+ mv.visitTypeInsn(NEW, "java/lang/Object");
mv.visitInsn(DUP);
+ mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "", "()V");
+ mv.visitFieldInsn(PUTFIELD, className, IDENTITY_FIELD, "Ljava/lang/Object;");
+ mv.visitLabel(l10);
+ mv.visitLineNumber(1, l10);
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitFieldInsn(GETFIELD, className, IDENTITY_FIELD, "Ljava/lang/Object;");
+ mv.visitVarInsn(ALOAD, 1);
+ mv.visitInsn(MONITOREXIT);
+ mv.visitLabel(l4);
+ mv.visitInsn(ARETURN);
+ mv.visitLabel(l2);
+ mv.visitLineNumber(1, l2);
+ mv.visitVarInsn(ALOAD, 1);
+ mv.visitInsn(MONITOREXIT);
+ mv.visitLabel(l5);
+ mv.visitInsn(ATHROW);
+ Label l11 = new Label();
+ mv.visitLabel(l11);
+ mv.visitLocalVariable("this", "L"+className+";", null, l6, l11, 0);
+ mv.visitMaxs(3, 2);
+ mv.visitEnd();
+ }
+
+ /**
+ * Generate the _ebean_getIdentity method for used with equals().
+ *
+ *

+    * private Object _ebean_getIdentity() {
+    *     synchronized (this) {
+    *         if (_ebean_identity != null) {
+    *             return _ebean_identity;
+    *         }
+    *
+    *         Object id = getId();
+    *         if (id != null) {
+    *             _ebean_identity = id;
+    *         } else {
+    *             _ebean_identity = new Object();
+    *         }
+    *
+    *         return _ebean_identity;
+    *     }
+    * }
+    *

+ */
+ private static void addGetIdentityObject(ClassVisitor cv, ClassMeta classMeta, int idFieldIndex) {
+
+ String className = classMeta.getClassName();
+
+ MethodVisitor mv;
+
+ mv = cv.visitMethod(ACC_PRIVATE, _EBEAN_GET_IDENTITY, "()Ljava/lang/Object;", null, null);
+ mv.visitCode();
+
+ Label l0 = new Label();
+ Label l1 = new Label();
+ Label l2 = new Label();
+ mv.visitTryCatchBlock(l0, l1, l2, null);
+ Label l3 = new Label();
+ Label l4 = new Label();
+ mv.visitTryCatchBlock(l3, l4, l2, null);
+ Label l5 = new Label();
+ mv.visitTryCatchBlock(l2, l5, l2, null);
+ Label l6 = new Label();
+ mv.visitLabel(l6);
+ mv.visitLineNumber(1, l6);
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitInsn(DUP);
mv.visitVarInsn(ASTORE, 1);
mv.visitInsn(MONITORENTER);
mv.visitLabel(l0);
@@ -130,16 +242,16 @@
mv.visitVarInsn(ALOAD, 0);
mv.visitFieldInsn(GETFIELD, className, IDENTITY_FIELD, "Ljava/lang/Object;");
mv.visitJumpInsn(IFNULL, l3);
- Label l9 = new Label();
- mv.visitLabel(l9);
- mv.visitLineNumber(1, l9);
+ Label l7 = new Label();
+ mv.visitLabel(l7);
+ mv.visitLineNumber(1, l7);
mv.visitVarInsn(ALOAD, 0);
mv.visitFieldInsn(GETFIELD, className, IDENTITY_FIELD, "Ljava/lang/Object;");
mv.visitVarInsn(ALOAD, 1);
mv.visitInsn(MONITOREXIT);
mv.visitLabel(l1);
mv.visitInsn(ARETURN);
-
+
mv.visitLabel(l3);
mv.visitLineNumber(1, l3);
mv.visitVarInsn(ALOAD, 0);
@@ -147,54 +259,49 @@
mv.visitVarInsn(ALOAD, 0);
mv.visitMethodInsn(INVOKESPECIAL, className, "_ebean_getField", "(ILjava/lang/Object;)Ljava/lang/Object;");
mv.visitVarInsn(ASTORE, 2);
-
+
+ Label l8 = new Label();
+ mv.visitLabel(l8);
+ mv.visitLineNumber(1, l8);
+ mv.visitVarInsn(ALOAD, 2);
+ Label l9 = new Label();
+ mv.visitJumpInsn(IFNULL, l9);
Label l10 = new Label();
mv.visitLabel(l10);
mv.visitLineNumber(1, l10);
+ mv.visitVarInsn(ALOAD, 0);
mv.visitVarInsn(ALOAD, 2);
- mv.visitJumpInsn(IFNULL, l5);
+ mv.visitFieldInsn(PUTFIELD, className, IDENTITY_FIELD, "Ljava/lang/Object;");
Label l11 = new Label();
- mv.visitLabel(l11);
- mv.visitLineNumber(1, l11);
- mv.visitVarInsn(ALOAD, 2);
- mv.visitVarInsn(ALOAD, 1);
- mv.visitInsn(MONITOREXIT);
- mv.visitLabel(l4);
- mv.visitInsn(ARETURN);
- mv.visitLabel(l5);
- mv.visitLineNumber(1, l5);
+ mv.visitJumpInsn(GOTO, l11);
+ mv.visitLabel(l9);
+ mv.visitLineNumber(1, l9);
mv.visitVarInsn(ALOAD, 0);
- mv.visitFieldInsn(GETFIELD, className, IDENTITY_FIELD, "Ljava/lang/Object;");
- Label l12 = new Label();
- mv.visitJumpInsn(IFNONNULL, l12);
- Label l13 = new Label();
- mv.visitLabel(l13);
- mv.visitLineNumber(1, l13);
- mv.visitVarInsn(ALOAD, 0);
mv.visitTypeInsn(NEW, "java/lang/Object");
mv.visitInsn(DUP);
mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "", "()V");
mv.visitFieldInsn(PUTFIELD, className, IDENTITY_FIELD, "Ljava/lang/Object;");
- mv.visitLabel(l12);
- mv.visitLineNumber(1, l12);
+ mv.visitLabel(l11);
+ mv.visitLineNumber(1, l11);
mv.visitVarInsn(ALOAD, 0);
mv.visitFieldInsn(GETFIELD, className, IDENTITY_FIELD, "Ljava/lang/Object;");
mv.visitVarInsn(ALOAD, 1);
mv.visitInsn(MONITOREXIT);
- mv.visitLabel(l6);
+ mv.visitLabel(l4);
mv.visitInsn(ARETURN);
mv.visitLabel(l2);
mv.visitLineNumber(1, l2);
mv.visitVarInsn(ALOAD, 1);
mv.visitInsn(MONITOREXIT);
- mv.visitLabel(l7);
+ mv.visitLabel(l5);
mv.visitInsn(ATHROW);
- Label l14 = new Label();
- mv.visitLabel(l14);
- mv.visitLocalVariable("this", "L" + className + ";", null, l8, l14, 0);
- mv.visitLocalVariable("tmpId", "Ljava/lang/Object;", null, l10, l2, 2);
+ Label l12 = new Label();
+ mv.visitLabel(l12);
+ mv.visitLocalVariable("this", "L"+className+";", null, l6, l12, 0);
+ mv.visitLocalVariable("tmpId", "Ljava/lang/Object;", null, l8, l2, 2);
mv.visitMaxs(3, 3);
mv.visitEnd();
+
}

Rob 24 Feb 22:03
Need to review this

Refer to Bug 80 ... use of long.

Rob 25 Feb 09:47
issue for long, double and float

This was an issue for long, double and float primitive types.

Extra bytecodes needed to compare those to the matching 0 value in _ebean_getIdentity() ... method used by the generated equals().

Fixed in HEAD.

woResponse

Upload a file