Created attachment 30769 [details]
Simple bean showing true/false table for correct equals implementation

Your first option could throw a NullPointerException.
&& has a higher precedence than ||, so if you build the AST for that expression,
it would look like this:
Assume OR is the || operator and AND the && operator.
        
                      OR
                       |
         --------------------------
         |                        |
 this.codigo==null               AND
                                  |
                     ------------------------------
                     |                            |
              other.codigo==null   !this.codigo.equals(other.codigo))

Java evaluates left to right, so assume other.codigo==null evaluates to true.
AND cannot short-circuit the result of the expression and must evaluate the
second node. If this.codigo==null were true, you get your NullPointerException.

The second one works (doesn't throw an exception) and conforms to the semantics
you described. About being faster? Well, build the AST and you can take your
conclusions.

Another question: is that the semantics the persistence specs expects? (I
haven't looked carefully yet, but it should be considered).

Thanks for your comments. I've made my homework:

1) I've used ||, not | and &&, not &. I think one of us need to double check
lang docs. Lang specs 3.0, page 509, says "The && operator is like & (§15.22.2),
but evaluates its right-hand operand only if the value of its left-hand operand
is true." (in this case, the "OR" operation) - in some languages this is called
"precedence promotion". If this is wrong, then I was being programming wrong
since 1995...

2) I've posted a test program, showing logic behind my claim. There is no NPE.
It's a NB project, you can run it and give a try.

3) I don't have found any specs about this. I just take care about what SQL
standards states: NULL comparision with anything (including NULL) is false. 

<IMHO> There is an reason to the "is null" operator in every SQL standard
database around the world. Since Entity are object representation of database
row, then it's logic for equals comparisions obey same criteria.</IMHO>

Reading specs, I've found following paragraph (ejb-3_0-pdf-spec-persistence.pdf,
page 20):
"The primary key class must define equals and hashCode methods. The semantics of
value equality for these methods must be consistent with the database equality
for the database types to which the key is mapped.". Again, IMHO, this apply to
equals of every entity (every entity will have a primary key - explicit or
implicit).

Hum...

I agree that in a && operator the right hand side only gets executed/evaluated
if the left hand side is true. But my argument is about operator precedence...

But you're right. I'll double check the language specs.

How about this:

public boolean equals(Object object) {
    if (this == object) return true;
    if (object == null) return false;
    if (object instanceof Tabela) {
        Tabela other = (Tabela) object;
        if (codigo == null || other.codigo == null) return false;
        return codigo.equals(other.codigo);
    } else return false;
}

I think it makes the code much simpler to read and it abides to the semantics
you described.

If you fell comfortable with, for me, following the rules is what matter.
I didn't tested your equals implementation with sample project. Did you?

I have a problem with the proposal that equals() return false for a generated
entity class with a null primary key.  My concern is based on Java language
semantics (which I think should trump SQL database semantics when you are
implementing Java classes :-).

Currently, the generated hashCode() method will return zero unconditionally if
the primary key field is null.  If one compares the hashCode() values of two
different entity class instances with null primary keys, the hash codes are ==
but the equals() method returns false.  This is not explicitly disallowed, but
the javadocs for java.lang.Object imply that it is not recommended.

I would propose one of the following solutions:

(1) Accept the recommendation to make equals() return false
    if the primary key fields are null, but change hashCode()
    so that it will return different values (perhaps by
    delegating to super.hashCode()).

(2) Leave the hashCode() alone, but make equals() return
    true if the primary key fields of both instances
    are null.

I would prefer (2) over (1) but either seems like a reasonable approach from a
Java perspective.

I do understand your point of view.
But, choosing by something not recomended by Java specs (besides not wrong) and
choosing bt something wrong (from SQL point of view), I'll choose the first.
So, in options 1) and 2), I'll do prefer option 1.
IMHO, Making hashCode returns super.hashCode() in case where PK is null,
enforces idea where the objects are different.

I'd like to be able to do something like this:

...
tx.begin();
Order = em.find("some Order to update");
LineItem li1 = new LineItem(blah);
LineItem li2 = new LineItem(blue);
Order.addLine(li1); adds to persistent Set<LineItem>
Order.addLine(li2);
tx.commit(); // cascade persists new LineItems

In order for this to work, you need to have the Set<LineItem> work correctly, which means having non-
equal instances return false to equals. It's also bad form to change the hashCode of an instance once 
it's been put into a Set. This will have disastrous results if caching persistent instances between 
transactions.

Since the id of an instance can change during the execution of a program (for generated id fields), I'd 
say it's a poor choice for being the basis of either hashCode or equals. If the application doesn't have 
any intrinsic meaning for equals, then the code generator shouldn't try to create an equals or hashCode 
implementation that does anything but return the standard Object behavior. 

Because of uniquing (there can be exactly zero or one instance of a specific database object associated 
with any persistence context) there is no issue with using == for equals and System.identityHashCode 
for hashCode.

If the application does have a field which serves as the basis for equals, the wizard should ask for it. 
The id field is only suitable if it is managed by the application (and not generated by the jpa provider or 
the database).

So here's what I would implement for the default case (generated id field):
public boolean equals(Object object) {
  return this == object;
}
public int hashCode() {
    return System.identityHashCode(this);
}

If the user selected a field (e.g. codigo) to serve as the equality field (might be the id field if not 
generated):
public boolean equals(Object object) {
    if (this == object) return true;
    if (object == null) return false;
    if (object instanceof Tabela) {
        Tabela other = (Tabela) object;
        if (codigo == null || other.codigo == null) return false;
        return codigo.equals(other.codigo);
    } else return false;
}
public int hashCode() {
    return codigo.intValue();
}

These examples are applicable to a single id field in the generated entity class. Compound primary keys 
in the database that use identity classes instead of identity fields are just a bit more complex.

There are two wizards:
1) New Entity Classes from DB
2) New Entity Class

For 1, we generate Entity Classes and PK classes in the case of composite PKs. 
For this case, we require id fields and do not support generated id fields,
which means if a user wanted those, he'd need to modify the generated classes
anyway.  Shouldn't equals/hashCode in this case use the id fields?
Here's what's generated today with a single id field:
   public int hashCode() {
        int hash = 0;
        hash += (this.id != null ? this.id.hashCode() : 0);
        return hash;
    }

    public boolean equals(Object object) {
        // TODO: Warning - this method won't work in the case the id fields are
not set
        if (!(object instanceof Tabela)) {
            return false;
        }
        Tabela other = (Tabela)object;
        if (this.id != other.id && (this.id == null ||
!this.id.equals(other.id))) return false;
        return true;
    }

There are variations based on primitive vs. Object and composite pks.

For 2, we generate @GeneratedValue(strategy = GenerationType.AUTO), but we
generate the same type of methods as above, so perhaps that should change as
Craig suggested.

Should be fixed to 5.5

The problem is that I don't see agreement on how it should change.

Sorry for delay posting my opinion about latest comments.
I think having a not so good implementation is much better than having a bad
implementation.
Since the good points raised by everyone, I agree we should keep it as simple as
possible, and developers should implement as the way they want.
I have systems with about 1000 entity with the implementation I proposed, and
the system works fine, and I use LOTs of collections (either Lists, Iterators,
HashMaps, Arrays, and so on), and never found a case where the behaviour could
lead to mistakes.
Anyway, I'm only one, and my systems are only a fraction of the real world, and
I must agree they can't be the best example - I work on a highly controlled
environment, where I use my own implementations of models and sometimes even
collections - all taking in mind the rules defined for beans equality.
So, I'll ask just to enhance a little the equals method, giving it a better
"format", and make the hashCode to use the latest suggestion (return Object hash
when there is no PK - if I understood well the proposition).
IT: I completelly disagree with mrjdo. The proposal don't affect the way object
will be on a collection (even because, in their sample, the objects are
different). I'm higly inclined to support proposal 1 from craigmcc.

Sincerely, I'm not sure that this issue should be addressed, since it requests a change incompatible with what we had in
5.5 and 6.0. Perhaps what we need is a template for hashCode() and equals(), but that is not exactly trivial.

Also, although brviking's suggestion kind of makes sense to me, I don't know if it is what most people use. brviking,
could you support your proposal with links to articles, blogs, etc.?

I am not seeing any complaints about the current implementation of equals(), so closing the issue. It seems the current
implementation serves the needs of most people.