Liran Zelkha has posted comments on this change. Change subject: core: WIP: Adding JPA to ovirt ......................................................................
Patch Set 1: (11 comments) http://gerrit.ovirt.org/#/c/22806/1/backend/manager/modules/common/pom.xml File backend/manager/modules/common/pom.xml: Line 50: <dependency> Line 51: <groupId>org.hibernate</groupId> Line 52: <artifactId>hibernate-entitymanager</artifactId> Line 53: <version>4.3.0.Final</version> Line 54: </dependency> > You should add here explicitly the dependency for the artifact containing t Done Line 55: Line 56: </dependencies> Line 57: Line 58: <build> Line 50: <dependency> Line 51: <groupId>org.hibernate</groupId> Line 52: <artifactId>hibernate-entitymanager</artifactId> Line 53: <version>4.3.0.Final</version> Line 54: </dependency> > Is it a whitespace at the eol? Done Line 55: Line 56: </dependencies> Line 57: Line 58: <build> http://gerrit.ovirt.org/#/c/22806/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroup.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroup.java: Line 100: @Column(name = "trusted_service") Line 101: private boolean trustedService; Line 102: Line 103: @Column(name = "cluster_policy_id") Line 104: @Type(type = "org.ovirt.engine.core.dao.GuidMapper") > You are going to have to repeat this a lot, I guess. Maybe worth an @TypeDe Done Line 105: private Guid clusterPolicyId; Line 106: Line 107: @Transient Line 108: private String clusterPolicyName; http://gerrit.ovirt.org/#/c/22806/1/backend/manager/modules/dal/pom.xml File backend/manager/modules/dal/pom.xml: Line 87: <dependency> Line 88: <groupId>org.hibernate</groupId> Line 89: <artifactId>hibernate-entitymanager</artifactId> Line 90: <version>4.3.0.Final</version> Line 91: </dependency> > You should add here the dependency on the artifact containing the javax.per Done Line 92: </dependencies> Line 93: <build> Line 94: <filters> Line 95: <filter>src/test/filters/${engine.db}.properties</filter> http://gerrit.ovirt.org/#/c/22806/1/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: Line 33: Line 34: if (val instanceof Guid) Line 35: return new Guid(((Guid) val).getUuid()); Line 36: Line 37: throw new UnsupportedOperationException("Can't convert " + val.getClass() + " to GUID"); > Shouldn't this be HibernateException? Done Line 38: } Line 39: Line 40: @Override Line 41: public Serializable disassemble(Object val) throws HibernateException { Line 45: Line 46: if (val instanceof UUID) Line 47: return new Guid((UUID) val); Line 48: Line 49: throw new UnsupportedOperationException("Can't convert " + val.getClass() + " to GUID"); > Same as above. Done Line 50: } Line 51: Line 52: @Override Line 53: public boolean equals(Object x, Object y) throws HibernateException { http://gerrit.ovirt.org/#/c/22806/1/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 14: emf = Persistence.createEntityManagerFactory("ovirt"); Line 15: } Line 16: } Line 17: } Line 18: return emf.createEntityManager(); > This isn't a managed component (EJB, CDI bean, etc) so that injection isn't Done Line 19: } Line 20: Line 21: public static EntityManagerFactory getEntityManagerFactory() { Line 22: return emf; http://gerrit.ovirt.org/#/c/22806/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsGroupDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsGroupDAODbFacadeImpl.java: Line 26: * <code>VdsGroupDAODbFacadeImpl</code> provides an implementation of {@link VdsGroupDAO} that uses code previously Line 27: * found in {@link org.ovirt.engine.core.dal.dbbroker.DbFacade}. Line 28: * Line 29: */ Line 30: public class VdsGroupDAODbFacadeImpl extends HibernateFacade implements VdsGroupDAO { > In the past, when we had hibernate we used to have separate classes *DbFaca I want to do the change in a transitional way, so I'm not that will be possible... Line 31: @Override Line 32: public VDSGroup get(Guid id) { Line 33: return getEntityManager().find(VDSGroup.class, id); Line 34: } Line 40: CriteriaQuery<VDSGroup> query = cb.createQuery(VDSGroup.class); Line 41: Root<VDSGroup> group = query.from(VDSGroup.class); Line 42: query.where(cb.and(cb.equal(group.get("id"), id), Line 43: cb.and(cb.equal(group.get("user_id"), userID), Line 44: cb.equal(group.get("is_filtered"), isFiltered)))); > JPQL or (HQL) seems a better and less verbose alternative for this kind of I think it's more readable. Don't you think? Line 45: Line 46: return em.createQuery(query).getSingleResult(); Line 47: } Line 48: http://gerrit.ovirt.org/#/c/22806/1/backend/manager/modules/dal/src/main/resources/META-INF/persistence.xml File backend/manager/modules/dal/src/main/resources/META-INF/persistence.xml: Line 1: <persistence xmlns="http://java.sun.com/xml/ns/persistence"; Line 2: xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; Line 3: xsi:schemaLocation="http://java.sun.com/xml/ns/persistence http://java.sun.com/xml/ns/persistence/persistence_2_0.xsd"; Line 4: version="2.0"> Line 5: <persistence-unit name="ovirt"> > Where does this persistence unit gets the connections from? Don't you need True. It's still a work in progress. I focus on the test classes for now. Line 6: <provider>org.hibernate.jpa.HibernatePersistenceProvider</provider> Line 7: <class>org.ovirt.engine.core.common.businessentities.VDSGroup</class> Line 8: <properties> Line 9: <property name="hibernate.dialect" value="org.hibernate.dialect.PostgreSQLDialect" /> http://gerrit.ovirt.org/#/c/22806/1/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VdsGroupDAOTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VdsGroupDAOTest.java: Line 50: is = BaseDAOTestCase.class.getResourceAsStream("/test-database.properties"); Line 51: try { Line 52: properties.load(is); Line 53: } catch (Exception e) { Line 54: e.printStackTrace(); > Why are you catching this exception? Shouldn't this failure just fail the t Done Line 55: } Line 56: Line 57: Map props = new HashMap(); Line 58: props.put("javax.persistence.jdbc.driver", properties.getProperty("database.driver")); -- To view, visit http://gerrit.ovirt.org/22806 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cd0bbf9f0913955cb3e1facfa9a4bdc1f1ab24d Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa <gustavo.pedr...@eldorado.org.br> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> 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
