Merge remote-tracking branch 'upstream/main' into upg_main

dbasunag · dbasunag · commit 664d3fa9b17f · 2025-10-21T12:55:43.000-04:00
diff --git a/tests/model_registry/model_catalog/test_filter_options_endpoint.py b/tests/model_registry/model_catalog/test_filter_options_endpoint.py
@@ -0,0 +1,121 @@
+import pytest
+from typing import Self
+from simple_logger.logger import get_logger
+
+from tests.model_registry.model_catalog.utils import validate_filter_options_structure
+from tests.model_registry.utils import get_rest_headers, execute_get_command
+from utilities.user_utils import UserTestSession
+
+LOGGER = get_logger(name=__name__)
+
+pytestmark = [
+    pytest.mark.usefixtures("updated_dsc_component_state_scope_session", "model_registry_namespace", "original_user")
+]
+
+
+@pytest.mark.parametrize(
+    "user_token_for_api_calls,",
+    [
+        pytest.param(
+            {},
+            id="test_filter_options_admin_user",
+        ),
+        pytest.param(
+            {"user_type": "test"},
+            id="test_filter_options_non_admin_user",
+        ),
+        pytest.param(
+            {"user_type": "sa_user"},
+            id="test_filter_options_service_account",
+        ),
+    ],
+    indirect=["user_token_for_api_calls"],
+)
+class TestFilterOptionsEndpoint:
+    """
+    Test class for validating the models/filter_options endpoint
+    RHOAIENG-36696
+    """
+
+    def test_filter_options_endpoint_validation(
+        self: Self,
+        model_catalog_rest_url: list[str],
+        user_token_for_api_calls: str,
+        test_idp_user: UserTestSession,
+    ):
+        """
+        Comprehensive test for filter_options endpoint.
+        Validates all acceptance criteria:
+        - A GET request returns a 200 OK response
+        - Response includes filter options for string-based properties with values array containing distinct values
+        - Response includes filter options for numeric properties with range object containing min/max values
+        - Core properties are present (license, provider, tasks, validated_on)
+        """
+        url = f"{model_catalog_rest_url[0]}models/filter_options"
+        LOGGER.info(f"Testing filter_options endpoint: {url}")
+
+        # This will raise an exception if the status code is not 200/201 (validates acceptance criteria #1)
+        response = execute_get_command(
+            url=url,
+            headers=get_rest_headers(token=user_token_for_api_calls),
+        )
+
+        assert response is not None, "Filter options response should not be None"
+        LOGGER.info("Filter options endpoint successfully returned 200 OK")
+
+        # Expected core properties based on current API response
+        expected_properties = {"license", "provider", "tasks", "validated_on"}
+
+        # Comprehensive validation using single function (validates acceptance criteria #2, #3, #4)
+        is_valid, errors = validate_filter_options_structure(response=response, expected_properties=expected_properties)
+        assert is_valid, f"Filter options validation failed: {'; '.join(errors)}"
+
+        filters = response["filters"]
+        LOGGER.info(f"Found {len(filters)} filter properties: {list(filters.keys())}")
+        LOGGER.info("All filter options validation passed successfully")
+
+    @pytest.mark.skip(reason="TODO: Implement after investigating backend DB queries")
+    def test_comprehensive_coverage_against_database(
+        self: Self,
+        model_catalog_rest_url: list[str],
+        user_token_for_api_calls: str,
+        test_idp_user: UserTestSession,
+    ):
+        """
+        STUBBED: Validate filter options are comprehensive across all sources/models in DB.
+        Acceptance Criteria: The returned options are comprehensive and not limited to a
+        subset of models or a single source.
+
+        TODO IMPLEMENTATION PLAN:
+        1. Investigate backend endpoint logic:
+           - Find the source code for /models/filter_options endpoint in kubeflow/model-registry
+           - Understand what DB tables it queries (likely model/artifact tables)
+           - Identify the exact SQL queries used to build filter values
+           - Determine database schema and column names
+
+        2. Replicate queries via pod shell:
+           - Use get_model_catalog_pod() to access catalog pod
+           - Execute psql commands via pod.execute()
+           - Query same tables/columns the endpoint uses
+           - Extract all distinct values for string properties: SELECT DISTINCT license FROM models;
+           - Extract min/max ranges for numeric properties: SELECT MIN(metric), MAX(metric) FROM models;
+
+        3. Compare results:
+           - API response filter values should match DB query results exactly
+           - Ensure no values are missing (comprehensive coverage)
+           - Validate across all sources, not just one
+
+        4. DB Access Pattern Example:
+           catalog_pod = get_model_catalog_pod(client, namespace)[0]
+           result = catalog_pod.execute(
+               command=["psql", "-U", "catalog_user", "-d", "catalog_db", "-c", "SELECT DISTINCT license FROM models;"],
+               container="catalog"
+           )
+
+        5. Implementation considerations:
+           - Handle different data types (strings vs arrays like tasks)
+           - Parse psql output correctly
+           - Handle null/empty values
+           - Ensure database connection credentials are available
+        """
+        pytest.skip("TODO: Implement comprehensive coverage validation after backend investigation")
diff --git a/tests/model_registry/model_catalog/utils.py b/tests/model_registry/model_catalog/utils.py
@@ -1,4 +1,4 @@
-from typing import Any
+from typing import Any, Tuple, List
 import yaml
 
 from kubernetes.dynamic import DynamicClient
@@ -105,6 +105,130 @@ def _extract_properties_and_required(schema: dict[Any, Any]) -> tuple[set[str],
     return all_properties - excluded_fields, required_fields - excluded_fields
 
 
+def validate_filter_options_structure(
+    response: dict[Any, Any], expected_properties: set[str] | None = None
+) -> Tuple[bool, List[str]]:
+    """
+    Comprehensive validation of filter_options response structure.
+
+    Validates:
+    - Top-level structure (filters object)
+    - All property types and their required fields
+    - Core properties presence (if specified)
+    - String properties: type, values array, distinct values
+    - Numeric properties: type, range object, min/max validity
+
+    Args:
+        response: The API response to validate
+        expected_properties: Optional set of core properties that must be present
+
+    Returns:
+        Tuple of (is_valid, list_of_errors)
+    """
+    errors = []
+
+    # Validate top-level structure
+    if not isinstance(response, dict):
+        errors.append("Response should be a dictionary")
+        return False, errors
+
+    if "filters" not in response:
+        errors.append("Response should contain 'filters' object")
+        return False, errors
+
+    filters = response["filters"]
+    if not isinstance(filters, dict):
+        errors.append("Filters should be a dictionary")
+        return False, errors
+
+    if not filters:
+        errors.append("Filters object should not be empty")
+        return False, errors
+
+    # Validate expected core properties if specified
+    if expected_properties:
+        for prop in expected_properties:
+            if prop not in filters:
+                errors.append(f"Core property '{prop}' should be present in filter options")
+
+    # Validate each property structure
+    for prop_name, prop_data in filters.items():
+        if not isinstance(prop_data, dict):
+            errors.append(f"Property '{prop_name}' should be a dictionary")
+            continue
+
+        if "type" not in prop_data:
+            errors.append(f"Property '{prop_name}' should have 'type' field")
+            continue
+
+        prop_type = prop_data["type"]
+        if not isinstance(prop_type, str) or not prop_type.strip():
+            errors.append(f"Type for '{prop_name}' should be a non-empty string")
+            continue
+
+        # Validate string properties
+        if prop_type == "string":
+            if "values" not in prop_data:
+                errors.append(f"String property '{prop_name}' should have 'values' array")
+                continue
+
+            values = prop_data["values"]
+            if not isinstance(values, list):
+                errors.append(f"Values for '{prop_name}' should be a list")
+                continue
+
+            if not values:
+                errors.append(f"Values array for '{prop_name}' should not be empty")
+                continue
+
+            # Validate individual values
+            for i, value in enumerate(values):
+                if not isinstance(value, str):
+                    errors.append(f"Value at index {i} for '{prop_name}' should be string, got: {type(value)}")
+                elif not value.strip():
+                    errors.append(f"Value at index {i} for '{prop_name}' should not be empty or whitespace")
+
+            # Check for distinct values (no duplicates)
+            try:
+                if len(values) != len(set(values)):
+                    errors.append(f"Values for '{prop_name}' should be distinct (found duplicates)")
+            except TypeError:
+                errors.append(f"Values for '{prop_name}' should be a list of strings, found unhashable type")
+
+        # Validate numeric properties - checking multiple type names since we don't know what the API will return
+        elif prop_type in ["number", "numeric", "float", "integer", "int"]:
+            if "range" not in prop_data:
+                errors.append(f"Numeric property '{prop_name}' should have 'range' object")
+                continue
+
+            range_obj = prop_data["range"]
+            if not isinstance(range_obj, dict):
+                errors.append(f"Range for '{prop_name}' should be a dictionary")
+                continue
+
+            # Check min/max presence
+            if "min" not in range_obj:
+                errors.append(f"Range for '{prop_name}' should have 'min' value")
+            if "max" not in range_obj:
+                errors.append(f"Range for '{prop_name}' should have 'max' value")
+
+            if "min" in range_obj and "max" in range_obj:
+                min_val = range_obj["min"]
+                max_val = range_obj["max"]
+
+                # Validate min/max are numeric
+                if not isinstance(min_val, (int, float)):
+                    errors.append(f"Min value for '{prop_name}' should be numeric, got: {type(min_val)}")
+                if not isinstance(max_val, (int, float)):
+                    errors.append(f"Max value for '{prop_name}' should be numeric, got: {type(max_val)}")
+
+                # Validate logical relationship (min <= max)
+                if isinstance(min_val, (int, float)) and isinstance(max_val, (int, float)) and min_val > max_val:
+                    errors.append(f"Min value ({min_val}) should be <= max value ({max_val}) for '{prop_name}'")
+
+    return len(errors) == 0, errors
+
+
 def validate_model_catalog_configmap_data(configmap: ConfigMap, num_catalogs: int) -> None:
     """
     Validate the model catalog configmap data.
diff --git a/uv.lock b/uv.lock
