aminghadersohi commented on code in PR #35018: URL: https://github.com/apache/superset/pull/35018#discussion_r2334798195
########## tests/integration_tests/dao/base_dao_test.py: ########## @@ -0,0 +1,1397 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Integration tests for BaseDAO functionality. + +This module contains comprehensive integration tests for the BaseDAO class and its +subclasses, covering database operations, CRUD methods, flexible column support, +column operators, and error handling. + +Tests use an in-memory SQLite database for isolation and to replicate the unit test +environment behavior. User model deletions are avoided due to circular dependency +constraints with self-referential foreign keys. +""" + +import datetime +import time +import uuid + +import pytest +from flask_appbuilder.models.filters import BaseFilter +from flask_appbuilder.security.sqla.models import User +from sqlalchemy import Column, DateTime, Integer, String +from sqlalchemy.ext.declarative import declarative_base +from sqlalchemy.orm.session import Session + +from superset.daos.base import BaseDAO, ColumnOperator, ColumnOperatorEnum +from superset.daos.chart import ChartDAO +from superset.daos.dashboard import DashboardDAO +from superset.daos.database import DatabaseDAO +from superset.daos.user import UserDAO +from superset.extensions import db +from superset.models.core import Database +from superset.models.dashboard import Dashboard +from superset.models.slice import Slice + +# Create a test model for comprehensive testing +Base = declarative_base() + + +class ExampleModel(Base): # type: ignore + __tablename__ = "example_model" + id = Column(Integer, primary_key=True) + uuid = Column(String(36), unique=True, nullable=False) + slug = Column(String(100), unique=True) + name = Column(String(100)) + code = Column(String(50), unique=True) + created_on = Column(DateTime, default=datetime.datetime.utcnow) + + +class ExampleModelDAO(BaseDAO[ExampleModel]): + model_cls = ExampleModel + id_column_name = "id" + base_filter = None + + +class MockModel: + def __init__(self, id=1, name="test"): + self.id = id + self.name = name + + +class TestDAO(BaseDAO[MockModel]): + model_cls = MockModel + + +@pytest.fixture(autouse=True) +def mock_g_user(app_context): + """Mock the flask g.user for security context.""" + # Within app context, we can safely mock g + from flask import g + + mock_user = User() + mock_user.id = 1 + mock_user.username = "test_user" + + # Set g.user directly instead of patching + g.user = mock_user + yield + + # Clean up + if hasattr(g, "user"): + delattr(g, "user") + + +# ============================================================================= +# Integration Tests - These tests use the actual database +# ============================================================================= + + +def test_column_operator_enum_complete_coverage(user_with_data: Session) -> None: + """ + Test that every single ColumnOperatorEnum operator is covered by tests. + This ensures we have comprehensive test coverage for all operators. + """ + # Simply verify that we can create queries with all operators + for operator in ColumnOperatorEnum: + column_operator = ColumnOperator( + col="username", opr=operator, value="test_value" + ) + # Just check it doesn't raise an error + assert column_operator.opr == operator + + +def test_find_by_id_with_default_column(app_context: Session) -> None: + """Test find_by_id with default 'id' column.""" + # Create a user to test with + user = User( + username="test_find_by_id", + first_name="Test", + last_name="User", + email="t...@example.com", + active=True, + ) + db.session.add(user) + db.session.commit() + + # Find by numeric id + found = UserDAO.find_by_id(user.id, skip_base_filter=True) + assert found is not None + assert found.id == user.id + assert found.username == "test_find_by_id" + + # Test with non-existent id + not_found = UserDAO.find_by_id(999999, skip_base_filter=True) + assert not_found is None + + +def test_find_by_id_with_uuid_column(app_context: Session) -> None: + """Test find_by_id with custom uuid column.""" + # Create a dashboard with uuid + dashboard = Dashboard( + dashboard_title="Test UUID Dashboard", + slug="test-uuid-dashboard", + published=True, + ) + db.session.add(dashboard) + db.session.commit() + + # Find by uuid string using the uuid column + found = DashboardDAO.find_by_id( + str(dashboard.uuid), id_column="uuid", skip_base_filter=True + ) + assert found is not None + assert found.uuid == dashboard.uuid + assert found.dashboard_title == "Test UUID Dashboard" + + # Find by numeric id (should still work) + found_by_id = DashboardDAO.find_by_id(dashboard.id, skip_base_filter=True) + assert found_by_id is not None + assert found_by_id.id == dashboard.id + + # Test with non-existent uuid + not_found = DashboardDAO.find_by_id(str(uuid.uuid4()), skip_base_filter=True) + assert not_found is None + + +def test_find_by_id_with_slug_column(app_context: Session) -> None: + """Test find_by_id with slug column fallback.""" + # Create a dashboard with slug + dashboard = Dashboard( + dashboard_title="Test Slug Dashboard", + slug="test-slug-dashboard", + published=True, + ) + db.session.add(dashboard) + db.session.commit() + + # Find by slug using the slug column + found = DashboardDAO.find_by_id( + "test-slug-dashboard", id_column="slug", skip_base_filter=True + ) + assert found is not None + assert found.slug == "test-slug-dashboard" + assert found.dashboard_title == "Test Slug Dashboard" + + # Test with non-existent slug + not_found = DashboardDAO.find_by_id("non-existent-slug", skip_base_filter=True) + assert not_found is None + + +def test_find_by_id_with_invalid_column(app_context: Session) -> None: + """Test find_by_id returns None when column doesn't exist.""" + # This should return None gracefully + result = UserDAO.find_by_id("not_a_valid_id", skip_base_filter=True) + assert result is None + + +def test_find_by_id_skip_base_filter(app_context: Session) -> None: + """Test find_by_id with skip_base_filter parameter.""" + # Create users with different active states + active_user = User( + username="active_user", + first_name="Active", + last_name="User", + email="act...@example.com", + active=True, + ) + inactive_user = User( + username="inactive_user", + first_name="Inactive", + last_name="User", + email="inact...@example.com", + active=False, + ) + db.session.add_all([active_user, inactive_user]) + db.session.commit() + + # Without skipping base filter (if one exists) + found_active = UserDAO.find_by_id(active_user.id, skip_base_filter=False) + assert found_active is not None + + # With skipping base filter + found_active_skip = UserDAO.find_by_id(active_user.id, skip_base_filter=True) + assert found_active_skip is not None + + # Both should find the user since UserDAO might not have a base filter + assert found_active.id == active_user.id + assert found_active_skip.id == active_user.id + + +def test_find_by_ids_with_default_column(app_context: Session) -> None: + """Test find_by_ids with default 'id' column.""" + # Create multiple users + users = [] + for i in range(3): + user = User( + username=f"test_find_by_ids_{i}", + first_name=f"Test{i}", + last_name="User", + email=f"test{i}@example.com", + active=True, + ) + users.append(user) + db.session.add(user) + db.session.commit() + + # Find by multiple ids + ids = [user.id for user in users] + found = UserDAO.find_by_ids(ids, skip_base_filter=True) + assert len(found) == 3 + found_ids = [u.id for u in found] + assert set(found_ids) == set(ids) + + # Test with mix of existent and non-existent ids + mixed_ids = [users[0].id, 999999, users[1].id] + found_mixed = UserDAO.find_by_ids(mixed_ids, skip_base_filter=True) + assert len(found_mixed) == 2 + + # Test with empty list + found_empty = UserDAO.find_by_ids([], skip_base_filter=True) + assert found_empty == [] + + +def test_find_by_ids_with_uuid_column(app_context: Session) -> None: + """Test find_by_ids with uuid column.""" + # Create multiple dashboards + dashboards = [] + for i in range(3): + dashboard = Dashboard( + dashboard_title=f"Test UUID Dashboard {i}", + slug=f"test-uuid-dashboard-{i}", + published=True, + ) + dashboards.append(dashboard) + db.session.add(dashboard) + db.session.commit() + + # Find by multiple uuids + uuids = [str(dashboard.uuid) for dashboard in dashboards] + found = DashboardDAO.find_by_ids(uuids, id_column="uuid", skip_base_filter=True) + assert len(found) == 3 + found_uuids = [str(d.uuid) for d in found] + assert set(found_uuids) == set(uuids) + + # Test with mix of ids and uuids - search separately by column + found_by_id = DashboardDAO.find_by_ids([dashboards[0].id], skip_base_filter=True) + found_by_uuid = DashboardDAO.find_by_ids( + [str(dashboards[1].uuid)], id_column="uuid", skip_base_filter=True + ) + assert len(found_by_id) == 1 + assert len(found_by_uuid) == 1 + + +def test_find_by_ids_with_slug_column(app_context: Session) -> None: + """Test find_by_ids with slug column.""" + # Create multiple dashboards + dashboards = [] + for i in range(3): + dashboard = Dashboard( + dashboard_title=f"Test Slug Dashboard {i}", + slug=f"test-slug-dashboard-{i}", + published=True, + ) + dashboards.append(dashboard) + db.session.add(dashboard) + db.session.commit() + + # Find by multiple slugs + slugs = [dashboard.slug for dashboard in dashboards] + found = DashboardDAO.find_by_ids(slugs, id_column="slug", skip_base_filter=True) + assert len(found) == 3 + found_slugs = [d.slug for d in found] + assert set(found_slugs) == set(slugs) + + +def test_find_by_ids_with_invalid_column(app_context: Session) -> None: + """Test find_by_ids returns empty list when column doesn't exist.""" + # This should return empty list gracefully + result = UserDAO.find_by_ids(["not_a_valid_id"], skip_base_filter=True) + assert result == [] + + +def test_find_by_ids_skip_base_filter(app_context: Session) -> None: + """Test find_by_ids with skip_base_filter parameter.""" + # Create users + users = [] + for i in range(3): + user = User( + username=f"test_skip_filter_{i}", + first_name=f"Test{i}", + last_name="User", + email=f"test{i}@example.com", + active=True, + ) + users.append(user) + db.session.add(user) + db.session.commit() + + ids = [user.id for user in users] + + # Without skipping base filter + found_no_skip = UserDAO.find_by_ids(ids, skip_base_filter=False) + assert len(found_no_skip) == 3 + + # With skipping base filter + found_skip = UserDAO.find_by_ids(ids, skip_base_filter=True) + assert len(found_skip) == 3 + + +def test_base_dao_create_with_item(app_context: Session) -> None: + """Test BaseDAO.create with an item parameter.""" + # Create a user item + user = User( + username="created_with_item", + first_name="Created", + last_name="Item", + email="crea...@example.com", + active=True, + ) + + # Create using the item + created = UserDAO.create(item=user) + assert created is not None + assert created.username == "created_with_item" + assert created.first_name == "Created" + + # Verify it's in the session + assert created in db.session + + # Commit and verify it persists + db.session.commit() + + # Find it again to ensure it was saved + found = UserDAO.find_by_id(created.id, skip_base_filter=True) + assert found is not None + assert found.username == "created_with_item" + + +def test_base_dao_create_with_attributes(app_context: Session) -> None: + """Test BaseDAO.create with attributes parameter.""" + # Create using attributes dict + attributes = { + "username": "created_with_attrs", + "first_name": "Created", + "last_name": "Attrs", + "email": "at...@example.com", + "active": True, + } + + created = UserDAO.create(attributes=attributes) + assert created is not None + assert created.username == "created_with_attrs" + assert created.email == "at...@example.com" + + # Commit and verify + db.session.commit() + found = UserDAO.find_by_id(created.id, skip_base_filter=True) + assert found is not None + assert found.username == "created_with_attrs" + + +def test_base_dao_create_with_both_item_and_attributes(app_context: Session) -> None: + """Test BaseDAO.create with both item and attributes (override behavior).""" + # Create a user item + user = User( + username="item_username", + first_name="Item", + last_name="User", + email="i...@example.com", + active=False, + ) + + # Override some attributes + attributes = { + "username": "override_username", + "active": True, + } + + created = UserDAO.create(item=user, attributes=attributes) + assert created is not None + assert created.username == "override_username" # Should be overridden + assert created.active is True # Should be overridden + assert created.first_name == "Item" # Should keep original + assert created.last_name == "User" # Should keep original + + db.session.commit() + + +def test_base_dao_update_with_item(app_context: Session) -> None: + """Test BaseDAO.update with an item parameter.""" + # Create a user first + user = User( + username="update_test", + first_name="Original", + last_name="User", + email="origi...@example.com", + active=True, + ) + db.session.add(user) + db.session.commit() + + # Update the user + user.first_name = "Updated" + updated = UserDAO.update(item=user) + assert updated is not None + assert updated.first_name == "Updated" + + db.session.commit() + + # Verify the update persisted + found = UserDAO.find_by_id(user.id, skip_base_filter=True) + assert found is not None + assert found.first_name == "Updated" + + +def test_base_dao_update_with_attributes(app_context: Session) -> None: + """Test BaseDAO.update with attributes parameter.""" + # Create a user first + user = User( + username="update_attrs_test", + first_name="Original", + last_name="User", + email="origi...@example.com", + active=True, + ) + db.session.add(user) + db.session.commit() + + # Update using attributes + attributes = {"first_name": "Updated", "last_name": "Attr User"} + updated = UserDAO.update(item=user, attributes=attributes) + assert updated is not None + assert updated.first_name == "Updated" + assert updated.last_name == "Attr User" + + db.session.commit() + + +def test_base_dao_update_detached_item(app_context: Session) -> None: + """Test BaseDAO.update with a detached item.""" + # Create a user first + user = User( + username="detached_test", + first_name="Original", + last_name="User", + email="detac...@example.com", + active=True, + ) + db.session.add(user) + db.session.commit() + + user_id = user.id + + # Expunge to detach from session + db.session.expunge(user) + + # Update the detached user + user.first_name = "Updated Detached" + updated = UserDAO.update(item=user) + assert updated is not None + assert updated.first_name == "Updated Detached" + + db.session.commit() + + # Verify the update persisted + found = UserDAO.find_by_id(user_id, skip_base_filter=True) + assert found is not None + assert found.first_name == "Updated Detached" + + +def test_base_dao_delete_single_item(app_context: Session) -> None: + """Test BaseDAO.delete with a single item.""" + # Create a dashboard instead of user to avoid circular dependencies + dashboard = Dashboard( + dashboard_title="Delete Test", + slug="delete-test", + published=True, + ) + db.session.add(dashboard) + db.session.commit() + + dashboard_id = dashboard.id + + DashboardDAO.delete([dashboard]) + db.session.commit() + + # Verify it's gone + found = DashboardDAO.find_by_id(dashboard_id, skip_base_filter=True) + assert found is None + + +def test_base_dao_delete_multiple_items(app_context: Session) -> None: + """Test BaseDAO.delete with multiple items.""" + # Create multiple dashboards instead of users to avoid circular dependencies + dashboards = [] + for i in range(3): + dashboard = Dashboard( + dashboard_title=f"Delete Multi {i}", + slug=f"delete-multi-{i}", + published=True, + ) + dashboards.append(dashboard) + db.session.add(dashboard) + db.session.commit() + + dashboard_ids = [dashboard.id for dashboard in dashboards] + + DashboardDAO.delete(dashboards) + db.session.commit() + + # Verify they're all gone + for dashboard_id in dashboard_ids: + found = DashboardDAO.find_by_id(dashboard_id, skip_base_filter=True) + assert found is None Review Comment: this inside the loop. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
