Yevgeny Zaspitsky 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 2. I'd create org.ovirt.engine.core.hibernate package for the stuff like this 3. Please have a look into org.hibernate.type.PostgresUUIDType class. That might be useful for us. 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. 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. 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 hardly. Why do we need support converting a single java type into multiple SQL types? Don't we use only UUID PostgerSQL type for that? If we really need that, I'd create 3 different UserType implementations and attach the appropriate definition for every usage. That way the decision will be at development time rather than in production run-time. 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 immutable the implementation of the method should be the oneliner - "return val;" . 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. Line 117: BLOB IMHO that should be Types.OTHER, that's how PostgreSQL JDBC implemented. 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 Line 21: extends BaseDAODbFacade Please correct me if I'm wrong, but I do not see a good reason why this class extends BaseDaoDbFacade. 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 a >concrete DAO class in order to consume its services. I'd prefer using this class by composition rather than inheritance. In that case we' need a singleton factory with the following interface: <T> HibernateFacade<> createHibernateFacade(Class<T> entityClass); The implementation would have EntityManager instance that will be passed through constructor to the created instance. The DAOs will obtain the instance of the HF in their intialization sequence (preferably constructor) by using the factory. That aproach will make HF mockable in the DAO tests and the tests easier. 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 Line 59: boolean detach detach argument implies that the user of the method know what that means. shouldn't this method be private. 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. shouldn't this method be private. 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 rather than doing Reflection tricks on every invocation. 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 Line 3: org.hibernate.annotations please add import for TypeDef Line 3: guid_mapper I guess the name should be just "guid" Line 4: org.ovirt.engine.core.dao.GuidMapper please add import for GuidMapper -- 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
