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.

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

Search callback routing cleanup #1368

Merged
merged 5 commits into from Aug 1, 2014

Conversation

tilgovi
Copy link
Contributor

@tilgovi tilgovi commented Aug 1, 2014

I cleaned a few things up after the simple-search merge and unified the routing and location usage a little bit. I also fixed the "tag:foo" facet.

This unifies the stream and page_search query parameter usage. Both now
use the 'q' parameter. Additionally, the search string is passed to
`simpleSearch` directive and the `QueryParser` instead of an object
with a "query" or "q" key.

The clump of conditionals in the route update event handler is removed.
Instead, the viewer and page search redirect to one another and the
stream can be totally ignorant of the involvement of the location.

The views are updated to use ?q= for /u and /t shortcuts.
@tilgovi tilgovi added this to the 2014-08-08 milestone Aug 1, 2014
@gergely-ujvari
Copy link
Contributor

Currently the stream does not work, we have annotator-store error:

014-08-01 08:37:21,303 ERROR [annotator] Exception on /search_raw [POST]
Traceback (most recent call last):
  File "/home/ujvari/git5/h/local/lib/python2.7/site-packages/flask/app.py", line 1360, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/home/ujvari/git5/h/local/lib/python2.7/site-packages/flask/app.py", line 1358, in full_dispatch_request
    rv = self.dispatch_request()
  File "/home/ujvari/git5/h/local/lib/python2.7/site-packages/flask/app.py", line 1344, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/ujvari/git5/h/local/lib/python2.7/site-packages/annotator/store.py", line 257, in search_annotations_raw
    res = g.annotation_class.search_raw(request)
  File "/home/ujvari/git5/h/local/lib/python2.7/site-packages/annotator/elasticsearch.py", line 160, in search_raw
    return e.result
AttributeError: 'RequestError' object has no attribute 'result'
2014-08-01 08:37:21,304 ERROR [h.streamer] Parsing filter: {"filter":{"match_policy":"include_all","clauses":[{"field":["/quote","/tags","/text","/uri","/user"],"operator":"matches","value":"reply","case_sensitive":false,"options":{"es":{"query_type":"multi_match","match_type":"cross_fields","and_or":"and","fields":["quote","tags","text","uri","user"]}}}],"actions":{"create":true,"update":true,"delete":true},"past_data":{"load_past":"hits","hits":50}}}
Traceback (most recent call last):
  File "/home/ujvari/git5/h/h/streamer.py", line 516, in on_message
    self.send_annotations()
  File "/home/ujvari/git5/h/h/streamer.py", line 472, in send_annotations
    annotations = store.search_raw(self.query.query)
  File "/home/ujvari/git5/h/h/api.py", line 107, in search_raw
    result = self._invoke_subrequest(subreq)
  File "/home/ujvari/git5/h/h/api.py", line 122, in _invoke_subrequest
    raise exception_response(result.status_int)
HTTPServerError: The server has either erred or is incapable of performing the requested operation.

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

Also broke tests.
On Jul 31, 2014 11:39 PM, "gergely-ujvari" notifications@github.com wrote:

Currently the stream does not work, we have annotator-store error:

014-08-01 08:37:21,303 ERROR [annotator] Exception on /search_raw [POST]
Traceback (most recent call last):
File "/home/ujvari/git5/h/local/lib/python2.7/site-packages/flask/app.py", line 1360, in full_dispatch_request
rv = self.handle_user_exception(e)
File "/home/ujvari/git5/h/local/lib/python2.7/site-packages/flask/app.py", line 1358, in full_dispatch_request
rv = self.dispatch_request()
File "/home/ujvari/git5/h/local/lib/python2.7/site-packages/flask/app.py", line 1344, in dispatch_request
return self.view_functionsrule.endpoint
File "/home/ujvari/git5/h/local/lib/python2.7/site-packages/annotator/store.py", line 257, in search_annotations_raw
res = g.annotation_class.search_raw(request)
File "/home/ujvari/git5/h/local/lib/python2.7/site-packages/annotator/elasticsearch.py", line 160, in search_raw
return e.result
AttributeError: 'RequestError' object has no attribute 'result'
2014-08-01 08:37:21,304 ERROR [h.streamer] Parsing filter: {"filter":{"match_policy":"include_all","clauses":[{"field":["/quote","/tags","/text","/uri","/user"],"operator":"matches","value":"reply","case_sensitive":false,"options":{"es":{"query_type":"multi_match","match_type":"cross_fields","and_or":"and","fields":["quote","tags","text","uri","user"]}}}],"actions":{"create":true,"update":true,"delete":true},"past_data":{"load_past":"hits","hits":50}}}
Traceback (most recent call last):
File "/home/ujvari/git5/h/h/streamer.py", line 516, in on_message
self.send_annotations()
File "/home/ujvari/git5/h/h/streamer.py", line 472, in send_annotations
annotations = store.search_raw(self.query.query)
File "/home/ujvari/git5/h/h/api.py", line 107, in search_raw
result = self._invoke_subrequest(subreq)
File "/home/ujvari/git5/h/h/api.py", line 122, in _invoke_subrequest
raise exception_response(result.status_int)
HTTPServerError: The server has either erred or is incapable of performing the requested operation.


Reply to this email directly or view it on GitHub
#1368 (comment).

@gergely-ujvari
Copy link
Contributor

@tilgovi: Changing the tag object to tags broken the tag:foo search

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

@gergely-ujvari changing the tag object to 'tags' fixed the tag:foo search for me. The query for the backend is 'tags' is it not?

@tilgovi tilgovi removed the 3 - Done label Aug 1, 2014
@gergely-ujvari
Copy link
Contributor

Hmmm, but it has broken the page search. Let me check.

@gergely-ujvari
Copy link
Contributor

Oh, I see I was inconsistent. Let me fix that the tag/tags issue.

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

I'm testing here on the commits I pushed to this PR (without your most recent one). Tag search works in the stream with tag:foo and the stream works and there is no error.

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

Okay. I like "tag" as the facet. Not sure which you intended.

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

I'll fix up the tests.

@gergely-ujvari
Copy link
Contributor

Okay. The tag facet will stay. I'll fix the stream to expect that. (And after that we can squash these fix commits)

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

Okay. Tests fixed.

@gergely-ujvari
Copy link
Contributor

Ok, the tags/tag is fixed too.
One thing remains. Currently the stream any search throws that annotator-store exception

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

Hmmm. I can't reproduce it. 😕

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

Are you using the ?q= format?

@gergely-ujvari
Copy link
Contributor

Hmm.

So if you search for just "test" and you have an annotation with a body
containing the word test, it gives you back?

On 08/01/2014 08:52 AM, Randall Leeds wrote:

Hmmm. I can't reproduce it. 😕


Reply to this email directly or view it on GitHub
#1368 (comment).

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

Well, now I'm suddenly getting a coffeescript error:

FilterError: coffeescript: subprocess had error: stderr=TypeError: Cannot read property '0' of null
  at Lexer.exports.Lexer.Lexer.stringToken (/home/tilgovi/src/hypothesis/h/node_modules/coffee-script/lib/coffee-script/lexer.js:172:46)
  at Lexer.exports.Lexer.Lexer.tokenize (/home/tilgovi/src/hypothesis/h/node_modules/coffee-script/lib/coffee-script/lexer.js:31:143)
  at Object.<anonymous> (/home/tilgovi/src/hypothesis/h/node_modules/coffee-script/lib/coffee-script/coffee-script.js:49:36)
  at Object.compile (/home/tilgovi/src/hypothesis/h/node_modules/coffee-script/lib/coffee-script/coffee-script.js:34:19)
  at compileScript (/home/tilgovi/src/hypothesis/h/node_modules/coffee-script/lib/coffee-script/command.js:215:33)
  at Socket.<anonymous> (/home/tilgovi/src/hypothesis/h/node_modules/coffee-script/lib/coffee-script/command.js:254:14)
  at Socket.EventEmitter.emit (events.js:117:20)
  at _stream_readable.js:920:16
  at process._tickCallback (node.js:415:13)

@gergely-ujvari
Copy link
Contributor

Me too

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

Missing single quote:

-      and_or: 'and'
+      and_or: 'and

@gergely-ujvari
Copy link
Contributor

Yeah, noticed. Sorry for that. It was unintentional.

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

You mean it wasn't on purpose? ;)

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

It's all good.

@gergely-ujvari
Copy link
Contributor

:) Let me fix and rebase

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

I'm on it.

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

@gergely-ujvari lets see if travis is happy

@gergely-ujvari
Copy link
Contributor

@tilgovi: I can still reproduce the store issue. Which ES version are you using?

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

1.1.0

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

Also tag search doesn't really work for me. It returns everything.

@gergely-ujvari
Copy link
Contributor

Stream or page-search?

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

Stream. I'll check page.

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

Tag returns no results for me in page search, but every annotation in stream.

@gergely-ujvari
Copy link
Contributor

Strange. Yes seem to be working for me... are we looking the same branch?

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

TypeError: Cannot read property 'autofalse' of undefined
    at ViewFilter._checkMatch (http://localhost:5000/assets/scripts/hypothesis.js?8b11937d:3556:28)
    at ViewFilter.filter (http://localhost:5000/assets/scripts/hypothesis.js?8b11937d:3673:30)
    at ViewFilter.filter (http://localhost:5000/assets/scripts/hypothesis.js?8b11937d:6:61)
    at http://localhost:5000/assets/scripts/hypothesis.js?8b11937d:1259:29
    at new Search (http://localhost:5000/assets/scripts/hypothesis.js?8b11937d:1467:7)
    at invoke (http://localhost:5000/assets/scripts/vendor/angular.js:3720:17)
    at Object.instantiate (http://localhost:5000/assets/scripts/vendor/angular.js:3731:23)
    at http://localhost:5000/assets/scripts/vendor/angular.js:6848:28
    at link (http://localhost:5000/assets/scripts/vendor/angular-route.js:907:26)
    at nodeLinkFn (http://localhost:5000/assets/scripts/vendor/angular.js:6271:13) <div ng-view="" whenscrolled="loadMore(10)" class="ng-scope"> 

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

I would hope we're looking at the same branch. We both just pushed to it :-D

@gergely-ujvari
Copy link
Contributor

Let me recheck all. (Installed newer ES meanwhile)

@gergely-ujvari
Copy link
Contributor

Okay, so. Upgrading ES from 1.0.1 made the store exception go away.

@gergely-ujvari
Copy link
Contributor

Stream search. For me this is working pretty well: http://0.0.0.0:5000/stream?q=tag:foo

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

Are you sure? It returns everything for me.

@gergely-ujvari
Copy link
Contributor

@tilgovi: can you send a screenshot. It may help for me figure out this.

Page search: I see the problem, it returns every annotation with a tag in it.

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

I just fixed page search. You had missed a spot in QueryParser.

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

Both views working for me now.

@gergely-ujvari
Copy link
Contributor

Hmm. I still get the page search problem.
So if you have annotation A with tag 'foo'
and annotation B with tag 'bar'

and you search for tag:foo it returns both

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

You're right.

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

But not on stream search.

@gergely-ujvari
Copy link
Contributor

Yes, the error is in the ViewFilter. Debugging right now.

The filtered copy list was not assigned back to the variable and
accidently we were comparing the value.length instead of the filter.terms.length
to check whether all tags have matched or not.
@gergely-ujvari
Copy link
Contributor

@tilgovi : if it also works for you, I think this is good to merge.

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

@gergely-ujvari looks fine for me. Merge when the tests pass.

gergely-ujvari added a commit that referenced this pull request Aug 1, 2014
@gergely-ujvari gergely-ujvari merged commit d21a04a into master Aug 1, 2014
@gergely-ujvari gergely-ujvari deleted the search-callback-routing-cleanup branch August 1, 2014 07:46
@gergely-ujvari
Copy link
Contributor

@tilgovi: Thanks for cleaning up the routes! The code looks much nicer now! 🍺

@tilgovi
Copy link
Contributor Author

tilgovi commented Aug 1, 2014

🍺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants