-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
feat: create function for get_sqla_engine with context #21790
Changes from 10 commits
f6490fa
fc0b0d5
897c921
b824ab3
2497b72
296d8f2
83130ca
4b4613f
ca24e25
c006017
cbb400c
63d4a9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,7 @@ | |
import logging | ||
import textwrap | ||
from ast import literal_eval | ||
from contextlib import closing | ||
from contextlib import closing, contextmanager | ||
from copy import deepcopy | ||
from datetime import datetime | ||
from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Type | ||
|
@@ -362,6 +362,18 @@ def get_effective_user(self, object_url: URL) -> Optional[str]: | |
else None | ||
) | ||
|
||
@contextmanager | ||
def get_sqla_engine_with_context( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eventually I think we want to rename this to |
||
self, | ||
schema: Optional[str] = None, | ||
nullpool: bool = True, | ||
source: Optional[utils.QuerySource] = None, | ||
) -> Engine: | ||
try: | ||
yield self.get_sqla_engine(schema=schema, nullpool=nullpool, source=source) | ||
except Exception as ex: | ||
raise self.db_engine_spec.get_dbapi_mapped_exception(ex) | ||
|
||
def get_sqla_engine( | ||
self, | ||
schema: Optional[str] = None, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -158,64 +158,64 @@ def test_override_role_permissions_is_admin_only(self): | |
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") | ||
def test_override_role_permissions_1_table(self): | ||
database = get_example_database() | ||
engine = database.get_sqla_engine() | ||
schema = inspect(engine).default_schema_name | ||
with database.get_sqla_engine_with_context() as engine: | ||
schema = inspect(engine).default_schema_name | ||
|
||
perm_data = ROLE_TABLES_PERM_DATA.copy() | ||
perm_data["database"][0]["schema"][0]["name"] = schema | ||
perm_data = ROLE_TABLES_PERM_DATA.copy() | ||
perm_data["database"][0]["schema"][0]["name"] = schema | ||
|
||
response = self.client.post( | ||
"/superset/override_role_permissions/", | ||
data=json.dumps(perm_data), | ||
content_type="application/json", | ||
) | ||
self.assertEqual(201, response.status_code) | ||
response = self.client.post( | ||
"/superset/override_role_permissions/", | ||
data=json.dumps(perm_data), | ||
content_type="application/json", | ||
) | ||
self.assertEqual(201, response.status_code) | ||
|
||
updated_override_me = security_manager.find_role("override_me") | ||
self.assertEqual(1, len(updated_override_me.permissions)) | ||
birth_names = self.get_table(name="birth_names") | ||
self.assertEqual( | ||
birth_names.perm, updated_override_me.permissions[0].view_menu.name | ||
) | ||
self.assertEqual( | ||
"datasource_access", updated_override_me.permissions[0].permission.name | ||
) | ||
updated_override_me = security_manager.find_role("override_me") | ||
self.assertEqual(1, len(updated_override_me.permissions)) | ||
birth_names = self.get_table(name="birth_names") | ||
self.assertEqual( | ||
birth_names.perm, updated_override_me.permissions[0].view_menu.name | ||
) | ||
self.assertEqual( | ||
"datasource_access", updated_override_me.permissions[0].permission.name | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this doesn't have to be inside the context manager. |
||
|
||
@pytest.mark.usefixtures( | ||
"load_energy_table_with_slice", "load_birth_names_dashboard_with_slices" | ||
) | ||
def test_override_role_permissions_drops_absent_perms(self): | ||
database = get_example_database() | ||
engine = database.get_sqla_engine() | ||
schema = inspect(engine).default_schema_name | ||
with database.get_sqla_engine_with_context() as engine: | ||
schema = inspect(engine).default_schema_name | ||
|
||
override_me = security_manager.find_role("override_me") | ||
override_me.permissions.append( | ||
security_manager.find_permission_view_menu( | ||
view_menu_name=self.get_table(name="energy_usage").perm, | ||
permission_name="datasource_access", | ||
override_me = security_manager.find_role("override_me") | ||
override_me.permissions.append( | ||
security_manager.find_permission_view_menu( | ||
view_menu_name=self.get_table(name="energy_usage").perm, | ||
permission_name="datasource_access", | ||
) | ||
) | ||
) | ||
db.session.flush() | ||
db.session.flush() | ||
|
||
perm_data = ROLE_TABLES_PERM_DATA.copy() | ||
perm_data["database"][0]["schema"][0]["name"] = schema | ||
perm_data = ROLE_TABLES_PERM_DATA.copy() | ||
perm_data["database"][0]["schema"][0]["name"] = schema | ||
|
||
response = self.client.post( | ||
"/superset/override_role_permissions/", | ||
data=json.dumps(perm_data), | ||
content_type="application/json", | ||
) | ||
self.assertEqual(201, response.status_code) | ||
updated_override_me = security_manager.find_role("override_me") | ||
self.assertEqual(1, len(updated_override_me.permissions)) | ||
birth_names = self.get_table(name="birth_names") | ||
self.assertEqual( | ||
birth_names.perm, updated_override_me.permissions[0].view_menu.name | ||
) | ||
self.assertEqual( | ||
"datasource_access", updated_override_me.permissions[0].permission.name | ||
) | ||
response = self.client.post( | ||
"/superset/override_role_permissions/", | ||
data=json.dumps(perm_data), | ||
content_type="application/json", | ||
) | ||
self.assertEqual(201, response.status_code) | ||
updated_override_me = security_manager.find_role("override_me") | ||
self.assertEqual(1, len(updated_override_me.permissions)) | ||
birth_names = self.get_table(name="birth_names") | ||
self.assertEqual( | ||
birth_names.perm, updated_override_me.permissions[0].view_menu.name | ||
) | ||
self.assertEqual( | ||
"datasource_access", updated_override_me.permissions[0].permission.name | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||
|
||
def test_clean_requests_after_role_extend(self): | ||
session = db.session | ||
|
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.
What is the difference between having this new
get_sqla_engine_with_context
with the decorator VS using it in our existingget_sqla_engine
? I Mean, couldn't we just use the existing one and make use or not of the new functionality the decorator brings when needed? or would that mean changing a ton of places where theget_sqla_engine
is being used right now?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.
For now there's no difference, but Hugh is planning to add support for SSH tunneling, which would require a setup phase before the engine is created, and a teardown after. In order for the SSH tunnel to work everywhere we will need to replace all existing calls with the new context manager.
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.
Oh ok ok, makes sense then 😎 thanks!