Prevent app from crashing #498

rupav · 2018-02-06T14:23:50Z

Fixes #494

Checklist

I have read the Contribution & Best practices Guide and my PR follows them.
My branch is up-to-date with the Upstream master branch.
I have added necessary documentation (if appropriate)

Changes proposed in this pull request:

Prevents app from crashing for those engines whose qtype like news not implemented, yet is parsed, and creates a 400/404 error internally.
parsijoo, although with 200 status_code, returns empty urls, and hence cases like this is also handled.
https://aqueous-refuge-13068.herokuapp.com/

Few queries to look for:
search chelsea, friend, local or any other query on google+news and parsijoo+news in both original http://query-server.herokuapp.com/ and https://aqueous-refuge-13068.herokuapp.com/ .

rupav · 2018-02-06T14:27:35Z

app/scrapers/parsijoo.py

-        print('Parsijoo parsed: ' + str(urls))
-
+        try:
+            print('Parsijoo parsed: ' + str(urls))


If I don't use try except block, I am getting error :
'charmap' codec can't encode characters in position 29-31: character maps to <undefined>.
This error comes up when urls are empty.
It seems to be a console error rather than python as per statckoverflow . So handled this way.

looks like encoding problem

rupav · 2018-02-06T14:32:32Z

@raju249 @realslimshanky let me know if app is now not crashing anyhow.
I know for a given status code I haven't put a correct corresponding message to show.
But for that, in this PR only, I will create an external file, ContentManagement, where I will map, all error codes like,
400, 419 etc., to their respective message to show on browser. I will fetch this in python script and then I will show message as per error code.
Can I have the liberty to create another file ContentManagement?

vaibhavsingh97

Good Job 👏

vaibhavsingh97 · 2018-02-06T14:41:54Z

app/scrapers/generalized.py

@@ -42,8 +42,12 @@ def get_page(self, query, startIndex=0, qtype=''):
        if self.name == 'mojeek' and qtype == 'news':
            payload['fmt'] = 'news'
        response = requests.get(url, headers=self.headers, params=payload)
+        status_code = response.status_code
+        print(status_code)


Remove this statement. IMO, we can use logging to give detail logs on console. Let me know your opinion

@vaibhavsingh97 I don't know about this logging.(I thought print is only used to debug) Can you please point me to a link to read on this.

You can look here and here

vaibhavsingh97 · 2018-02-06T14:42:20Z

app/scrapers/generalized.py

+        status_code = response.status_code
+        print(status_code)
+        if(status_code == 400 or status_code == 404):
+            return (None, status_code)
        print(response.url)


vaibhavsingh97 · 2018-02-06T14:42:47Z

app/scrapers/generalized.py

+                if(len(urls) == 0):
+                    return (None, status_code)
+                else:
+                    print("Couldn't fetch more results.")


This is the case where, urls fetched are not equal to expected number of urls. Should not be removed from logs then in my opinion. As we have not implemented no. of urls fetched yet.

I am not asking to remove this, I am asking to replace with logging statements

vaibhavsingh97 · 2018-02-06T14:44:11Z

app/scrapers/parsijoo.py

-        print('Parsijoo parsed: ' + str(urls))
-
+        try:
+            print('Parsijoo parsed: ' + str(urls))


looks like encoding problem

vaibhavsingh97 · 2018-02-06T14:44:37Z

app/server.py

@@ -36,11 +36,24 @@ def store(url, links):
 def index():
    return render_template('index.html', engines_list=scrapers.keys())

+'''
+def bad_request(error):


this function doesn't handle the case of csv

@vaibhavsingh97 can this be created as a separate issue for beginners. No case handling for csv too was present earlier. Should I open an issue for the same for beginners?

@rupav Yes, That would be great and help beginner to solve the issue as well 👍

vaibhavsingh97 · 2018-02-06T14:44:54Z

app/server.py

-    return make_response(response, error[0])
+    print(error[2])
+    if error[2] == 'xml':
+        print("Its XML!!!!!")


vaibhavsingh97 · 2018-02-06T14:47:47Z

? But for that, in this PR only, I will create an external file, ContentManagement, where I will map, all error codes like,
400, 419 etc., to their respective message to show on browser. I will fetch this in python script and then I will show message as per error code.
Can I have the liberty to create another file ContentManagement?

Will this will not create problem for other file types like csv and xml. Can you please give more detail on this.

rupav · 2018-02-06T14:52:40Z

@vaibhavsingh97 .No, regarding your query on adding a message corresponding to a given error,
it will not create error for xml. And for csv, its message will be shown in default json object. It will work just like it now...

vaibhavsingh97 · 2018-02-06T14:54:21Z

@rupav I am getting [object Object] in CSV

rupav · 2018-02-06T14:55:22Z

Ok, @vaibhavsingh97 looking into it. 👍

realslimshanky · 2018-02-06T15:06:03Z

Prevents app from crashing for those engines whose qtype like news not implemented, yet is parsed, and creates a 400/404 error internally.

IMHO, a user-friendly message should be included on the client side like News hasn't been implemented for this search engine yet.

vaibhavsingh97 · 2018-02-06T15:21:22Z

@realslimshanky This we can add as an enhancement and let beginner would solve that issue so from this atleast some new comer will join us and can join our awesome team 😄

rupav · 2018-02-06T15:57:42Z

@vaibhavsingh97 done (PS: https://travis-ci.org/fossasia/query-server/jobs/338069203 tests are failing because I introduced one more value to return 😅 i.e. status_code) These can be changed after this PR?
@realslimshanky please review and let me know if I should make ContentManagement file too.

vaibhavsingh97 · 2018-02-06T16:17:59Z

@rupav It will better if you fix travis as well

rupav · 2018-02-06T16:20:47Z

Ok @vaibhavsingh97 , so I have to change tests then. But one question. If I change tests in this PR, how can travis will be validating with new tests since new tests has not been merged 🤔?

vaibhavsingh97 · 2018-02-07T05:47:21Z

@rupav So what travis does is pull your commit and run tests present in your commit so if you change/add tests than travis will test those tests

realslimshanky

Please check the travis report for your PR as many tests are failing.

rupav · 2018-02-07T17:11:21Z

yes @realslimshanky , since I introduces one more parameter, tests are failing. Looking into them 👍

rupav · 2018-02-08T15:48:18Z

@vaibhavsingh97 I solved almost all conflicts. had to comment out a test too (as it will always fail),
there is one I couldnt solve. Please see python 3.6 travis logs, which states
AssertionError: assert [{'desc': 'mo...'mock_title'}] == {'error': 'Cou...Server Error'} .
This means its not fetching results. But I tried running locally with given query, qformat, and engine, giving correct results.
ping @realslimshanky

realslimshanky · 2018-02-08T21:17:33Z

Firstly it's not recommended to skip tests, especially when we are modifying the API.
I tried to look into your changes and break apart the test alongside your modifications. The error Travis is throwing is the one when server.py breaks while retrieving and sending result from the scrapper. Maybe I'm not getting to the point where the problem lies, even the environment is almost the same for local dev as well. We should hold this implementation until there is a solution.

rupav · 2018-02-09T04:23:39Z

test/test_server.py

@@ -76,6 +77,7 @@ def test_api_search_for_no_response(mock_bad_request):
            mock_bad_request.assert_called_with([404, 'No response',
                                                 'google:fossasia'])
            assert resp == "Mock Response"
+'''


@realslimshanky can you please see this test I skipped(commented out)...
test_api_searh_for_no_response, why line 77 will ever be called? My point is why bad_request function will be called when '/api/v1/search/google?query=fossasia' fetches correct results, so it will not lead to calling up bad request. That's why this test will fail every time 😫

Actually, it is not testing for response from the server, it's testing the response already stored in catch. And since the request is made only once it's not present in cache.

So if not present in the cache, which code ideally should be executed, bad_request should have been called ?
Which statements are we talking about here ... Please help. @realslimshanky

I have not worked with pytest/mock yet and would take some time to fully understand in order to assist you better. I would like to request contributors who have worked on this module to please help out.
cc: @vaibhavsingh97 @bhaveshAn

ping @cclauss @SaptakS

@cclauss @bhaveshAn @vaibhavsingh97 please suggest.

umangahuja1 · 2018-02-10T06:37:26Z

Bing news is also crashing (Try : cricket)

vaibhavsingh97 · 2018-04-13T15:10:51Z

@rupav Status?

rupav · 2018-05-17T12:09:42Z

@vaibhavsingh97 sorry for such a long delay, will get back to this, once I am through the documentation of pytest. I will update this as soon as I understand the problem ( which I raised earlier ).

mariobehling · 2018-09-03T05:41:17Z

Closing this due to inactivity. Please reopen new PR if you want to resolve this.

rupav commented Feb 6, 2018

View reviewed changes

rupav force-pushed the mock-494-issue branch from 381644b to b840bee Compare February 6, 2018 14:38

vaibhavsingh97 reviewed Feb 6, 2018

View reviewed changes

vaibhavsingh97 suggested changes Feb 6, 2018

View reviewed changes

vaibhavsingh97 assigned rupav Feb 6, 2018

vaibhavsingh97 added DO NOT MERGE Work In Progress labels Feb 6, 2018

rupav force-pushed the mock-494-issue branch from b840bee to fc4632c Compare February 6, 2018 15:41

Prevent app from crashing

cbf3941

rupav force-pushed the mock-494-issue branch from fc4632c to cbf3941 Compare February 6, 2018 15:50

realslimshanky suggested changes Feb 7, 2018

View reviewed changes

rupav force-pushed the mock-494-issue branch 7 times, most recently from 60fec61 to b469b4d Compare February 8, 2018 04:32

rupav force-pushed the mock-494-issue branch 7 times, most recently from 33fda7b to feeec13 Compare February 8, 2018 15:33

Add modified tests

2ad4f21

rupav force-pushed the mock-494-issue branch from feeec13 to 2ad4f21 Compare February 8, 2018 15:38

rupav commented Feb 9, 2018

View reviewed changes

rupav mentioned this pull request Feb 10, 2018

block qtype for a given engine if not implemented #500

Merged

3 tasks

Merge branch 'master' into mock-494-issue

1f0fec6

mariobehling closed this Sep 3, 2018

Prevent app from crashing #498

Prevent app from crashing #498

Conversation

rupav commented Feb 6, 2018

Checklist

Changes proposed in this pull request:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rupav commented Feb 6, 2018

vaibhavsingh97 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

vaibhavsingh97 commented Feb 6, 2018

rupav commented Feb 6, 2018

vaibhavsingh97 commented Feb 6, 2018

rupav commented Feb 6, 2018

realslimshanky commented Feb 6, 2018

vaibhavsingh97 commented Feb 6, 2018

rupav commented Feb 6, 2018

vaibhavsingh97 commented Feb 6, 2018

rupav commented Feb 6, 2018

vaibhavsingh97 commented Feb 7, 2018

realslimshanky left a comment

Choose a reason for hiding this comment

rupav commented Feb 7, 2018

rupav commented Feb 8, 2018

realslimshanky commented Feb 8, 2018

rupav Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rupav Feb 9, 2018 • edited Loading

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

umangahuja1 commented Feb 10, 2018

vaibhavsingh97 commented Apr 13, 2018

rupav commented May 17, 2018

mariobehling commented Sep 3, 2018

rupav Feb 9, 2018 •

edited

Loading

rupav Feb 9, 2018 •

edited

Loading