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

AO-22 Add support for Advanced Query Syntax #70

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

AO-22 Add support for Advanced Query Syntax #70

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
33accff
Advanced Search Initial Support
reubenvarghese1 Jun 13, 2019
a381e4a
Advanced Search Initial Support
reubenvarghese1 Jun 13, 2019
03ead9f
Advanced Search Initial Support
reubenvarghese1 Jun 14, 2019
6e88bf2
Advanced Search Initial Support
reubenvarghese1 Jun 14, 2019
3102de9
Advanced Search Initial Support
reubenvarghese1 Jun 14, 2019
e9ec7fe
Advanced Search Initial Support
reubenvarghese1 Jun 14, 2019
0706d4c
Advanced Search Initial Support
reubenvarghese1 Jun 14, 2019
fb9d5b6
Advanced Search Initial Support
reubenvarghese1 Jun 14, 2019
efdcda1
Advanced Search added regex checks
reubenvarghese1 Jun 14, 2019
613134d
Advanced Search added regex checks
reubenvarghese1 Jun 14, 2019
c289432
Retaining Advanced Query search
reubenvarghese1 Jun 14, 2019
d226200
Advanced Query search Fixes
reubenvarghese1 Jun 14, 2019
3259cd0
Advanced Query search Fixes
reubenvarghese1 Jun 14, 2019
ca0ca4d
Advanced Query search Fixes
reubenvarghese1 Jun 15, 2019
00f7783
Advanced Query Search Frontend Changes
reubenvarghese1 Jul 2, 2019
bdfcd55
Backend for Advanced Query Search
reubenvarghese1 Jul 3, 2019
f24fe19
Changes implemented partially
reubenvarghese1 Jul 8, 2019
0a5cca6
Changes implemented partially
reubenvarghese1 Jul 9, 2019
a6904ca
Trial
reubenvarghese1 Jul 11, 2019
0804487
Trial
reubenvarghese1 Jul 11, 2019
f635bc5
Trial
reubenvarghese1 Jul 11, 2019
a71f7f7
Trial
reubenvarghese1 Jul 11, 2019
c6cc91c
Trial
reubenvarghese1 Jul 12, 2019
80a93b4
Trial
reubenvarghese1 Jul 12, 2019
1a30516
Trial
reubenvarghese1 Jul 12, 2019
d8a3c47
Trial with case sensitivity
reubenvarghese1 Jul 12, 2019
4f30974
Trial with case sensitivity
reubenvarghese1 Jul 12, 2019
b4c6a57
Trial with case sensitivity
reubenvarghese1 Jul 12, 2019
828ec46
Trial with case sensitivity
reubenvarghese1 Jul 13, 2019
51755a1
Trial with case sensitivity
reubenvarghese1 Jul 13, 2019
069ed9f
Trial with case sensitivity
reubenvarghese1 Jul 13, 2019
62725a5
Trial with case sensitivity
reubenvarghese1 Jul 13, 2019
bf095a1
Fixed moduleid and uid error
reubenvarghese1 Jul 13, 2019
51e8adb
Fixed moduleid and uid error
reubenvarghese1 Jul 13, 2019
d41064c
Fixed moduleid and uid error
reubenvarghese1 Jul 13, 2019
f3cc839
Trial with case sensitivity
reubenvarghese1 Aug 8, 2019
bff4db5
Trial with case sensitivity
reubenvarghese1 Aug 8, 2019
418f919
Trial with case sensitivity
reubenvarghese1 Aug 8, 2019
db524ac
Trial with case sensitivity
reubenvarghese1 Aug 8, 2019
3b2ef2a
Trial with case sensitivity
reubenvarghese1 Aug 8, 2019
534f175
Trial with case sensitivity
reubenvarghese1 Aug 8, 2019
514b05f
Moved Parsing to SearchPage
reubenvarghese1 Aug 8, 2019
df696ba
Moved Parsing to SearchPage
reubenvarghese1 Aug 8, 2019
88cb630
Moved Parsing to SearchPage
reubenvarghese1 Aug 8, 2019
0e39fa3
Moved Parsing to SearchPage
reubenvarghese1 Aug 8, 2019
7ec8a3d
Moved Parsing to SearchPage
reubenvarghese1 Aug 8, 2019
f5fae33
Moved Parsing to SearchPage
reubenvarghese1 Aug 8, 2019
45506f9
Moved Parsing to SearchPage
reubenvarghese1 Aug 8, 2019
77e0a78
Moved Parsing to SearchPage
reubenvarghese1 Aug 8, 2019
f250080
Moved Parsing to SearchPage
reubenvarghese1 Aug 8, 2019
05d4ab8
Moved Parsing to SearchPage
reubenvarghese1 Aug 9, 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
3 changes: 2 additions & 1 deletion src/main/java/org/openmrs/addonindex/legacy/LegacyController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ public String findModules(
@RequestParam(value = "openmrs_version", required = false) String openmrsVersion,
@RequestParam(value = "excludeModule", required = false) List<String> excludeModuleIds
) throws Exception {
Collection<AddOnInfoSummary> results = index.search(AddOnType.OMOD, query, null);
Collection<AddOnInfoSummary> results = index.search(AddOnType.OMOD, query, null, null, null,
null, null);

LegacyFindModulesResponse response = new LegacyFindModulesResponse();
response.setsEcho(sEcho);
Expand Down
15 changes: 8 additions & 7 deletions src/main/java/org/openmrs/addonindex/rest/AddOnController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@

import java.util.Collection;

import org.openmrs.addonindex.domain.AddOnInfoAndVersions;
import org.openmrs.addonindex.domain.AddOnInfoSummary;
import org.openmrs.addonindex.domain.AddOnType;
import org.openmrs.addonindex.domain.AddOnVersion;
import org.openmrs.addonindex.domain.*;
Copy link
Member

Choose a reason for hiding this comment

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

Avoid wildcard imports. (Can do this in your IDE settings.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry about that, I've now disabled it in my IDE settings

import org.openmrs.addonindex.service.Index;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
Expand All @@ -38,9 +35,13 @@ public AddOnController(Index index) {

@RequestMapping(method = RequestMethod.GET, value = "/api/v1/addon")
public Collection<AddOnInfoSummary> search(@RequestParam(value = "type", required = false) AddOnType type,
@RequestParam(value = "q", required = false) String query,
@RequestParam(value = "tag", required = false) String tag) throws Exception {
return index.search(type, query, tag);
@RequestParam(value = "q", required = false) String query,
@RequestParam(value = "tag", required = false) String tag,
@RequestParam(value = "moduleid", required = false) String uid,
@RequestParam(value = "name", required = false) String name,
@RequestParam(value = "exclude", required = false) String excludeText,
@RequestParam(value = "status", required = false) AddOnStatus status) throws Exception {
return index.search(type, query, tag, uid, name, excludeText, status);
}

@RequestMapping(method = RequestMethod.GET, value = "/api/v1/addon", params = "modulePackage")
Expand Down
25 changes: 20 additions & 5 deletions src/main/java/org/openmrs/addonindex/service/ElasticSearchIndex.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,7 @@
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.search.builder.SearchSourceBuilder;
import org.elasticsearch.search.sort.SortOrder;
import org.openmrs.addonindex.domain.AddOnInfoAndVersions;
import org.openmrs.addonindex.domain.AddOnInfoSummary;
import org.openmrs.addonindex.domain.AddOnInfoSummaryAndStats;
import org.openmrs.addonindex.domain.AddOnType;
import org.openmrs.addonindex.domain.*;
Copy link
Member

Choose a reason for hiding this comment

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

same

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -98,8 +95,21 @@ public void index(AddOnInfoAndVersions infoAndVersions) throws IOException {
}

@Override
public Collection<AddOnInfoSummary> search(AddOnType type, String query, String tag) throws IOException {
public Collection<AddOnInfoSummary> search(AddOnType type, String query, String tag, String uid,
String name, String exclude, AddOnStatus status) throws IOException {
BoolQueryBuilder boolQB = QueryBuilders.boolQuery();
if (name != null){
Copy link
Member

Choose a reason for hiding this comment

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

formatting :-) should be

if (clause) {

//Exact match on moduleID
Copy link
Member

Choose a reason for hiding this comment

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

copy-paste error. this is name, not module id

boolQB.filter(QueryBuilders.matchQuery("name.raw", name));
Copy link
Member

Choose a reason for hiding this comment

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

this should be case-insensitive.
probably all of them should be. Are they?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, all are case-insensitive. Keywords are by default case-sensitive. To make this field case-insensitive is why I added a normalizer to it(this feature is only available starting v5.2 of elasticsearch. So, I guess we should update the ReadME to include this)

Copy link
Member

Choose a reason for hiding this comment

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

Please update the README. Is that about also sourcing the -setting.json file, or do we also need to update the deployment scripts to use a later version of elasticsearch?

}
if (uid != null){
//Exact match on moduleID
boolQB.filter(QueryBuilders.matchQuery("moduleId", uid));
}
if (status != null){
//Exact match on status
boolQB.filter(QueryBuilders.matchQuery("status", status));
}
if (type != null) {
//Exact match on type
boolQB.filter(QueryBuilders.matchQuery("type", type));
Expand Down Expand Up @@ -127,6 +137,11 @@ public Collection<AddOnInfoSummary> search(AddOnType type, String query, String
boolQB.should(QueryBuilders.matchPhraseQuery("name", query).slop(2).fuzziness("AUTO"));
boolQB.minimumNumberShouldMatch(1);
}

if (exclude != null){
boolQB.mustNot(QueryBuilders.multiMatchQuery(exclude , "name", "tags", "description",
"moduleId", "status", "_id", "type"));
}

BoostingQueryBuilder boostingQB = QueryBuilders.boostingQuery();
boostingQB.positive(boolQB);
Expand Down
8 changes: 3 additions & 5 deletions src/main/java/org/openmrs/addonindex/service/Index.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,7 @@
import java.util.Collection;
import java.util.List;

import org.openmrs.addonindex.domain.AddOnInfoAndVersions;
import org.openmrs.addonindex.domain.AddOnInfoSummary;
import org.openmrs.addonindex.domain.AddOnInfoSummaryAndStats;
import org.openmrs.addonindex.domain.AddOnType;
import org.openmrs.addonindex.domain.*;
Copy link
Member

Choose a reason for hiding this comment

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

same


/**
* Interface for accessing the datastore where we have indexed information about add-ons and versions.
Expand All @@ -25,7 +22,8 @@ public interface Index {

void index(AddOnInfoAndVersions infoAndVersions) throws Exception;

Collection<AddOnInfoSummary> search(AddOnType type, String query, String tag) throws Exception;
Collection<AddOnInfoSummary> search(AddOnType type, String query, String tag, String uid,
String name, String exclude, AddOnStatus status) throws Exception;

Collection<AddOnInfoAndVersions> getAllByType(AddOnType type) throws Exception;

Expand Down
7 changes: 6 additions & 1 deletion src/main/resources/elasticsearch/addOnInfoAndVersions-mappings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,12 @@
"type": "keyword"
},
"name": {
"type": "text"
"type": "text",
"fields": {
"raw": {
"type": "keyword"
}
}
},
"description": {
"type": "text"
Expand Down
77 changes: 30 additions & 47 deletions src/main/ui/app/component/SearchBox.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import {Component} from "react";
import {withRouter} from "react-router";
import {Button, Col, Form, FormControl, InputGroup} from "react-bootstrap";
import searchQuery from "search-query-parser";

class SearchBox extends Component {

Expand All @@ -20,68 +21,50 @@ class SearchBox extends Component {
}

setQuery(query) {
this.setState({ query: query }, function() {
this.parseQuery();
})}

setSplitQuery(splitQuery) {
this.setState({
splitQuery: splitQuery
query: query
});
}

parseQuery(){
if (this.state.query){
let advancedQuery = this.state.query;
doSearch() {
if (this.state.query) {
let advancedQuery = this.state.query.toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

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

bad style to call this variable advancedQuery if you don't know if it's advanced or not.
Does lowercasing it possibly mess up exact matching?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Typically, lowercasing would mess it up as the keyword field is case sensitive. However, fortunately, only our name field is having a mix of case. The rest of the fields are all lowercased. Even while performing exact matching on the name field, I reckon that both "Observations Administration" and "observations administration" should produce the same result. I have ensured this by adding a normalizer to the name field. This normalizer basically helps make the keyword field case-insensitive

//Basic Regex Matching to fix query inconsistencies
//Removing all extra spaces i.e. two or more
advancedQuery = advancedQuery.replace(/\s+/g,' ').trim();
//Removing all spaces to the left of search keys
advancedQuery = advancedQuery.replace(new RegExp("\\s+:","g"),":");
//Removing all spaces to the right of search keys
advancedQuery = advancedQuery.replace(new RegExp(":\\s+","g"),":");
let queryComponents = {};
//Determining query type
if(advancedQuery.includes(":")){
let querySplit = advancedQuery.split(' ');
let tempQuery = "";
querySplit.forEach(m => {
if (m.includes(":")){
let [key, value] = m.split(':');
if (key === "type"){
queryComponents["type"] = value.toUpperCase();
let url = "/search?";
// Check if query is an advanced query. We want to use the Parser only if the query is advanced
if (advancedQuery.includes(":") || advancedQuery.includes("-")){
Copy link
Member

Choose a reason for hiding this comment

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

I think we want a way for the user to include a : or a - in the query. Can we document what it is in the help text? (Maybe not even so important to support this.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So do you mean to say that every query should have an advanced query like structure? IMO, certain users just prefer searching for text instead of having to worry about the structure of the advanced query and I believe we should continue to support that. Those who wish to go advanced will also have the option to do so. However, should you feel that we should do away with a simple query, do let me know and I will implement it :)

let options = {keywords: ['type', 'tag', 'query', 'moduleid', 'status', 'name'], offsets: false};
Copy link
Member

Choose a reason for hiding this comment

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

At a minimum, pull this out to wider scope as a const. Even better would be to introduce a class that encapsulates all of the parsing and construction of the URL.

let searchQueryObj = searchQuery.parse(advancedQuery, options);
Object.keys(searchQueryObj).forEach(function (key) {
if (searchQueryObj[key]){
if (key === "text" || key === "query") {
url += "&q=" + searchQueryObj[key];
}
else if (key === "tag") {
queryComponents["tag"] = value.toLowerCase();
else if (key === "type"){
url += "&type=" + searchQueryObj[key].toUpperCase();
}
else if (key === "exclude"){
if (searchQueryObj[key].text){
url += "&exclude=" + searchQueryObj[key].text;
}
}
else if (key === "status"){
Copy link
Member

Choose a reason for hiding this comment

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

you're handling "type" and "status" in the same way, can you combine these branches?

url += "&status=" + searchQueryObj[key].toUpperCase();
}
else {
url += "&" + key + "=" + searchQueryObj[key];
}
}
else {
tempQuery = tempQuery + ' ' + m;
queryComponents["query"] = tempQuery;
}
});
}
else{
queryComponents["query"] = advancedQuery;
}
this.setSplitQuery(queryComponents);
}
}

doSearch() {
if (this.state.splitQuery) {
let url = "/search?";

if (this.state.splitQuery.type) {
url += "type=" + this.state.splitQuery.type;
}

if (this.state.splitQuery.query) {
url += "&q=" + this.state.splitQuery.query;
});
}

if (this.state.splitQuery.tag) {
url += "&tag=" + this.state.splitQuery.tag;
else {
url += "&q=" + advancedQuery;
}

this.props.router.push(url);
Expand Down
33 changes: 21 additions & 12 deletions src/main/ui/app/route/SearchPage.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,16 @@ export default class SearchPage extends Component {
}

doSearch() {
const advancedQuery = this.props.location.query;
const query = this.props.location.query.q;
const type = this.props.location.query.type;
const tag = this.props.location.query.tag;
const searchKey = `type:${type ? type : "all"} query:${query ? query : ""} tag:${tag ? tag : ""}`;
const tag = this.props.location.query.tag;
const exclude = this.props.location.query.exclude;
const moduleid = this.props.location.query.moduleid;
const name = this.props.location.query.name;
const status = this.props.location.query.status;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to enumerate all of these here? Can't they be encapsulated within SearchBox? E.g. this class only needs to care about a single search string ("query"), right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry you had to see that. It was very foolish of me to do so :)


const searchKey = `${type ? "type:" + type : ""} ${query ? "query:" + query : ""} ${tag ? "tag:" + tag : ""} ${exclude ? "-" + exclude : ""} ${name ? "name:" + name : ""} ${status ? "status:" + status : ""} ${moduleid ? "moduleId:" + moduleid : ""}`;

if (this.state.latestSearch === searchKey) {
return;
Expand All @@ -43,15 +49,13 @@ export default class SearchPage extends Component {
this.handleStartSearch(searchKey);

let url = "/api/v1/addon?";
if (type) {
url += "type=" + type;
}
if (query) {
url += "&q=" + query;
}
if (tag) {
url += "&tag=" + tag;
Copy link
Member

Choose a reason for hiding this comment

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

This change implies there should be a change to the URL of the link for searching by tag, right? I don't see it changed in this PR.

}

Object.keys(advancedQuery).forEach(function (key) {
if (advancedQuery[key]){
url += ("&" + key + "=" + advancedQuery[key]);
}
});

fetch(url)
.then(response => {
return response.json();
Expand Down Expand Up @@ -80,13 +84,18 @@ export default class SearchPage extends Component {
let simpleQuery = this.props.location.query.q;
let moduleType = this.props.location.query.type;
let moduleTag = this.props.location.query.tag;
let moduleExclude = this.props.location.query.exclude;
let moduleid = this.props.location.query.moduleid;
let moduleName = this.props.location.query.name;
let moduleStatus = this.props.location.query.status;

if (this.state && this.state.error) {
return <div>{this.state.error}</div>
}
else {
return (
<div>
<SearchBox initialQuery={`type:${moduleType ? moduleType : "all"} query:${simpleQuery ? simpleQuery : ""} tag:${moduleTag ? moduleTag : ""}`}/>
<SearchBox initialQuery={`${moduleType ? "type:" + moduleType : ""} ${simpleQuery ? "query:" + simpleQuery : ""} ${moduleTag ? "tag:" + moduleTag : ""} ${moduleExclude ? "-" + moduleExclude : ""} ${moduleName ? "name:" + moduleName : ""} ${moduleStatus ? "status:" + moduleStatus : ""} ${moduleid ? "moduleId:" + moduleid : ""}`}/>

{this.state.latestSearch ?
<div>Searching for {this.state.latestSearch}</div>
Expand Down
25 changes: 15 additions & 10 deletions src/main/ui/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading