Martin Mucha has posted comments on this change. Change subject: core: Introduce NetworkAttachment entity ......................................................................
Patch Set 19: (2 comments) https://gerrit.ovirt.org/#/c/32411/19/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/NetworkAttachment.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/NetworkAttachment.java: Line 107: return false; Line 108: if (getClass() != obj.getClass()) Line 109: return false; Line 110: NetworkAttachment other = (NetworkAttachment) obj; Line 111: if (id == null) { > with the transition to ORM all we should care of is the real identifiers of "with the transition to ORM all we should care of is the real identifiers of this class, which is id" — that's considered to be not true. Since we(at least I don't) do not know (yet?), which operations will be performed on entities, we cannot tell, what we have to compare. Namely, we have to know first, whether these entities will be processed also with unset id when stored in collections, if so, then comparing just ids spells crazy results. Usually it's recommended to compare some unique business id instead of or along with db ID. See Hibernate recommendation related to id generation / object identity. Ok, I'll regenerate equals&hashcode using these two fields. Note: this syntax is Java7 syntax, with direct field access, allowing comparing classes with their descendants (the last thing is necessary due to hibernate design). Line 112: if (other.id != null) Line 113: return false; Line 114: } else if (!id.equals(other.id)) Line 115: return false; Line 107: return false; Line 108: if (getClass() != obj.getClass()) Line 109: return false; Line 110: NetworkAttachment other = (NetworkAttachment) obj; Line 111: if (id == null) { > I'm not generating the equals method. IMO, it is more readable to use Objec ad generating equals: with java7 syntax is easier to do it manually, but why to waste time on that, if IDE(at least intellij) does it for you flawlessly, every time, in secs. I understand, that changing property would mean that this object is different and thus nonexistant in hashset. But that does not make a bug. We have to find out, what should be correct identity of object and implement it. Actually all those caching *is* probably an actual problem. I'll try to lookaround for usages. But I think, this should be stated clearly without it. Is any two cars same because they has same number on plate? If we care only about that number, they are. We should be able to identify non-transient data in NetworkAttachment and create equals/hashcode with this knowledge only. I don't think properties are transient data. That would mean, that we can use instead of them different properties from any other equal NetworkAttachment, which (for example during update) is simply not true. Line 112: if (other.id != null) Line 113: return false; Line 114: } else if (!id.equals(other.id)) Line 115: return false; -- To view, visit https://gerrit.ovirt.org/32411 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ided81dc2be68dc4c7a9d491697f887cdae477a2c Gerrit-PatchSet: 19 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
