betodealmeida commented on code in PR #21790:
URL: https://github.com/apache/superset/pull/21790#discussion_r1001228048
##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -130,15 +130,10 @@ def assert_log(state: str, error_message: Optional[str] =
None):
@contextmanager
def create_test_table_context(database: Database):
- database.get_sqla_engine().execute(
- "CREATE TABLE test_table AS SELECT 1 as first, 2 as second"
- )
- database.get_sqla_engine().execute(
- "INSERT INTO test_table (first, second) VALUES (1, 2)"
- )
- database.get_sqla_engine().execute(
- "INSERT INTO test_table (first, second) VALUES (3, 4)"
- )
+ with database.get_sqla_engine_with_context() as engine:
+ engine.execute("CREATE TABLE test_table AS SELECT 1 as first, 2 as
second")
+ engine.execute("INSERT INTO test_table (first, second) VALUES (1, 2)")
+ engine.execute("INSERT INTO test_table (first, second) VALUES (3, 4)")
yield db.session
database.get_sqla_engine().execute("DROP TABLE test_table")
Review Comment:
Here too?
##########
tests/integration_tests/access_tests.py:
##########
@@ -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
+ )
Review Comment:
I think this doesn't have to be inside the context manager.
##########
superset/models/core.py:
##########
@@ -362,6 +362,18 @@ def get_effective_user(self, object_url: URL) ->
Optional[str]:
else None
)
+ @contextmanager
+ def get_sqla_engine_with_context(
Review Comment:
Eventually I think we want to rename this to `get_sqla_engine`, since we
want this to be the one and only way to create an engine.
##########
tests/integration_tests/access_tests.py:
##########
@@ -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
+ )
@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
+ )
Review Comment:
Same here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
