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

Conversation

@sturmer
Copy link
Contributor

@sturmer sturmer 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.

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

@codecov-io codecov-io 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.71%   65.74%   +0.02%     
==========================================
  Files         459      459              
  Lines       21983    21980       -3     
  Branches     2415     2413       -2     
==========================================
+ Hits        14446    14450       +4     
+ Misses       7416     7410       -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 ebb7fbc...2b0d317. Read the comment docs.

@sturmer sturmer force-pushed the 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)
Copy link
Contributor

@john-bodley john-bodley May 21, 2019

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

@john-bodley john-bodley May 21, 2019

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

@john-bodley john-bodley May 21, 2019

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

Copy link
Contributor Author

@sturmer sturmer May 22, 2019

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.

Copy link
Contributor Author

@sturmer sturmer May 24, 2019

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?

Copy link
Contributor Author

@sturmer sturmer May 28, 2019

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

@john-bodley john-bodley May 21, 2019

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

@mistercrunch mistercrunch May 22, 2019

Edits to this file seems unrelated to your PR title

Copy link
Contributor Author

@sturmer sturmer May 22, 2019

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 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 5791-improve-code-quality-based-on-lgtm_com-1 branch from 93cfe38 to e9c0a75 May 24, 2019
@sturmer
Copy link
Contributor Author

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

@sturmer sturmer 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 5791-improve-code-quality-based-on-lgtm_com-1 branch 2 times, most recently from e71dcf4 to e96416c May 29, 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 5791-improve-code-quality-based-on-lgtm_com-1 branch 2 times, most recently from 82df7c7 to 28acf04 Jun 21, 2019
@sturmer
Copy link
Contributor Author

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

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

@sturmer sturmer force-pushed the 5791-improve-code-quality-based-on-lgtm_com-1 branch from cd59f46 to 2b0d317 Jun 26, 2019
@stale
Copy link

@stale stale bot commented Aug 25, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive label Aug 25, 2019
@stale stale bot closed this Sep 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants