fix: check type of url before performing string actions #19569

eschutho · 2022-04-06T15:42:05Z

SUMMARY

This small change ensures that a uri is a string before performing the strip method on it. If the param is a URL, like the original function, it will just return the value.

TESTING INSTRUCTIONS

automated tests to check that the make_url_safe function can take both a string or a Url

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2022-04-06T15:53:42Z

Codecov Report

Merging #19569 (2b9f90c) into master (444b4f5) will decrease coverage by 12.71%.
The diff coverage is 77.77%.

❗ Current head 2b9f90c differs from pull request most recent head 79c76c9. Consider uploading reports for the commit 79c76c9 to get more accurate results

@@             Coverage Diff             @@
##           master   #19569       +/-   ##
===========================================
- Coverage   66.65%   53.94%   -12.72%     
===========================================
  Files        1680     1680               
  Lines       64271    64274        +3     
  Branches     6564     6564               
===========================================
- Hits        42842    34673     -8169     
- Misses      19727    27899     +8172     
  Partials     1702     1702

Flag	Coverage Δ
hive	`52.71% <66.66%> (+<0.01%)`	⬆️
mysql	`?`
postgres	`?`
presto	`52.56% <66.66%> (+<0.01%)`	⬆️
python	`56.51% <77.77%> (-25.88%)`	⬇️
sqlite	`?`
unit	`47.22% <77.77%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/databases/utils.py	`28.88% <77.77%> (-52.07%)`	⬇️
superset/utils/dashboard_import_export.py	`0.00% <0.00%> (-100.00%)`	⬇️
superset/key_value/commands/upsert.py	`0.00% <0.00%> (-89.59%)`	⬇️
superset/key_value/commands/update.py	`0.00% <0.00%> (-89.37%)`	⬇️
superset/key_value/commands/delete.py	`0.00% <0.00%> (-85.30%)`	⬇️
superset/key_value/commands/delete_expired.py	`0.00% <0.00%> (-80.77%)`	⬇️
superset/dashboards/commands/importers/v0.py	`14.79% <0.00%> (-75.15%)`	⬇️
superset/datasets/commands/importers/v0.py	`24.82% <0.00%> (-68.80%)`	⬇️
superset/databases/commands/test_connection.py	`31.42% <0.00%> (-68.58%)`	⬇️
superset/datasets/commands/update.py	`25.88% <0.00%> (-68.24%)`	⬇️
... and 264 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 444b4f5...79c76c9. Read the comment docs.

etr2460 · 2022-04-06T16:42:47Z

superset/databases/utils.py

@@ -112,7 +112,9 @@ def make_url_safe(raw_url: str) -> URL:
    :param raw_url:
    :return:
    """
+
+    url = str(raw_url).strip()


if url might not be a string, can you fix the typing of the function?

Yeah, that's tricky. I found that sometimes we're passing in Url because this method has already previously been run on a uri before it was passed in here, but it would technically be better if this function were passed in a string. But since the typing is just a hint, we still need to provide our own guards against incorrect types. Do you think adding Url to the typing is still helpful or maybe a comment here?

Actually now that I look at the function parameter types for the original make_url, it can take a URL and then just returns the same URL. Good call. I think I'll do the same here.

Thanks for making the change. We definitely want the type hints to match the actual types passed in. mypy should ideally catch this when type checking too, i'm wondering why that wasn't the case

etr2460

lgtm, thanks for adding tests too!

* ensure url is a string * return url if param is a url (cherry picked from commit aa419b8)

…he#19569)" This reverts commit 6b3cd10.

* ensure url is a string * return url if param is a url (cherry picked from commit aa419b8)

sadpandajoe · 2022-04-07T16:04:10Z

🏷️ preset:2022.11

* ensure url is a string * return url if param is a url

superset-github-bot bot added the preset-io label Apr 6, 2022

pull-request-size bot added the size/M label Apr 6, 2022

superset-github-bot bot added the Superset-Community-Partners Preset community partner program participants label Apr 6, 2022

eschutho force-pushed the elizabeth/fix-url-strip branch from e85dcb6 to 117d113 Compare April 6, 2022 15:43

eschutho requested a review from craig-rueda April 6, 2022 15:43

etr2460 reviewed Apr 6, 2022

View reviewed changes

etr2460 approved these changes Apr 6, 2022

View reviewed changes

eschutho changed the title ~~fix: ensure url is a string~~ fix: check type of url before performing string actions Apr 6, 2022

eschutho removed the Superset-Community-Partners Preset community partner program participants label Apr 6, 2022

eschutho added 2 commits April 6, 2022 15:41

ensure url is a string

c9b83a4

return url if param is a url

79c76c9

eschutho force-pushed the elizabeth/fix-url-strip branch from a7ad6e2 to 79c76c9 Compare April 6, 2022 22:41

eschutho merged commit aa419b8 into apache:master Apr 7, 2022

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Apr 7, 2022

fix: check type of url before performing string actions (apache#19569)

6b3cd10

* ensure url is a string * return url if param is a url (cherry picked from commit aa419b8)

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Apr 7, 2022

fix: check type of url before performing string actions (apache#19569)

491ffd6

* ensure url is a string * return url if param is a url (cherry picked from commit aa419b8)

sadpandajoe added a commit to preset-io/superset that referenced this pull request Apr 7, 2022

Revert "fix: check type of url before performing string actions (apac…

f1b1dd3

…he#19569)" This reverts commit 6b3cd10.

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Apr 7, 2022

fix: check type of url before performing string actions (apache#19569)

e067c3f

* ensure url is a string * return url if param is a url (cherry picked from commit aa419b8)

superset-github-bot bot added the preset:2022.11 label Apr 7, 2022

philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022

fix: check type of url before performing string actions (apache#19569)

2d767c9

* ensure url is a string * return url if param is a url

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: check type of url before performing string actions #19569

fix: check type of url before performing string actions #19569

eschutho commented Apr 6, 2022 •

edited

Loading

codecov bot commented Apr 6, 2022 •

edited

Loading

etr2460 Apr 6, 2022

eschutho Apr 6, 2022

eschutho Apr 6, 2022 •

edited

Loading

etr2460 Apr 6, 2022

etr2460 left a comment

sadpandajoe commented Apr 7, 2022

fix: check type of url before performing string actions #19569

fix: check type of url before performing string actions #19569

Conversation

eschutho commented Apr 6, 2022 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Apr 6, 2022 • edited Loading

Codecov Report

etr2460 Apr 6, 2022

Choose a reason for hiding this comment

eschutho Apr 6, 2022

Choose a reason for hiding this comment

eschutho Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

etr2460 Apr 6, 2022

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

sadpandajoe commented Apr 7, 2022

eschutho commented Apr 6, 2022 •

edited

Loading

codecov bot commented Apr 6, 2022 •

edited

Loading

eschutho Apr 6, 2022 •

edited

Loading