[explore] Fix and test slice id logging issue #3339

timifasubaa · 2017-08-19T00:06:50Z

This PR fixes and tests the slice_id logging issue (#3311)
@mistercrunch @john-bodley

I was able to successfully test the /explore case but I had issues with the ajax call (or /explore_json) case. The error was CSRF token is missing. I might be missing something either in the login or in the process of making the request for JSON. The login call triggers the error. This is the same way other functions that needed to login did so.

If you want to dig in, the stack trace I get is :

Traceback (most recent call last):
  File "/Users/timi_fasubaa/Desktop/incubator-superset/tests/core_tests.py", line 739, in test_slice_id_is_always_logged_correctly_on_ajax_request
    self.login(username="admin")
  File "/Users/timi_fasubaa/Desktop/incubator-superset/tests/base_tests.py", line 131, in login
    self.assertIn('Welcome', resp)
AssertionError: u'Welcome' not found in u'<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final//EN">\n<title>400 Bad Request</title>\n<h1>Bad Request</h1>\n<p>The CSRF token is missing.</p>\n'
    """Fail immediately, with the given message."""
>>  raise self.failureException('u\'Welcome\' not found in u\'<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final//EN">\\n<title>400 Bad Request</title>\\n<h1>Bad Request</h1>\\n<p>The CSRF token is missing.</p>\\n\'')

mistercrunch · 2017-08-19T00:46:23Z

see how the other tests include a self.login call.

mistercrunch · 2017-08-19T00:52:06Z

Also looking at Travis build logs it looks like json.loads doesn't like None

timifasubaa · 2017-08-19T01:41:41Z

I took inspiration from test_user_profile and test_fetch_datasource_metadata and what I have as far as I can see is consistent with what they do.
Strangely, when I run

nosetests tests.core_tests:CoreTests.test_user_profile
nosetests tests.core_tests:CoreTests.test_fetch_datasource_metadata

They both error out with the same CSRF issue. Assuming both tests worked before, the fault may lie somewhere in the underlying authentication logic on my computer and not the tests. Can you please try on yours and see if the issue is still there?

mistercrunch · 2017-08-19T07:19:22Z

Why nosetests over ./run_specific_test.sh? It sets env vars you need.

xrmx · 2017-08-19T08:24:23Z

superset/models/core.py

            try:
+                slice_id = d.get('slice_id') or json.loads(d.get('form_data')).get('slice_id')


This will not catch when you don't have nor slice_id neither form_data:
https://travis-ci.org/apache/incubator-superset/jobs/266166713#L4907

timifasubaa · 2017-08-19T19:31:27Z

@mistercrunch I don't know why nosetests came to mind when I was on the terminal. I tried out ./run_specific_test.sh and it worked fine.

@xrmx I also addressed the possible issue with the case where neither is filled in. I don't know if making slice_id 0 after the exception is thrown is the best solution. Anything that doesn't log the right slice_id will fail the tests I wrote.

On line 73 in core_tests.py, I called explunge_all() to store things out of a session. I don't understand why things were out of the session in the first place (all other similar calls to get_resp() never had to call that function but after I did, it worked. Am I doing something wrong there?

coveralls · 2017-08-19T19:43:08Z

Coverage decreased (-0.09%) to 69.297% when pulling 2028c787040bbc1fa61c0b3ead4c647fda9548e6 on timifasubaa:fix__slice_id__logging__issue into 6fc837d on apache:master.

john-bodley · 2017-08-19T22:35:50Z

Personally I would have kept what you had but wrapped in a int(...) call per #3311 (comment).

timifasubaa · 2017-08-20T05:11:55Z

@john-bodley I was in agreement until @xrmx 's comment above. The reason I changed it is that in the event there is not form_data and json.loads takes in None, it will throw a TypeError, which won't be caught by the existing catch block. Would you prefer I changed it to also catch ValueError in addition to Exception?

Also, @mistercrunch , I'm not sure why the test coverage decreased when I wrote tests. Am I missing something?

mistercrunch · 2017-08-21T23:33:42Z

tests/core_tests.py

+        slc = db.session.query(models.Slice).filter_by(slice_name='Girls').one()
+        db.session.expunge_all()
+        self.get_resp(slc.slice_url)
+        qry = db.session.query(models.Log).filter_by(slice_id=slc.id, )


typically no trailing comma on function call

mistercrunch · 2017-08-21T23:35:46Z

superset/models/core.py

            try:
+                slice_id = d.get('slice_id') 


it's better to keep try blocks as nimble as possible, 765:766 are safe (dict.get won't throw key errors if the key doesn't exist it'll just return None) and can be executed outside the try block.

mistercrunch · 2017-08-21T23:36:19Z

tests/core_tests.py

+        # superset/explore_json case
+        self.login(username="admin")
+        slc = db.session.query(models.Slice).filter_by(slice_name='Girls').one()
+        qry = db.session.query(models.Log).filter_by(slice_id=slc.id, )


same here, no trailing coma on method calls

mistercrunch · 2017-08-21T23:39:19Z

tests/core_tests.py

+        # superset/explore_json case
+        self.login(username="admin")
+        slc = db.session.query(models.Slice).filter_by(slice_name='Girls').one()
+        qry = db.session.query(models.Log).filter_by(slice_id=slc.id, )


NIT: I'd move this to right above the self.assertEqual(1, qry.count()) line as the variable doesn't need to exist while the request takes place. Shorter lived variables are better in general. It's pretty minor but could cause confusion as to when the query is actually executed.

mistercrunch · 2017-08-21T23:42:29Z

superset/models/core.py

+                slice_id = d.get('slice_id') 
+                form_data = d.get('form_data')
+                if not slice_id and form_data:
+                    slice_id = json.loads(form_data).get('slice_id')                    
                slice_id = int(slice_id) if slice_id else 0
            except ValueError:


json.loads might raise something else than ValueError, for instance json.loads(None) raises TypeError. You might just want to be safe and catch all exceptions here as we're not trying to handle a specific type of error but just keep the program flowing regardless of what error may be raised.

mistercrunch · 2017-08-22T00:10:56Z

Also rebasing may help with coveralls checks

coveralls · 2017-08-22T19:29:08Z

Coverage decreased (-0.07%) to 69.319% when pulling 8f5d9c8 on timifasubaa:fix__slice_id__logging__issue into 1fda6f0 on apache:master.

coveralls · 2017-08-22T19:29:08Z

Coverage decreased (-0.07%) to 69.319% when pulling 8f5d9c8 on timifasubaa:fix__slice_id__logging__issue into 1fda6f0 on apache:master.

mistercrunch · 2017-08-22T19:31:31Z

LGTM

xrmx · 2017-08-23T07:48:29Z

superset/models/core.py

            try:
-                slice_id = int(slice_id) if slice_id else 0
-            except ValueError:
+                slice_id = int(d.get('slice_id') or json.loads(d.get('form_data')).get('slice_id'))


Isn't this ugly? You have slice_id and form_data already unpacked before and you are not using them here. That would make the code a lot more readable.

Also blind exceptions are an antipattern. If you want to go this route just catch TypeError and ValueError

Would yo prefer that I break it out in the if else style I had before that doesn't use exception for flow control? I am trying out the ValueError and TypeError approach now.

xrmx · 2017-08-23T21:05:31Z

superset/models/core.py

-                slice_id = int(slice_id) if slice_id else 0
-            except ValueError:
+                slice_id = int(slice_id or json.loads(form_data).get('slice_id'))
+            except Exception:


Please remove blind exceptions, You only need ValueError and TypeError

xrmx

It's not, for sure not pep8. Also you fat fingered www

timifasubaa · 2017-08-23T21:20:53Z

@xrmx You're right. I reviewed and updated it. What do you think of the current state?

coveralls · 2017-08-23T21:31:33Z

Coverage decreased (-0.07%) to 69.319% when pulling 9d254a4 on timifasubaa:fix__slice_id__logging__issue into 1fda6f0 on apache:master.

coveralls · 2017-08-23T21:35:17Z

Coverage decreased (-0.08%) to 69.314% when pulling 9d254a4 on timifasubaa:fix__slice_id__logging__issue into 1fda6f0 on apache:master.

xrmx · 2017-08-23T21:42:01Z

@timifasubaa i think that you should fix issues pointed by the reviewer instead

coveralls · 2017-08-23T21:54:02Z

Coverage decreased (-0.08%) to 69.314% when pulling 84d49bd on timifasubaa:fix__slice_id__logging__issue into 1fda6f0 on apache:master.

timifasubaa · 2017-08-23T22:01:32Z

@xrmx how about now?

coveralls · 2017-08-23T23:00:47Z

Coverage decreased (-0.08%) to 69.314% when pulling b4aa907 on timifasubaa:fix__slice_id__logging__issue into 1fda6f0 on apache:master.

coveralls · 2017-08-24T00:03:51Z

Coverage decreased (-0.08%) to 69.314% when pulling c6036ac on timifasubaa:fix__slice_id__logging__issue into 1fda6f0 on apache:master.

mistercrunch · 2017-08-24T16:11:38Z

LGTM, merging

timifasubaa added 2 commits August 10, 2017 14:42

[explore] fixed padding bug on filter section

3ae2ae5

Merge branch 'master' of https://github.com/apache/incubator-superset

ce0adb3

xrmx reviewed Aug 19, 2017

View reviewed changes

mistercrunch requested changes Aug 21, 2017

View reviewed changes

timifasubaa added 7 commits August 22, 2017 11:32

merge from upstream

d59d95b

fix slice_id logging issue

183c108

[superset-sqllab] fix slice_id population in appropriate column

78732d9

[explore-logging] test the slice_id logging fix

a638978

fix travis errors

8e80b7f

fix nits pointed out in PR comments

9a74baa

cleanup tests

8f5d9c8

timifasubaa force-pushed the fix__slice_id__logging__issue branch from 3a244cb to 8f5d9c8 Compare August 22, 2017 19:07

xrmx suggested changes Aug 23, 2017

View reviewed changes

made python more beautiful

d825933

xrmx suggested changes Aug 23, 2017

View reviewed changes

made python even more beautiful

cfe69a2

xrmx reviewed Aug 23, 2017

View reviewed changes

made python even more more beautiful

942b3bc

made python even more more more beautiful

9d254a4

fix lint error

84d49bd

make exception handling more specific

1d4b5d7

fixed silly error

b4aa907

fixed argument indentation

c6036ac

mistercrunch merged commit 8d877e8 into apache:master Aug 24, 2017

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.19.1 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[explore] Fix and test slice id logging issue #3339

[explore] Fix and test slice id logging issue #3339

timifasubaa commented Aug 19, 2017 •

edited

mistercrunch commented Aug 19, 2017

mistercrunch commented Aug 19, 2017

timifasubaa commented Aug 19, 2017

mistercrunch commented Aug 19, 2017

xrmx Aug 19, 2017

timifasubaa commented Aug 19, 2017

coveralls commented Aug 19, 2017 •

edited

john-bodley commented Aug 19, 2017

timifasubaa commented Aug 20, 2017 •

edited

mistercrunch Aug 21, 2017

mistercrunch Aug 21, 2017

mistercrunch Aug 21, 2017

mistercrunch Aug 21, 2017

mistercrunch Aug 21, 2017

mistercrunch commented Aug 22, 2017

coveralls commented Aug 22, 2017 •

edited

coveralls commented Aug 22, 2017

mistercrunch commented Aug 22, 2017

xrmx Aug 23, 2017

timifasubaa Aug 23, 2017

xrmx Aug 23, 2017

xrmx left a comment

timifasubaa commented Aug 23, 2017

coveralls commented Aug 23, 2017 •

edited

coveralls commented Aug 23, 2017 •

edited

xrmx commented Aug 23, 2017

coveralls commented Aug 23, 2017 •

edited

timifasubaa commented Aug 23, 2017

coveralls commented Aug 23, 2017 •

edited

coveralls commented Aug 24, 2017 •

edited

mistercrunch commented Aug 24, 2017

		try:
		slice_id = d.get('slice_id') or json.loads(d.get('form_data')).get('slice_id')

[explore] Fix and test slice id logging issue #3339

[explore] Fix and test slice id logging issue #3339

Conversation

timifasubaa commented Aug 19, 2017 • edited

mistercrunch commented Aug 19, 2017

mistercrunch commented Aug 19, 2017

timifasubaa commented Aug 19, 2017

mistercrunch commented Aug 19, 2017

Choose a reason for hiding this comment

timifasubaa commented Aug 19, 2017

coveralls commented Aug 19, 2017 • edited

john-bodley commented Aug 19, 2017

timifasubaa commented Aug 20, 2017 • edited

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

mistercrunch commented Aug 22, 2017

coveralls commented Aug 22, 2017 • edited

coveralls commented Aug 22, 2017

mistercrunch commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrmx left a comment

Choose a reason for hiding this comment

timifasubaa commented Aug 23, 2017

coveralls commented Aug 23, 2017 • edited

coveralls commented Aug 23, 2017 • edited

xrmx commented Aug 23, 2017

coveralls commented Aug 23, 2017 • edited

timifasubaa commented Aug 23, 2017

coveralls commented Aug 23, 2017 • edited

coveralls commented Aug 24, 2017 • edited

mistercrunch commented Aug 24, 2017

timifasubaa commented Aug 19, 2017 •

edited

coveralls commented Aug 19, 2017 •

edited

timifasubaa commented Aug 20, 2017 •

edited

coveralls commented Aug 22, 2017 •

edited

coveralls commented Aug 23, 2017 •

edited

coveralls commented Aug 23, 2017 •

edited

coveralls commented Aug 23, 2017 •

edited

coveralls commented Aug 23, 2017 •

edited

coveralls commented Aug 24, 2017 •

edited