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

BAH-273 #27

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

BAH-273 #27

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
077b026
Adjusted the openmrs dependacy to master version
ningosi Feb 14, 2018
b368e4d
Added correct dependancy of 2.1.3-SNAPSHOT
ningosi Feb 19, 2018
727a353
Initial work on BAH-273 lucerne search for patients(registration app)
ningosi Feb 20, 2018
f028cd1
BAH-273: Using OpenMRS Core Lucene API in PatientDaoImpl (registratio…
ningosi Feb 20, 2018
83ccc4c
Revert BahmniPatientDaoImplLuceneIT.java
ningosi Feb 21, 2018
bfe7cdf
Set global properties `patientIdentifierSearch.matchMode` in liquibas…
ningosi Feb 21, 2018
c2805cc
Autowired the service instead of using openmrs context to call it
ningosi Feb 21, 2018
964089b
Merge branch 'BAH-273' of https://github.com/ningosi/bahmni-core into…
ningosi Feb 21, 2018
3e344c9
Setting the gp value since openmrs already creates the property
ningosi Feb 22, 2018
8d7d20c
Autowire service class in the current class constructure
ningosi Feb 22, 2018
0cac042
Setting patientSearch.matchMode and person.attributeSearchMatchMode t…
ningosi Feb 28, 2018
ac2de0a
Include all the search possible parameters in one lucene method
ningosi Mar 2, 2018
ac0eda1
Added unit tests to verify the lucene search functionality
ningosi Mar 7, 2018
dfeb59a
Standard naming of tests nethods after review
ningosi Mar 12, 2018
67d3db7
Adding combination of lucene and hibernate query search
ningosi Apr 4, 2018
0cc1d8a
Added hibernate search annotations on the BahmniAddress and PatientPr…
ningosi Apr 4, 2018
bf119fe
Added combination of the 2 searches
ningosi Apr 4, 2018
5e56b8d
Add tests to confirm intersection of lucene and hibernateyy
ningosi Apr 13, 2018
6c6e16d
Address PR review initial comments
ningosi Apr 25, 2018
724d06a
Merge branch 'master' of https://github.com/Bahmni/bahmni-core into B…
ningosi Apr 25, 2018
d45ab53
Change the platform version to 2.1.3 release version
ningosi May 14, 2018
0b9563e
Add a filetr to the preconditions to only return raws that are NOT ha…
ningosi May 14, 2018
62c1f5a
Clean up blank lines and organize imports
ningosi May 14, 2018
b1ab8fa
Removing @Indexed from non hibernate class and reorgarnize imports
ningosi May 14, 2018
6798949
Autowiring classes in the constructors
ningosi May 14, 2018
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
47 changes: 31 additions & 16 deletions bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/PatientDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@
import org.openmrs.PatientIdentifier;
import org.openmrs.PatientIdentifierType;
import org.openmrs.RelationshipType;
import org.openmrs.api.PatientService;
import org.openmrs.api.context.Context;
import org.openmrs.module.bahmniemrapi.visitlocation.BahmniVisitLocationServiceImpl;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.stereotype.Repository;
import java.util.ArrayList;
import java.util.Arrays;
Expand All @@ -42,10 +44,12 @@
public class PatientDaoImpl implements PatientDao {

private SessionFactory sessionFactory;
private PatientService patientService;

@Autowired
public PatientDaoImpl(SessionFactory sessionFactory) {
public PatientDaoImpl(SessionFactory sessionFactory, PatientService patientService) {
this.sessionFactory = sessionFactory;
this.patientService = patientService;
}

@Override
Expand Down Expand Up @@ -77,24 +81,35 @@ public List<PatientResponse> getPatientsUsingLuceneSearch(String identifier, Str
String programAttributeFieldName, String[] addressSearchResultFields,
String[] patientSearchResultFields, String loginLocationUuid,
Boolean filterPatientsByLocation, Boolean filterOnAllIdentifiers) {

validateSearchParams(customAttributeFields, programAttributeFieldName, addressFieldName);

List<PatientIdentifier> patientIdentifiers = getPatientIdentifiers(identifier, filterOnAllIdentifiers, offset, length);
List<Integer> patientIds = patientIdentifiers.stream().map(patientIdentifier -> patientIdentifier.getPatient().getPatientId()).collect(toList());
Map<Object, Object> programAttributes = Context.getService(BahmniProgramWorkflowService.class).getPatientProgramAttributeByAttributeName(patientIds, programAttributeFieldName);
Set<Patient> patients = new HashSet<>();

if(identifier != null && !identifier.isEmpty()){
Copy link

Choose a reason for hiding this comment

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

prefer to use org.apache.commons.lang3.StringUtils.isNotEmpty to combine both these checks.

Copy link

Choose a reason for hiding this comment

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

this whole block can be refactored so as not to duplicate the code blocks about calling name and customAttribute search.
Also, you're making the same patientService.getPatients(String, boolean, int, int) call all three of identifier, name, and customAttribute searching. Don't we need to somehow distinguish between what field we're searching for?

patients.addAll(patientService.getPatients(identifier, false, offset, length));
if(name != null && !name.isEmpty()){
patients.retainAll( patientService.getPatients(name, false, offset, length));
}
if(customAttribute != null && !customAttribute.isEmpty()){
patients.retainAll( patientService.getPatients(customAttribute, false, offset, length));
}
}else if(name != null && !name.isEmpty()){
patients.addAll(patientService.getPatients(name, false, offset, length));
if(customAttribute!=null && !customAttribute.isEmpty()){
patients.retainAll(patientService.getPatients(customAttribute, false, offset, length));
}
}else if((customAttribute != null && !customAttribute.isEmpty())){
patients.addAll(patientService.getPatients(customAttribute, false, offset, length));
}

List<Integer> patientIds = patients.stream().map(Patient::getPatientId).collect(toList());
PatientResponseMapper patientResponseMapper = new PatientResponseMapper(Context.getVisitService(),new BahmniVisitLocationServiceImpl(Context.getLocationService()));
Set<Integer> uniquePatientIds = new HashSet<>();
List<PatientResponse> patientResponses = patientIdentifiers.stream()
.map(patientIdentifier -> {
Patient patient = patientIdentifier.getPatient();
if(!uniquePatientIds.contains(patient.getPatientId())) {
PatientResponse patientResponse = patientResponseMapper.map(patient, loginLocationUuid, patientSearchResultFields, addressSearchResultFields,
programAttributes.get(patient.getPatientId()));
uniquePatientIds.add(patient.getPatientId());
return patientResponse;
}else
return null;
Map<Object, Object> programAttributes = Context.getService(BahmniProgramWorkflowService.class).getPatientProgramAttributeByAttributeName(patientIds, programAttributeFieldName);
Copy link

Choose a reason for hiding this comment

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

should autowire this service in the constructor instead of doing Context.getService


List<PatientResponse> patientResponses = patients.stream()
.map(patient -> {
PatientResponse patientResponse = patientResponseMapper.map(patient,loginLocationUuid,patientSearchResultFields,addressSearchResultFields,programAttributes.get(patient.getPatientId()));
return patientResponse;
}).filter(Objects::nonNull)
.collect(toList());
return patientResponses;
Expand Down
3 changes: 3 additions & 0 deletions bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/model/BahmniAddress.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package org.bahmni.module.bahmnicore.model;

import org.hibernate.search.annotations.Indexed;

import java.util.LinkedHashMap;

@Indexed
Copy link

Choose a reason for hiding this comment

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

Is this going to do anything here? This is not a hibernate managed class.

public class BahmniAddress {

private String address1;
Expand Down
5 changes: 5 additions & 0 deletions ...java/org/bahmni/module/bahmnicore/model/bahmniPatientProgram/PatientProgramAttribute.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
package org.bahmni.module.bahmnicore.model.bahmniPatientProgram;

import org.hibernate.search.annotations.DocumentId;
import org.hibernate.search.annotations.Indexed;
import org.openmrs.attribute.Attribute;
import org.openmrs.attribute.BaseAttribute;

@Indexed
public class PatientProgramAttribute extends BaseAttribute<ProgramAttributeType, BahmniPatientProgram> implements Attribute<ProgramAttributeType, BahmniPatientProgram> {

@DocumentId
private Integer patientProgramAttributeId;

@Override
Expand Down
9 changes: 9 additions & 0 deletions bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/BahmniPatientService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,13 @@ public interface BahmniPatientService {
public List<Patient> get(String partialIdentifier, boolean shouldMatchExactPatientId);

public List<RelationshipType> getByAIsToB(String aIsToB);

/**
* Intersection of lucene and hibernate search in one query
* First take advantage of lucene and fall back to hibernate
* @param searchParameters
* @return List of PatientResponse
*/
List<PatientResponse> luceneHibernateSearch(PatientSearchParameters searchParameters);
Copy link
Member

Choose a reason for hiding this comment

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

We need a very detailed Javadoc header for this one given the apparently confusing name.

Copy link
Author

Choose a reason for hiding this comment

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

Provided java doc for the method in the service class

Copy link

Choose a reason for hiding this comment

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

Why does this method name need to say this. It's possible that the under-the-hood implementation does in fact use lucene and then hibernate, but is that actually part of the interface contract? Isn't this just "searchBy" or something like that?


}
34 changes: 33 additions & 1 deletion ...api/src/main/java/org/bahmni/module/bahmnicore/service/impl/BahmniPatientServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.bahmni.module.bahmnicore.service.impl;

import org.apache.commons.lang3.StringUtils;
import org.bahmni.module.bahmnicore.contract.patient.PatientSearchParameters;
import org.bahmni.module.bahmnicore.contract.patient.response.PatientConfigResponse;
import org.bahmni.module.bahmnicore.contract.patient.response.PatientResponse;
Expand All @@ -10,11 +11,14 @@
import org.openmrs.PersonAttributeType;
import org.openmrs.RelationshipType;
import org.openmrs.api.ConceptService;
import org.openmrs.api.PatientService;
import org.openmrs.api.PersonService;
import org.openmrs.api.context.Context;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Lazy;
import org.springframework.stereotype.Service;

import java.util.ArrayList;
import java.util.List;

@Service
Expand All @@ -23,13 +27,15 @@ public class BahmniPatientServiceImpl implements BahmniPatientService {
private PersonService personService;
private ConceptService conceptService;
private PatientDao patientDao;
private PatientService patientService;

@Autowired
public BahmniPatientServiceImpl(PersonService personService, ConceptService conceptService,
PatientDao patientDao) {
PatientDao patientDao, PatientService patientService) {
this.personService = personService;
this.conceptService = conceptService;
this.patientDao = patientDao;
this.patientService = patientService;
}

@Override
Expand Down Expand Up @@ -93,4 +99,30 @@ public List<RelationshipType> getByAIsToB(String aIsToB) {
return patientDao.getByAIsToB(aIsToB);
}

@Override
public List<PatientResponse> luceneHibernateSearch(PatientSearchParameters searchParameters) {
Copy link

Choose a reason for hiding this comment

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

Comment: I find that the method naming here makes this very hard to understand. How would anyone possibly know what luceneSearch, lucenseHibernateSearch, and search are suppose to do, and which ones handle which parameters?
Can we deprecate and/or remove some methods? E.g. if the point of this ticket is to make a faster-but-otherwise-equivalent version of "search" then let's use that name for the new behavior. If some of that old code needs to go into a private method, that's fine.

List<PatientResponse> luceneHibernateResponse = new ArrayList<>();
if(StringUtils.isNotEmpty(searchParameters.getIdentifier()) || StringUtils.isNotEmpty(searchParameters.getName()) || StringUtils.isNotEmpty(searchParameters.getCustomAttribute())){
List<PatientResponse> luceneResponse = luceneSearch(searchParameters);

for(PatientResponse patientResponse: luceneResponse){
Patient patient = patientService.getPatient(patientResponse.getPersonId());
searchParameters.setIdentifier(patient.getPatientIdentifier().getIdentifier());
if(StringUtils.isNotEmpty(searchParameters.getName())){
searchParameters.setName("");
}
if( StringUtils.isNotEmpty(searchParameters.getCustomAttribute())){
searchParameters.setCustomAttribute("");
}
luceneHibernateResponse.addAll(search(searchParameters));
Copy link

Choose a reason for hiding this comment

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

You're firing a hibernate/sql search for every result that comes back from the lucene search? Are we guaranteed that the number of these searches will be small?
Can't we just do one lucene search, one hibernate search, and intersect them? (Or even include the lucene responses in a "patientId in :list" in the hibernate/sql query?
(And, have we verified that actually using lucene to search for the other parameters is that hard to do?)

}
}else{
luceneHibernateResponse = search(searchParameters);
}



return luceneHibernateResponse;
}

}
Loading