Liran Zelkha has posted comments on this change. Change subject: core: Add JPA Java infrastructure ......................................................................
Patch Set 57: (18 comments) https://gerrit.ovirt.org/#/c/33835/57/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/GuidMapper.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/GuidMapper.java: > 1. I'd like to have a unit test for this class 1. It's something internal in Hibernate. Not sure how you want to unit test it (type of objects really sent from hibernate, when each method is activated, etc) 2. OK 3. I tried it. Doesn't work for us so we use internal Guid and not Java UUID Line 1: package org.ovirt.engine.core.dao; Line 2: Line 3: import java.io.Serializable; Line 4: import java.sql.PreparedStatement; Line 16: GuidMapper > IMHO GuidUserType is a better name. Done Line 21: } Line 22: Line 23: @Override Line 24: public Object deepCopy(Object val) throws HibernateException { Line 25: if (val == null) > Please add curly brackets for all if statements in the class. Done Line 26: return null; Line 27: Line 28: if (val instanceof byte[]) { Line 29: return new Guid((byte[]) val, true); Line 24: public Object deepCopy(Object val) throws HibernateException { Line 25: if (val == null) Line 26: return null; Line 27: Line 28: if (val instanceof byte[]) { > IMHO having a lot of "instance of" clauses might impact server performance Removed from the code - so irrelevant Line 29: return new Guid((byte[]) val, true); Line 30: } Line 31: Line 32: if (val instanceof UUID) Line 22: : @Override : public Object deepCopy(Object val) throws HibernateException { : if (val == null) : return null; : : if (val instanceof byte[]) { : return new Guid((byte[]) val, true); : } : : if (val instanceof UUID) : return new Guid((UUID) val); : : if (val instanceof Guid) : return new Guid(((Guid) val).getUuid()); : : throw new HibernateException("Can't convert " + val.getClass() + " to GUID"); : } > According to the UserType.deepCopy javadoc and as soon as Guid objects are Done Line 40: : @Override : public Serializable disassemble(Object val) throws HibernateException { : if (val == null) { : return null; : } : : if (val instanceof byte[]) { : return new Guid((byte[]) val, true); : } : : if (val instanceof UUID) : return new Guid((UUID) val); : : if (val instanceof Guid) : return (Guid) val; : : throw new HibernateException("Can't convert " + val.getClass() + " to GUID"); : } > Same here - Guid is immutable , so "return val;" should be enough. Done Line 117: BLOB > IMHO that should be Types.OTHER, that's how PostgreSQL JDBC implemented. Done https://gerrit.ovirt.org/#/c/33835/57/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/HibernateFacade.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/HibernateFacade.java: Line 15: @author lzelkha > AFAIK adding @author is against some oVirt standard Done Line 21: extends BaseDAODbFacade > Please correct me if I'm wrong, but I do not see a good reason why this cla I wanted to keep old behaviour. If someone is casting the DAO to BaseDaoDbFacade - it should still work. That's how the inheritance tree is built right now. Line 18: * @param <K> Line 19: * The entity key Line 20: */ Line 21: public abstract class HibernateFacade<T, K extends Serializable> extends BaseDAODbFacade { Line 22: @PersistenceContext > From this class being abstract I assume that intended to be sub-classed by True. But this way trivial methods, like save, get, update, etc - are available automatically without writing any code. And writing unit tests is very easy - all the test infra is there. Line 23: protected EntityManager em; Line 24: Line 25: public void save(T entity) { Line 26: em.merge(entity); Line 50: protected > please make this method private Done Line 59: boolean detach > detach argument implies that the user of the method know what that means. s We need it as protected, as some subtypes might specifically want detached objects. Line 78: return multiResultsNoCast(query, false); Line 79: } Line 80: Line 81: @SuppressWarnings("rawtypes") Line 82: public List multiResultsNoCast(Query query, boolean detach) { > detach argument implies that the user of the method know what that means. s Same as above Line 83: List entities = query.getResultList(); Line 84: Line 85: if (detach) { Line 86: for (Object entity : entities) { Line 106: : private static Class<?> findSuperClassParameterType(Object instance, int parameterIndex) { : Class<?> subClass = instance.getClass(); : while (subClass.getSuperclass() != HibernateFacade.class) { : subClass = subClass.getSuperclass(); : if (subClass == null) : throw new IllegalArgumentException(); : } : : ParameterizedType parameterizedType = (ParameterizedType) subClass.getGenericSuperclass(); : return (Class<?>) parameterizedType.getActualTypeArguments()[parameterIndex]; : } > I'd prefer sub-classes would pass their entity type in the constructor rath Done https://gerrit.ovirt.org/#/c/33835/57/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/package-info.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/package-info.java: Line 1: value = > Specifiying the name of the argument for "value" argument is redundant Done Line 3: org.hibernate.annotations > please add import for TypeDef Done Line 3: guid_mapper > I guess the name should be just "guid" Done Line 4: org.ovirt.engine.core.dao.GuidMapper > please add import for GuidMapper Done -- To view, visit https://gerrit.ovirt.org/33835 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ide82bf8cc647426e37dc42a113867c52699c3f0b Gerrit-PatchSet: 57 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <wallaroo1...@gmail.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
