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

#5791: Address errors found by lgtm.com #7403

Open
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@sturmer
Copy link

commented Apr 29, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

The website LGTM.com has detected a few errors from static analysis. This patch contains a possible fix for the first of them ("URL redirection from remote source") that follows the website's own recommendation: Check the input against some known source before using it. I've checked that the dashboard ID provided actually exists in DB and if it doesn't I return a 404.
I plan to solve the other 4 issues labeled as "Error" but wanted to get some early feedback before moving on.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable.

TEST PLAN

I've written a unit test that can be run e.g. like:
tox -e py37 -- tests/access_tests.py:RequestAccessTests.test_dashboard_endpoint_malicious_redirect

ADDITIONAL INFORMATION

  • Has associated issue: #5791
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

Please @mistercrunch as original reporter but feel free to re-assign as you see fit.

@pull-request-size pull-request-size bot added the size/S label Apr 29, 2019

@sturmer sturmer force-pushed the sturmer:5791-improve-code-quality-based-on-lgtm_com-1 branch from 0ab7e72 to a3eada7 May 2, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 2, 2019

Codecov Report

Merging #7403 into master will increase coverage by 0.02%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7403      +/-   ##
==========================================
+ Coverage   65.78%   65.81%   +0.02%     
==========================================
  Files         459      459              
  Lines       21924    21921       -3     
  Branches     2411     2409       -2     
==========================================
+ Hits        14423    14427       +4     
+ Misses       7380     7374       -6     
+ Partials      121      120       -1
Impacted Files Coverage Δ
superset/connectors/druid/views.py 68.18% <ø> (+0.43%) ⬆️
superset/assets/src/explore/controls.jsx 42.85% <ø> (ø) ⬆️
superset/assets/src/explore/exploreUtils.js 76.47% <ø> (+0.74%) ⬆️
superset/tasks/cache.py 76.72% <ø> (ø) ⬆️
superset/data/tabbed_dashboard.py 22.22% <ø> (-12.16%) ⬇️
superset/data/energy.py 100% <ø> (ø) ⬆️
...sets/src/SqlLab/components/ScheduleQueryButton.jsx 8.77% <0%> (ø) ⬆️
...c/explore/components/controls/withVerification.jsx 96.77% <100%> (ø) ⬆️
superset/utils/core.py 88.1% <100%> (ø) ⬆️
superset/connectors/druid/models.py 82.43% <100%> (+0.04%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9abcdcc...cd59f46. Read the comment docs.

@sturmer sturmer force-pushed the sturmer:5791-improve-code-quality-based-on-lgtm_com-1 branch from 3146e1d to 7bb26da May 20, 2019

@@ -83,6 +84,8 @@ def __init__(self, name, field_names, function):
class CustomPostAggregator(Postaggregator):
"""A way to allow users to specify completely custom PostAggregators"""
def __init__(self, name, post_aggregator):
# TODO(gianluca): Is this the right way? should I use the arg post_aggregator?
Postaggregator.__init__(self, None, None, name)

This comment has been minimized.

Copy link
@john-bodley

john-bodley May 21, 2019

Contributor

Please use super instead.

@@ -82,6 +82,7 @@ def __init__(self, host='localhost', port=8125,
If statsd_client argument is given, all other arguments are ignored and the
supplied client will be used to emit metrics.
"""
BaseStatsLogger.__init__(self, prefix)

This comment has been minimized.

Copy link
@john-bodley

john-bodley May 21, 2019

Contributor

Please use super instead.

def __get__(self, obj, objtype):
if not self.is_method:
self.is_method = True
copy = self.__copy__()
return functools.partial(copy.__call__, obj)

This comment has been minimized.

Copy link
@john-bodley

john-bodley May 21, 2019

Contributor

Could you provide some more context as to what you’re trying to achieve here?

This comment has been minimized.

Copy link
@sturmer

sturmer May 22, 2019

Author

The analyzer at lgtm.com finds this assignment harmful (https://lgtm.com/rules/910074/).

The problem is (AFAIU) that we are mutating a descriptor object, and this can cause surprises to different instances of the same class. The recommendation is to create a copy that contains the desired state and return that instead.

In a patch I haven't pushed yet, I do a copy.is_method = True before returning as I do here. I wanted to copy the present instance, mutate it, and return it.

This, however, makes the unit tests test_memoized_on_methods_with_watches and test_memoized_on_methods fail.

This comment has been minimized.

Copy link
@sturmer

sturmer May 24, 2019

Author

On further thought, I think that the code uses this pattern to actually achieve the desired result. What I propose, in this case, is to either silence the alert with # lgtm[py/mutable-descriptor], or to just ignore it. Maybe a comment in the code would be enough?

This comment has been minimized.

Copy link
@sturmer

sturmer May 28, 2019

Author

@john-bodley kindly when you find a minute can you advise on this?

@@ -428,6 +428,14 @@ def test_approve(self, mock_send_mime):
session.delete(security_manager.find_role(TEST_ROLE_NAME))
session.commit()

def test_dashboard_endpoint_malicious_redirect(self):
# todo(gianluca.ciccarelli@bolt.eu): I need a dashboard ID that will

This comment has been minimized.

Copy link
@john-bodley

john-bodley May 21, 2019

Contributor

You could try using -1 (though I’m unsure if that works), otherwise maybe querying for the maximum dashboard ID and adding one, or using 9999.

@@ -71,6 +71,7 @@ def _fetch_metadata_for(datasource):

class JavascriptPostAggregator(Postaggregator):
def __init__(self, name, field_names, function):
Postaggregator.__init__(self, function, field_names, name)

This comment has been minimized.

Copy link
@mistercrunch

mistercrunch May 22, 2019

Contributor

Edits to this file seems unrelated to your PR title

This comment has been minimized.

Copy link
@sturmer

sturmer May 22, 2019

Author

I agree. The thing is that I started on the issue but gave a title related to the first I had started tackling. Do you suggest changing the title or split the PR into a few ones? (I'd favor the first.)

@sturmer sturmer force-pushed the sturmer:5791-improve-code-quality-based-on-lgtm_com-1 branch from 7bb26da to 03ff19c May 22, 2019

@sturmer sturmer changed the title [WIP] #5791: Return 404 when requesting access to unexisting dashboard ID [WIP] #5791: Address errors found by lgtm.com May 22, 2019

@sturmer sturmer force-pushed the sturmer:5791-improve-code-quality-based-on-lgtm_com-1 branch from 93cfe38 to e9c0a75 May 24, 2019

@sturmer

This comment has been minimized.

Copy link
Author

commented May 28, 2019

@mistercrunch @john-bodley please when you have time can you have a look at this? In the meanwhile, I'll start addressing the lower-priority alerts (I remember seeing many unused imports, they should be safe to remove).

@sturmer

This comment has been minimized.

Copy link
Author

commented May 28, 2019

To be clear, I think this patch could be merged as-is, but I'll add some lower-priority work to it that won't hurt. At least that's the plan 😄

@sturmer sturmer force-pushed the sturmer:5791-improve-code-quality-based-on-lgtm_com-1 branch 2 times, most recently from e71dcf4 to e96416c May 28, 2019

@sturmer sturmer changed the title [WIP] #5791: Address errors found by lgtm.com #5791: Address errors found by lgtm.com Jun 21, 2019

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jun 21, 2019

@sturmer sturmer force-pushed the sturmer:5791-improve-code-quality-based-on-lgtm_com-1 branch from d6d1e88 to 82df7c7 Jun 21, 2019

@sturmer sturmer force-pushed the sturmer:5791-improve-code-quality-based-on-lgtm_com-1 branch from 82df7c7 to 28acf04 Jun 21, 2019

@sturmer

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

I need to re-scope this one in light of the more recent alerts. I think that the important is that I manage to fix the 4 errors currently pointed out.

@sturmer

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

Now it should fix the 4 Errors and tackle a few Warnings as a bonus. It would be useful to point LGTM.com to this branch and check that that is actually the case, but I haven't been able to.

Is there still interest in going through with this? @john-bodley @mistercrunch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.