CODEWITHSUNDEEP
javacollectionsobjectjpa-2.0
simpatico
simpatico

Reputation: 11107

Why my equals method won't work?

assertEquals(def.getMengs(), exp.getMengs());

fails, reporting: expected: java.util.HashSet<[...so geht die Legende ... ...legend has it ... ]> but was: java.util.HashSet<[...so geht die Legende ... ...legend has it ... ]>

Indeed, through the debugger I see that both sets contain only one Meaning with objId = 1 for both. I expected the following code in Meaning class (@Entity) to guarantee that the above works.

@Override
public boolean equals(Object object) {
   if (!(object instanceof Meaning)) {
        return false;
   }
   Meaning other = (Meaning) object;
   if (other != null && objId == other.objId) return true;
   return false;
}

@Override
public int hashCode() {
    int hash = 7;
    hash = 67 * hash + this.objId;
    return hash;
}

Indeed, this test passes:

 db.insert(admin);
    final Meaning meng = new Meaning(admin, new Expression("essen"));
    meng.setObjId(11);
    final Meaning meng1  = new Meaning(admin, new Expression("mangiare"));
    meng1.setObjId(11);
    assertEquals(meng,meng1);

So what could be my problem? Thery are both HashSets, they are both of the same size, and the objects inside them equals. Indeed

            assertEquals(def.getMengs().iterator().next(), exp.getMengs().iterator().next());

before it passes. However this won't (but I don't know why):

assertTrue(def.getMengs().containsAll(exp.getMengs()));

So, it's the problem.

Here's the test code:

try{
            db.insertWords(toEnumMap(mengs[i],admin));
        }catch(Exception e){
            fail(e.getMessage());
        }
        final Expression exp = db.get(Expression.class, mengs[i][0]);
        testGender(exp, mengs[i][2]);

        final Expression def = db.get(Expression.class, mengs[i][1]);
        assertNotNull(def);

        assertEquals(def.getMengs().iterator().next(), exp.getMengs().iterator().next());
        assertEquals(exp.getMengs().size(), def.getMengs().size());
        assertTrue(def.getMengs().containsAll(def.getMengs()));
        assertTrue(def.getMengs().containsAll(exp.getMengs()));
        assertEquals(def.getMengs(), exp.getMengs());

db.get just wraps em.find. InsertWords should be persisting def and exp.

public void insertWords(EnumMap<Input, MemoEntity> input) throws MultipleMengsException {
    insert(input.get(Input.expression)); //INSERT OR IGNORE
    final boolean isNewDef = insert(input.get(Input.definition));

    final Expression def = get(Expression.class, input.get(Input.definition).getId());
    final Expression exp = get(Expression.class, input.get(Input.expression).getId());
    final MUser usr = get(MUser.class, input.get(Input.user).getId());

    final Set<Meaning> mengs = getMengs(usr,def,isNewDef);
    if (mengs == null) {//is new to the user
        final Meaning meng = new Meaning(usr, exp, def);
        insert(meng);

    } else { //old meaning
        if (mengs.size() > 1) throw new MultipleMengsException(mengs);
        else{
            final Meaning meng = mengs.iterator().next();
            meng.addExp(exp);
            meng.setLastPublishedDate(null); //reschedule
        }
    }
    Logger.getAnonymousLogger().log(Level.INFO, "inserted pair <{0},{1}>", new String[]{exp.getExpression(), def.getExpression()});
}

public boolean insert(final MemoEntity entity) {
        if (em.find(entity.getClass(), entity.getId()) == null) {
            et.begin();
            em.persist(entity);
            et.commit();
            return true;
        }
        return false;
    }

public <MemoEntity> MemoEntity get(final Class<MemoEntity> entityClass, final Object primaryKey) {
        return em.find(entityClass, primaryKey);
    }

Upvotes: 1

Views: 819

Answers (6)

james
james

Reputation: 116

You are using hibernate entities, so you should never refer to member variables directly, as they may be lazily loaded from the database. so, your equals/hashCode method should use getObjId() instead of objId.

also, as mentioned in another post, if your objId is an object type (Integer, Long), you should not be using "==".

Upvotes: 0

J&#246;rn Horstmann
J&#246;rn Horstmann

Reputation: 34044

Is your objId an Integer or Long? Then your problem is probably related to autoboxing in the equals method:

objId == other.objId

This will work for small constants like in your first test since these are cached. Generally you should use the equals method in this case. That line in the equals method could better be written as:

 return objId == null ? this == other : objId.equals(other.objId);

Upvotes: 1

Kris
Kris

Reputation: 14468

If 

assertTrue(def.getMengs().containsAll(exp.getMengs()));

passes, then the most likely explanation is that def.getMengs() contains all elements from exp.getMengs() plus some other not in the latter.

Try reversing it, i.e.

assertTrue(exp.getMengs().containsAll(def.getMengs()));

or simply

assertEqual(exp.getMengs().size(), def.getMengs().size());

Edit

I see I've misread your question. However, this does clarify the situation. The equals method checks for 3 things. 1) That both are of type Set. 2) Same size and 3) That "a" contains all elements from "b".

You seem to be failing that last one. Indeed, since doing containsAll on a HashSet with itself is failing it must be the equals method on Meaning. Reading the code (Java 6) for the containsAll and contains methods on Sets clearly shows that the hashCode method is not used for this purpose.

Upvotes: 1

mdma
mdma

Reputation: 57777

HashCode and equals for entities are best written without using the surrogate ID for comparison, but rather implement comparison using the business attributes of the object or just the natural keys. See this SO question for details.

EDIT: As to why this doesn't work, my only guess is that you are modifying the object after adding it to the HashSet, in particular changing the ID. This will cause the contains method to fail, since it uses the hashCode to locate the object as a key in the hashmap, and if the ID has changed, it will be looking in the wrong place in the underlying hashMap.

Upvotes: 3

anon
anon

Reputation:

If you hash by objId, then it shouldn't be mutable (as the method setObjId suggests). In particular, if you insist on having that method, you shouldn't call it after putting an object in a hashset.

Upvotes: 0

Karel Petranek
Karel Petranek

Reputation: 15164

The problem is that containsAll compares references to the objects (using ==) instead of calling .equals. HashSet.equals uses containsAll internally according to the documentation.

A possible solution is to derive your own class from HashSet and override the containsAll method.

Upvotes: -1

Related Questions