Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TRUNK-5492 Replace Person and Patient hbm mapping file with annotations #2900

Merged
merged 3 commits into from
May 29, 2019
Merged

TRUNK-5492 Replace Person and Patient hbm mapping file with annotations #2900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
60a03e6
TRUNK-5492 Replace Person hbm mapping file with annotations
wluyima20 May 9, 2019
de6fbfe
TRUNK-5492 Removed unnecessary tests and updated comments on BaseCont…
wluyima20 May 17, 2019
21f54ca
TRUNK-5492 Removed getter/setter overrides and changed access type to…
wluyima20 May 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions api/src/main/java/org/openmrs/Patient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,30 +11,87 @@

import java.util.ArrayList;
import java.util.Collection;
import java.util.Date;
import java.util.LinkedList;
import java.util.List;
import java.util.Set;
import java.util.TreeSet;

import javax.persistence.Access;
import javax.persistence.AccessType;
import javax.persistence.CascadeType;
import javax.persistence.Column;
import javax.persistence.Entity;
import javax.persistence.JoinColumn;
import javax.persistence.ManyToOne;
import javax.persistence.OneToMany;
import javax.persistence.PrimaryKeyJoinColumn;
import javax.persistence.Table;

import org.hibernate.annotations.SortNatural;
import org.hibernate.search.annotations.ContainedIn;
import org.hibernate.search.annotations.Field;

/**
* Defines a Patient in the system. A patient is simply an extension of a person and all that that
* implies.
*
* @version 2.0
*/
@Entity
@Table(name = "patient")
@PrimaryKeyJoinColumn(name = "patient_id")
public class Patient extends Person {

public static final long serialVersionUID = 93123L;

@Column(name = "patient_id", nullable = false, updatable = false, insertable = false)
private Integer patientId;

@Column(name = "allergy_status", length = 50)
private String allergyStatus = Allergies.UNKNOWN;

@OneToMany(mappedBy = "patient", cascade = CascadeType.ALL, orphanRemoval = true)
@SortNatural
@ContainedIn
private Set<PatientIdentifier> identifiers;

@ManyToOne
@JoinColumn(name = "creator", updatable = false)
@Access(AccessType.PROPERTY)
private User creator;

@Column(name = "date_created", nullable = false, updatable = false, length = 19)
@Access(AccessType.PROPERTY)
private Date dateCreated;

@Column(name = "voided", nullable = false)
@Access(AccessType.PROPERTY)
@Field
private Boolean voided = Boolean.FALSE;

@Column(name = "date_voided", length = 19)
@Access(AccessType.PROPERTY)
private Date dateVoided;

@ManyToOne
@JoinColumn(name = "voided_by")
@Access(AccessType.PROPERTY)
private User voidedBy;

@Column(name = "void_reason")
@Access(AccessType.PROPERTY)
private String voidReason;

@ManyToOne
@JoinColumn(name = "changed_by")
@Access(AccessType.PROPERTY)
private User changedBy;

@Column(name = "date_changed", length = 19)
@Access(AccessType.PROPERTY)
private Date dateChanged;

// Constructors

/** default constructor */
Expand Down Expand Up @@ -99,6 +156,9 @@ public Patient(Patient patient) {
* @return internal identifier for patient
*/
public Integer getPatientId() {
if (this.patientId == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind explaining why you needed to do this?

Copy link
Member Author

@wluyima wluyima May 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In-memory, this field is null when you create the patient record since it is technically mapped to a FK column which is why it is mapped as not insertable and not updatable, I wonder why hibernate was setting it with xml but not with annotations though, my guess is there could be a hibernate specific annotations to fix it that JPA doesn't support, will look into it a little a bit to see if there is another work around

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of adding this comment to the code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can but I see that you already merged the PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do it as a followup commit.

this.patientId = getPersonId();
}
return this.patientId;
}

Expand Down Expand Up @@ -400,4 +460,5 @@ public void setId(Integer id) {
public Person getPerson() {
return this;
}

}
73 changes: 73 additions & 0 deletions api/src/main/java/org/openmrs/Person.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,32 @@
import java.util.Set;
import java.util.TreeSet;

import javax.persistence.Basic;
import javax.persistence.CascadeType;
import javax.persistence.Column;
import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;
import javax.persistence.Inheritance;
import javax.persistence.InheritanceType;
import javax.persistence.JoinColumn;
import javax.persistence.ManyToOne;
import javax.persistence.OneToMany;
import javax.persistence.OrderBy;
import javax.persistence.Table;
import javax.persistence.Temporal;
import javax.persistence.TemporalType;
import javax.persistence.Transient;

import org.codehaus.jackson.annotate.JsonIgnore;
import org.hibernate.annotations.BatchSize;
import org.hibernate.annotations.Cache;
import org.hibernate.annotations.CacheConcurrencyStrategy;
import org.hibernate.annotations.Formula;
import org.hibernate.annotations.LazyCollection;
import org.hibernate.annotations.LazyCollectionOption;
import org.hibernate.annotations.SortNatural;
import org.hibernate.search.annotations.ContainedIn;
import org.hibernate.search.annotations.DocumentId;
import org.hibernate.search.annotations.Field;
Expand All @@ -37,60 +62,106 @@
*
* @see org.openmrs.Patient
*/
@Entity
@Table(name = "person")
@Inheritance(strategy = InheritanceType.JOINED)
public class Person extends BaseChangeableOpenmrsData {

public static final long serialVersionUID = 2L;

private static final Logger log = LoggerFactory.getLogger(Person.class);

@Id
@GeneratedValue(strategy = GenerationType.AUTO)
@Column(name = "person_id")
@DocumentId
protected Integer personId;

@OneToMany(mappedBy = "person", cascade = CascadeType.ALL, orphanRemoval = true)
@LazyCollection(LazyCollectionOption.FALSE)
@SortNatural
@OrderBy("voided asc, preferred desc, date_created desc")
@Cache(usage = CacheConcurrencyStrategy.READ_WRITE)
@BatchSize(size = 1000)
private Set<PersonAddress> addresses = null;

@OneToMany(mappedBy = "person", cascade = CascadeType.ALL, orphanRemoval = true)
@LazyCollection(LazyCollectionOption.FALSE)
@SortNatural
@OrderBy("voided asc, preferred desc, date_created desc")
@Cache(usage = CacheConcurrencyStrategy.READ_WRITE)
@BatchSize(size = 1000)
@ContainedIn
private Set<PersonName> names = null;

@OneToMany(mappedBy = "person", cascade = CascadeType.ALL, orphanRemoval = true)
@LazyCollection(LazyCollectionOption.FALSE)
@SortNatural
@Cache(usage = CacheConcurrencyStrategy.READ_WRITE)
@BatchSize(size = 1000)
@ContainedIn
private Set<PersonAttribute> attributes = null;

@Field
@Column(length = 50)
private String gender;

@Column(name = "birthdate", length = 10)
private Date birthdate;

@Basic
@Temporal(TemporalType.TIME)
private Date birthtime;

@Column(name = "birthdate_estimated")
private Boolean birthdateEstimated = false;

@Column(name = "deathdate_estimated")
private Boolean deathdateEstimated = false;

@Field
@Column(nullable = false)
private Boolean dead = false;

@Column(name = "death_date", length = 19)
private Date deathDate;

@ManyToOne
@JoinColumn(name = "cause_of_death")
private Concept causeOfDeath;

@Column(name = "cause_of_death_non_coded")
private String causeOfDeathNonCoded;

@ManyToOne
@JoinColumn(name = "creator", updatable = false, insertable = false)
private User personCreator;

@Column(name = "date_created", updatable = false, insertable = false, nullable = false, length = 19)
private Date personDateCreated;

@ManyToOne
@JoinColumn(name = "changed_by", updatable = false, insertable = false)
private User personChangedBy;

@Column(name = "date_changed", updatable = false, insertable = false, length = 19)
private Date personDateChanged;

@Column(name = "voided", updatable = false, insertable = false, nullable = false)
private Boolean personVoided = false;

@ManyToOne
@JoinColumn(name = "voided_by", updatable = false, insertable = false)
private User personVoidedBy;

@Column(name = "date_voided", updatable = false, insertable = false, length = 19)
private Date personDateVoided;

@Column(name = "void_reason", updatable = false, insertable = false)
private String personVoidReason;

@Field
@Formula("case when exists (select * from patient p where p.patient_id = person_id) then 1 else 0 end")
private boolean isPatient;

/**
Expand All @@ -99,8 +170,10 @@ public class Person extends BaseChangeableOpenmrsData {
* This is "cached" for each user upon first load. When an attribute is changed, the cache is
* cleared and rebuilt on next access.
*/
@Transient
Map<String, PersonAttribute> attributeMap = null;

@Transient
private Map<String, PersonAttribute> allAttributeMap = null;

/**
Expand Down
2 changes: 1 addition & 1 deletion api/src/main/java/org/openmrs/api/db/hibernate/HibernatePatientDAO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,7 @@ public boolean isIdentifierInUseByAnotherPatient(PatientIdentifier patientIdenti
&& patientIdentifier.getIdentifierType().getUniquenessBehavior() == UniquenessBehavior.LOCATION;

// switched this to an hql query so the hibernate cache can be considered as well as the database
String hql = "select count(*) from PatientIdentifier pi, Patient p where pi.patient.patientId = p.patient.patientId "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bug fix? Or is it a change caused by switching to annotations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug fix because it was wrong before, annotations just exposed it, p is an alias for patient, and patient for sure doesn't have a patient field

String hql = "select count(*) from PatientIdentifier pi, Patient p where pi.patient.patientId = p.patientId "
+ "and p.voided = false and pi.voided = false and pi.identifier = :identifier and pi.identifierType = :idType";

if (checkPatient) {
Expand Down
6 changes: 6 additions & 0 deletions api/src/main/java/org/openmrs/api/handler/PatientSaveHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.openmrs.User;
import org.openmrs.annotation.Handler;
import org.openmrs.aop.RequiredDataAdvice;
import org.openmrs.api.context.Context;

/**
* This class deals with {@link Patient} objects when they are saved via a save* method in an
Expand Down Expand Up @@ -45,5 +46,10 @@ public void handle(Patient patient, User creator, Date dateCreated, String other
}
}
}

if(patient.getPatientId() == null){
patient.setCreator(Context.getAuthenticatedUser());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bug fix? Or is it a change caused by switching to annotations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really but sort of, more like bad design, this is needed because creator and dateCreated fields exist both in BaseOpenmrsData and Patient, this is actually bad practice because it defeats the purpose of inheritance, and this where xml and annotations differ, because annotations are pure java whereas xml isn't, with xml you get away with bad things just like we were doing before which is why I say sort of a bug.

Anyways, long story short, hibernate is unable to set these fields via the AuditableInterceptor mechanism because based on the ClassMedata it has built for Patient class, it sees the same fields on the superclass and the subclass and treats them as different because they are declared by different classes anyways, it only sets the values of the fields in the superclass and not the subclass so we have to manually set them from here as we used to do before to get around it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of adding this comment to the code?

patient.setDateCreated(new Date());
}
}
}
10 changes: 8 additions & 2 deletions api/src/main/java/org/openmrs/api/handler/PersonSaveHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@
package org.openmrs.api.handler;

import java.util.Date;
import java.util.HashSet;
import java.util.Set;

import org.openmrs.Address;
import org.openmrs.Person;
import org.openmrs.PersonAddress;
import org.openmrs.PersonAttribute;
Expand Down Expand Up @@ -42,13 +45,16 @@ public void handle(Person person, User creator, Date dateCreated, String other)

// address collection
if (person.getAddresses() != null && !person.getAddresses().isEmpty()) {
Set<Address> blankAddresses = new HashSet<>();
for (PersonAddress pAddress : person.getAddresses()) {
if (pAddress.isBlank()){
person.removeAddress(pAddress);
if (pAddress.isBlank()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bug fix? Or is it a change caused by switching to annotations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a bug, it's java 101, you can't remove an element from the same Iterable you are looping over, you get a ConcurrentModificationException

blankAddresses.add(pAddress);
continue;
}
pAddress.setPerson(person);
}

person.getAddresses().removeAll(blankAddresses);
}

// name collection
Expand Down
2 changes: 0 additions & 2 deletions api/src/main/resources/hibernate.cfg.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
<mapping resource="org/openmrs/api/db/hibernate/FormResource.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/GlobalProperty.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/Obs.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/Person.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/PersonAttribute.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/PersonAttributeType.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/PersonAddress.hbm.xml" />
Expand All @@ -62,7 +61,6 @@
<mapping resource="org/openmrs/api/db/hibernate/LoginCredential.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/Privilege.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/Role.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/Patient.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/PatientIdentifier.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/PatientIdentifierType.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/PatientProgram.hbm.xml" />
Expand Down
60 changes: 0 additions & 60 deletions api/src/main/resources/org/openmrs/api/db/hibernate/Patient.hbm.xml
View file
Open in desktop

This file was deleted.

Loading