AO-22 Add support for Advanced Query Syntax #70

reubenvarghese1 · 2019-06-14T11:41:14Z

This work was undertaken as part of a GSoC 2019 project. More details of the same may be found here

reubenvarghese1 · 2019-06-14T11:45:40Z

@djazayeri @Ruhanga please find an initial commit towards implementing AO 22. Do let me know of any changes that you find may be necessary(including the approach itself).

djazayeri · 2019-06-18T06:29:26Z

High-level comment: avoid hand-coding each individual advanced parameter. I would expect that most of the supported things map directly to one field in the elasticsearch index, so I would define a js object giving these mappings, and have some code that finds things before the : and checks if they're keys on that object.

djazayeri · 2019-06-18T06:34:11Z

Also, I hope we can support some common things like "exact phrase" or -exclude. Can you find an existing advanced query parsing library we can use? A quick google search came up with this: https://github.com/nepsilon/search-query-parser (though there may be better ones).

djazayeri

Where are your tests? :-)

djazayeri · 2019-07-03T23:49:48Z

src/main/java/org/openmrs/addonindex/rest/AddOnController.java

-import org.openmrs.addonindex.domain.AddOnInfoSummary;
-import org.openmrs.addonindex.domain.AddOnType;
-import org.openmrs.addonindex.domain.AddOnVersion;
+import org.openmrs.addonindex.domain.*;


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

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

djazayeri · 2019-07-03T23:50:41Z

src/main/java/org/openmrs/addonindex/service/ElasticSearchIndex.java

-import org.openmrs.addonindex.domain.AddOnInfoSummary;
-import org.openmrs.addonindex.domain.AddOnInfoSummaryAndStats;
-import org.openmrs.addonindex.domain.AddOnType;
+import org.openmrs.addonindex.domain.*;


djazayeri · 2019-07-03T23:51:42Z

src/main/java/org/openmrs/addonindex/service/ElasticSearchIndex.java

 		BoolQueryBuilder boolQB = QueryBuilders.boolQuery();
+		if (name != null){


formatting :-) should be

if (clause) {

djazayeri · 2019-07-03T23:53:02Z

src/main/java/org/openmrs/addonindex/service/ElasticSearchIndex.java

 		BoolQueryBuilder boolQB = QueryBuilders.boolQuery();
+		if (name != null){
+			//Exact match on moduleID


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

djazayeri · 2019-07-03T23:53:58Z

src/main/java/org/openmrs/addonindex/service/Index.java

-import org.openmrs.addonindex.domain.AddOnInfoSummary;
-import org.openmrs.addonindex.domain.AddOnInfoSummaryAndStats;
-import org.openmrs.addonindex.domain.AddOnType;
+import org.openmrs.addonindex.domain.*;


djazayeri · 2019-07-03T23:56:27Z

src/main/ui/app/component/SearchBox.jsx

-            let advancedQuery = this.state.query;
+    doSearch() {
+        if (this.state.query) {
+            let advancedQuery = this.state.query.toLowerCase();


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?

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

djazayeri · 2019-07-03T23:59:21Z

src/main/ui/app/component/SearchBox.jsx

+            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("-")){
+                let options = {keywords: ['type', 'tag', 'query', 'moduleid', 'status', 'name'], offsets: false};


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.

djazayeri · 2019-07-04T00:00:36Z

src/main/ui/app/component/SearchBox.jsx

-                            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("-")){


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.)

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 :)

djazayeri · 2019-07-04T00:03:40Z

src/main/ui/app/route/SearchPage.jsx

+        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;


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?

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

djazayeri · 2019-07-15T05:00:04Z

src/main/ui/app/component/SearchBox.jsx

+            //Removing all spaces to the left of search keys
+            query = query.replace(new RegExp("\\s+:","g"),":");
+            //Removing all spaces to the right of search keys
+            query = query.replace(new RegExp(":\\s+","g"),":");


Can you pull all this into a helper function like normalizeQuery()?

djazayeri

Made some comments. Also, this needs tests.

djazayeri · 2019-07-15T05:06:20Z

src/main/ui/app/component/SearchBox.jsx

-            if (this.state.type) {
-                url += "type=" + this.state.type;
+            // Check if query is an advanced query. We want to use the Parser only if the query is advanced
+            if (query.includes(":") || query.includes("-")) {


Peeking at https://www.npmjs.com/package/search-query-parser it says

If no keywords or ranges are specified, or if none are present in the given search query, then searchQuery.parse will return a string if tokenize is false

so you should rely on this, and avoid doing this test yourself. (E.g. what if I query for: notAValidKey:foo ?)

@djazayeri , although from what's mentioned in the docs, it seems that the parser just returns a string, it actually returns something like query: "user query" . This makes it impossible to differentiate a normal query from an advanced one. That is why I had implemented this test to check the pre-parsed query

djazayeri · 2019-07-15T05:06:43Z

src/main/ui/app/component/SearchBox.jsx

+            if (query.includes(":") || query.includes("-")) {
+                let searchQueryObj = searchQuery.parse(query, options);
+                Object.keys(searchQueryObj).forEach(function (key) {
+                    console.log(key);


need to remove this before merging the PR

djazayeri · 2019-07-15T05:45:19Z

src/main/java/org/openmrs/addonindex/rest/AddOnController.java

-	                                           @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,


In the big picture it would be nice to avoid changing method signature and adding these parameters all over the place, by just doing the query parsing entirely within the ElasticSearchIndex class.

Yes, that's true, I have currently asked around on forums. Let's hope a java based parser that's similar to the one we're currently using shows up

Conclusion: we didn't find a Java library for this, so we'll proceed doing it in the front end in JS.

djazayeri · 2019-07-15T05:48:06Z

src/main/resources/elasticsearch/addOnInfoAndVersions-settings.json

@@ -0,0 +1,11 @@
+{


Please give some more justification of why you're doing this (ideally in a code comment somewhere)

djazayeri · 2019-07-15T05:49:05Z

src/main/ui/app/component/SearchBox.jsx

-        if (this.state.type || this.state.query) {
+        if (this.state.query) {
+            let query = this.state.query.toLowerCase();
+            const options = {keywords: ['uid', 'type', 'tag', 'query', 'moduleid', 'status', 'name'], offsets: false};


pull this const outside of the function

djazayeri · 2019-07-15T05:51:55Z

src/main/ui/app/component/SearchBox.jsx

+                                url += "&exclude=" + searchQueryObj[key].text;
+                            }
+                        }
+                        else if (key === "status"){


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

djazayeri · 2019-07-15T05:54:15Z

src/main/ui/app/route/SearchPage.jsx

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


why are you constructing an url both in SearchBox and here?

djazayeri · 2019-07-15T05:56:08Z

src/main/ui/app/route/SearchPage.jsx

        if (this.state && this.state.error) {
            return <div>{this.state.error}</div>
        }
        else {
            return (
                    <div>
-                        <SearchBox initialQuery={this.props.location.query.q}/>
+                        <SearchBox initialQuery={`${query.type ? "type:" + query.type : ""} ${query.q ? "query:" + query.q : ""} ${query.tag ? "tag:" + query.tag : ""} ${query.exclude ? "-" + query.exclude : ""} ${query.name ? "name:" + query.name : ""} ${query.status ? "status:" + query.status : ""} ${query.moduleid ? "moduleId:" + query.moduleid : ""} ${query.uid ? "uid:" + query.uid : ""}`}/>


The fact that you're doing this feels like a code smell.

Maybe this would work better if SearchBox continues to just be mainly a UI element, and you move the parsing into this SearchPage class?

djazayeri · 2019-08-14T06:52:55Z

src/main/java/org/openmrs/addonindex/service/ElasticSearchIndex.java

 		BoolQueryBuilder boolQB = QueryBuilders.boolQuery();
+		if (name != null) {
+			//Exact match on name
+			boolQB.filter(QueryBuilders.matchQuery("name.raw", name));


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

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)

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?

djazayeri

Requesting change to README, and I think also a change to links to list add-ons by tag.

djazayeri · 2019-08-24T21:35:51Z

src/main/java/org/openmrs/addonindex/rest/AddOnController.java

-	                                           @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,


Conclusion: we didn't find a Java library for this, so we'll proceed doing it in the front end in JS.

djazayeri · 2019-08-24T21:36:42Z

src/main/java/org/openmrs/addonindex/service/ElasticSearchIndex.java

 		BoolQueryBuilder boolQB = QueryBuilders.boolQuery();
+		if (name != null) {
+			//Exact match on name
+			boolQB.filter(QueryBuilders.matchQuery("name.raw", name));


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?

djazayeri · 2019-08-24T21:41:08Z

src/main/ui/app/route/SearchPage.jsx

-			url += "&q=" + query;
-		}
-		if (tag) {
-			url += "&tag=" + tag;


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.

djazayeri · 2019-08-24T21:41:34Z

src/main/resources/elasticsearch/addOnInfoAndVersions-settings.json

@@ -0,0 +1,11 @@
+{


reubenvarghese1 added 11 commits June 14, 2019 03:32

Advanced Search Initial Support

33accff

Advanced Search Initial Support

a381e4a

Advanced Search Initial Support

03ead9f

Advanced Search Initial Support

6e88bf2

Advanced Search Initial Support

3102de9

Advanced Search Initial Support

e9ec7fe

Advanced Search Initial Support

0706d4c

Advanced Search Initial Support

fb9d5b6

Advanced Search added regex checks

efdcda1

Advanced Search added regex checks

613134d

Retaining Advanced Query search

c289432

reubenvarghese1 added 3 commits June 14, 2019 23:59

Advanced Query search Fixes

d226200

Advanced Query search Fixes

3259cd0

Advanced Query search Fixes

ca0ca4d

reubenvarghese1 added 2 commits July 3, 2019 04:07

Advanced Query Search Frontend Changes

00f7783

Backend for Advanced Query Search

bdfcd55

djazayeri requested changes Jul 4, 2019

View reviewed changes

reubenvarghese1 added 10 commits July 9, 2019 01:53

Changes implemented partially

f24fe19

Changes implemented partially

0a5cca6

Trial

a6904ca

Trial

0804487

Trial

f635bc5

Trial

a71f7f7

Trial

c6cc91c

Trial

80a93b4

Trial

1a30516

Trial with case sensitivity

d8a3c47

reubenvarghese1 added 7 commits July 13, 2019 14:32

Trial with case sensitivity

828ec46

Trial with case sensitivity

51755a1

Trial with case sensitivity

069ed9f

Trial with case sensitivity

62725a5

Fixed moduleid and uid error

bf095a1

Fixed moduleid and uid error

51e8adb

Fixed moduleid and uid error

d41064c

djazayeri reviewed Jul 15, 2019

View reviewed changes

djazayeri requested changes Jul 15, 2019

View reviewed changes

reubenvarghese1 added 16 commits August 8, 2019 18:54

Trial with case sensitivity

f3cc839

Trial with case sensitivity

bff4db5

Trial with case sensitivity

418f919

Trial with case sensitivity

db524ac

Trial with case sensitivity

3b2ef2a

Trial with case sensitivity

534f175

Moved Parsing to SearchPage

514b05f

Moved Parsing to SearchPage

df696ba

Moved Parsing to SearchPage

88cb630

Moved Parsing to SearchPage

0e39fa3

Moved Parsing to SearchPage

7ec8a3d

Moved Parsing to SearchPage

f5fae33

Moved Parsing to SearchPage

45506f9

Moved Parsing to SearchPage

77e0a78

Moved Parsing to SearchPage

f250080

Moved Parsing to SearchPage

05d4ab8

djazayeri reviewed Aug 14, 2019

View reviewed changes

djazayeri requested changes Aug 24, 2019

View reviewed changes

ibacher force-pushed the master branch from 8e01614 to c312126 Compare April 30, 2021 15:04

ibacher force-pushed the master branch from 36883f1 to d45eb49 Compare May 27, 2021 21:01

ibacher force-pushed the master branch from 99e953b to e61c5a8 Compare May 17, 2022 18:49

		BoolQueryBuilder boolQB = QueryBuilders.boolQuery();
		if (name != null){

AO-22 Add support for Advanced Query Syntax #70

Are you sure you want to change the base?

AO-22 Add support for Advanced Query Syntax #70

Conversation

reubenvarghese1 commented Jun 14, 2019 • edited Loading

reubenvarghese1 commented Jun 14, 2019

djazayeri commented Jun 18, 2019

djazayeri commented Jun 18, 2019

djazayeri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djazayeri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djazayeri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reubenvarghese1 commented Jun 14, 2019 •

edited

Loading