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

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

Merged
merged 17 commits into from
Aug 24, 2017

Conversation

timifasubaa
Copy link
Contributor

@timifasubaa timifasubaa commented Aug 19, 2017

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
Copy link
Member

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

@mistercrunch
Copy link
Member

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

@timifasubaa
Copy link
Contributor Author

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
Copy link
Member

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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
Copy link

coveralls commented Aug 19, 2017

Coverage Status

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

@john-bodley
Copy link
Member

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

@timifasubaa
Copy link
Contributor Author

timifasubaa commented Aug 20, 2017

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

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, )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typically no trailing comma on function call

try:
slice_id = d.get('slice_id')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# 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, )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, no trailing coma on method calls

# 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, )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Also rebasing may help with coveralls checks

@coveralls
Copy link

coveralls commented Aug 22, 2017

Coverage Status

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

1 similar comment
@coveralls
Copy link

Coverage Status

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

@mistercrunch
Copy link
Member

LGTM

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'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove blind exceptions, You only need ValueError and TypeError

Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@timifasubaa
Copy link
Contributor Author

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

@coveralls
Copy link

coveralls commented Aug 23, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Aug 23, 2017

Coverage Status

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

@xrmx
Copy link
Contributor

xrmx commented Aug 23, 2017

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

@coveralls
Copy link

coveralls commented Aug 23, 2017

Coverage Status

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

@timifasubaa
Copy link
Contributor Author

@xrmx how about now?

@coveralls
Copy link

coveralls commented Aug 23, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Aug 24, 2017

Coverage Status

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

@mistercrunch
Copy link
Member

LGTM, merging

@mistercrunch mistercrunch merged commit 8d877e8 into apache:master Aug 24, 2017
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.19.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants