hab278
commented
6 years ago I'll push that out
On Tue, Apr 17, 2018, 3:54 AM Marco Liberati notifications@github.com wrote:
@dej611 requested changes on this pull request.
In app/controllers/searches_controller.rb https://github.com/linguisticexplorer/Linguistic-Explorer/pull/214#discussion_r180015832 :
@@ -156,13 +156,23 @@ def rescue_from_search_error(exception) end
def perform_search(offset=0)
- result_groups = {}
Would it be possibile to isolate this bit of code in a separate function? That would make the perform_search function more readable and isolate the specific bug fix.
In app/models/search_results/comparisons.rb https://github.com/linguisticexplorer/Linguistic-Explorer/pull/214#discussion_r180016037 :
@@ -5,6 +5,11 @@ module Comparisons def result_rows=(result_rows) self.result_groups = result_rows.group_by { |row| row.parent_id } self.result_groups.values.map! { |row| row.map! { |r| r.child_id }.compact! }
- self.result_groups.each do |key, value|
Can we set nil in the mapping task above?
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/linguisticexplorer/Linguistic-Explorer/pull/214#pullrequestreview-110361387, or mute the thread https://github.com/notifications/unsubscribe-auth/ABu223iZKOMwIzW70LDT0BbOMljBT7EWks5tpZ_CgaJpZM4TLygH .
…eaking comparison searches. Fixed a small grammar error.
Gory details:
You can trigger this bug by saving at least two searches, going to Search > History, and then choosing one of the actions over the two searches defined at the bottom (it doesn't matter which). The bug is that regardless of what you do, the results are just the results of the first of the two saved searches.
The issue has to do with JSON deserialization in Rails. Take the intersection search comparison for example. The comparison works by:
Running both of the searches independently and intersecting the rows of each
Rendering a new search object whose "result_groups" parameter is the rows generated by the previous step, and whose query is just the query for the first search.
This new search object gets passed to the renderer and eventually ends up in the hands of preview.js, where a JSON object is created holding both the query and the result rows and is passed in a POST back to the (non-comparative) search controller. It looks like the search controller will just use the results in result_groups if it exits, and will otherwise run the specified query on the database.
This is where things go wrong. There's a step when the search object is being built where the search rows are converted into a hash with parent IDs as the keys and child IDs as the values. If the rows have no child values, we get a hash that looks like this:
{"4681"=>[], "4683"=>[]}
The JSON that ends up being passed into the POST in preview.js looks similar. When Rails deserializes the JSON, it eliminates fields that are nil or []. I think this is related to the "deep munging" Rails does. I don't know exactly how this process works on hashes, but in practice it doesn't seem to care that we have keys. Because there are only empty values, what ends up happening is result_groups gets completely erased. This means that the search controller gets no result_groups, and just ends up running the first query (again) and returning just the results from that.
I think there are ways to mitigate this behavior in server config or controller settings, but because Terraling is running such an ancient version of Rails, none of them are available.
I pushed a fix for this that I'm not proud of, but it appears to work. It converts the empty arrays pre-serialization into empty strings, and then converts empty strings post-deserialization back into empty arrays. The searches I've run locally work as expected now. I don't really know Ruby or Rails very well, so sorry if this isn't a good solution or if I haven't coded something very elegantly.
For what it's worth, I also tried setting comparison search to return static results (I think this functionality broke whenever dynamic results were introduced), but this broke a bunch of other things.
Please take a look and let me know what you think.