-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
fix: datasource save, improve data validation #22038
fix: datasource save, improve data validation #22038
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22038 +/- ##
=======================================
Coverage 67.00% 67.01%
=======================================
Files 1815 1815
Lines 69523 69547 +24
Branches 7479 7479
=======================================
+ Hits 46585 46608 +23
- Misses 21008 21009 +1
Partials 1930 1930
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few thoughts/comments
datasource_post["default_endpoint"] = "http://www.google.com" | ||
data = dict(data=json.dumps(datasource_post)) | ||
resp = self.client.post("/datasource/save/", data=data) | ||
assert resp.status_code == 500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we really be expecting 500? This may just be me, but when I see 500, I immediately think "the system borked". Could 422 work better here?
tests/unit_tests/utils/urls_tests.py
Outdated
[ | ||
("http://localhost/", True), | ||
("http://localhost/superset/1", True), | ||
("localhost/superset/1", False), | ||
("ftp://localhost/superset/1", False), | ||
("http://external.com", False), | ||
("external.com", False), | ||
("///localhost", False), | ||
("xpto://localhost:[3/1/", False), | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also add an internal and external https://
here?
SUMMARY
Improves data validation on datasource save, sanitise default endpoint on datasets
ADDITIONAL INFORMATION