Skip to content
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

[BUGFIX] Ensure that Checkpoint deserializes proper action subclass #9701

Merged
merged 4 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 1 addition & 11 deletions great_expectations/checkpoint/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

from __future__ import annotations

import importlib
import json
import logging
from typing import (
Expand Down Expand Up @@ -46,7 +45,7 @@
ValidationResultIdentifier,
)
from great_expectations.data_context.util import instantiate_class_from_config
from great_expectations.exceptions import ClassInstantiationError, DataContextError
from great_expectations.exceptions import ClassInstantiationError
from great_expectations.render.renderer import (
EmailRenderer,
MicrosoftTeamsRenderer,
Expand Down Expand Up @@ -401,15 +400,6 @@ class PagerdutyAlertAction(ValidationAction):
notify_on: Literal["all", "failure", "success"] = "failure"
severity: Literal["critical", "error", "warning", "info"] = "critical"

@root_validator
def _root_validate_deps(cls, values: dict) -> dict:
if not importlib.util.find_spec("pypd"):
raise DataContextError( # noqa: TRY003
'ModuleNotFoundError: No module named "pypd". "pypd" is required for PageDuty notification actions.' # noqa: E501
)

return values

@override
def _run( # type: ignore[override] # signature does not match parent # noqa: PLR0913
self,
Expand Down
26 changes: 24 additions & 2 deletions great_expectations/checkpoint/v1_checkpoint.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,16 @@

import great_expectations.exceptions as gx_exceptions
from great_expectations._docs_decorators import public_api
from great_expectations.checkpoint.actions import ValidationAction # noqa: TCH001
from great_expectations.checkpoint.actions import (
EmailAction,
MicrosoftTeamsNotificationAction,
OpsgenieAlertAction,
PagerdutyAlertAction,
SlackNotificationAction,
SNSNotificationAction,
StoreValidationResultAction,
UpdateDataDocsAction,
)
from great_expectations.compatibility.pydantic import BaseModel, root_validator, validator
from great_expectations.core.expectation_validation_result import (
ExpectationSuiteValidationResult, # noqa: TCH001
Expand All @@ -22,10 +31,23 @@
from great_expectations.render.renderer.renderer import Renderer

if TYPE_CHECKING:
from typing_extensions import TypeAlias

Check warning on line 34 in great_expectations/checkpoint/v1_checkpoint.py

View check run for this annotation

Codecov / codecov/patch

great_expectations/checkpoint/v1_checkpoint.py#L34

Added line #L34 was not covered by tests

from great_expectations.data_context.store.validation_definition_store import (
ValidationDefinitionStore,
)

CheckpointAction: TypeAlias = Union[
EmailAction,
MicrosoftTeamsNotificationAction,
OpsgenieAlertAction,
PagerdutyAlertAction,
SlackNotificationAction,
SNSNotificationAction,
StoreValidationResultAction,
UpdateDataDocsAction,
]


class Checkpoint(BaseModel):
"""
Expand All @@ -45,7 +67,7 @@

name: str
validation_definitions: List[ValidationDefinition]
actions: List[ValidationAction]
actions: List[CheckpointAction]
result_format: ResultFormat = ResultFormat.SUMMARY
id: Union[str, None] = None

Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,7 @@ extend-exclude = [
runtime-evaluated-base-classes = [
# NOTE: ruff is unable to detect that these are subclasses of pydantic.BaseModel
"pydantic.BaseModel",
"great_expectations.checkpoint.v1_checkpoint.Checkpoint",
"great_expectations.datasource.fluent.fluent_base_model.FluentBaseModel",
"great_expectations.datasource.fluent.interfaces.Datasource",
"great_expectations.datasource.fluent.sql_datasource.SQLDatasource",
Expand Down
51 changes: 2 additions & 49 deletions tests/actions/test_core_actions.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import json
import logging
from typing import Type, Union
from typing import Type
from unittest import mock

import pytest
Expand All @@ -23,7 +23,7 @@
)
from great_expectations.checkpoint.util import smtplib
from great_expectations.checkpoint.v1_checkpoint import Checkpoint, CheckpointResult
from great_expectations.compatibility.pydantic import BaseModel, Field, ValidationError
from great_expectations.compatibility.pydantic import ValidationError
from great_expectations.core.expectation_validation_result import (
ExpectationSuiteValidationResult,
)
Expand All @@ -37,7 +37,6 @@
ExpectationSuiteIdentifier,
ValidationResultIdentifier,
)
from great_expectations.render.renderer.renderer import Renderer
from great_expectations.util import is_library_loadable

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -764,52 +763,6 @@ def test_action_deserialization(
actual = action_class.parse_obj(serialized_action)
assert isinstance(actual, action_class)

@pytest.mark.parametrize(
"action_class, init_params",
[(k, v) for k, v in ACTION_INIT_PARAMS.items()],
ids=[k.__name__ for k in ACTION_INIT_PARAMS],
)
@pytest.mark.unit
def test_action_deserialization_within_parent_class(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we can test this with Checkpoint, I'd like to remove this test

self, action_class: ValidationAction, init_params: dict
):
"""
The matter of deserializing an Action into the relevant subclass is the responsibility of
the Checkpoint in production code.

In order to test Actions alone, we utilize a dummy class with a single field to ensure
properly implementation of Pydantic discriminated unions.

Ref: https://docs.pydantic.dev/1.10/usage/types/#discriminated-unions-aka-tagged-unions
"""

# This somewhat feels like we're testing Pydantic code but it's the only way to ensure that
# we've properly implemented the Pydantic discriminated union feature.
class DummyClassWithActionChild(BaseModel):
class Config:
# Due to limitations of Pydantic V1, we need to specify the json_encoders at every level of the hierarchy # noqa: E501
json_encoders = {Renderer: lambda r: r.serialize()}

action: Union[
APINotificationAction,
EmailAction,
MicrosoftTeamsNotificationAction,
OpsgenieAlertAction,
PagerdutyAlertAction,
SlackNotificationAction,
SNSNotificationAction,
StoreValidationResultAction,
UpdateDataDocsAction,
] = Field(..., discriminator="type")

action = action_class(**init_params)
instance = DummyClassWithActionChild(action=action)

json_dict = instance.json()
parsed_action = DummyClassWithActionChild.parse_raw(json_dict)

assert isinstance(parsed_action.action, action_class)


class TestV1ActionRun:
@pytest.fixture
Expand Down
46 changes: 43 additions & 3 deletions tests/checkpoint/test_v1_checkpoint.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,14 @@
from great_expectations.checkpoint.actions import (
MicrosoftTeamsNotificationAction,
SlackNotificationAction,
UpdateDataDocsAction,
ValidationAction,
)
from great_expectations.checkpoint.v1_checkpoint import Checkpoint, CheckpointResult
from great_expectations.checkpoint.v1_checkpoint import (
Checkpoint,
CheckpointAction,
CheckpointResult,
)
from great_expectations.compatibility.pydantic import ValidationError
from great_expectations.core.batch_definition import BatchDefinition
from great_expectations.core.expectation_suite import ExpectationSuite
Expand Down Expand Up @@ -209,7 +214,9 @@ def test_checkpoint_serialization(

@pytest.mark.filesystem
def test_checkpoint_filesystem_round_trip_adds_ids(
self, tmp_path: pathlib.Path, actions: list[ValidationAction]
self,
tmp_path: pathlib.Path,
actions: list[CheckpointAction],
):
with working_directory(tmp_path):
context = gx.get_context(mode="file")
Expand Down Expand Up @@ -367,6 +374,37 @@ def test_checkpoint_deserialization_failure(

assert expected_error in str(e.value)

@pytest.mark.unit
def test_checkpoint_deserialization_with_actions(self, mocker: MockerFixture):
# Arrange
context = mocker.Mock(spec=AbstractDataContext)
context.validation_definition_store.get.return_value = mocker.Mock(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deserializing a checkpoint hits the validation definition store?

spec=ValidationDefinition
)
set_context(context)

# Act
serialized_checkpoint = {
"actions": [
{"site_names": [], "type": "update_data_docs"},
{"slack_webhook": "test", "type": "slack"},
{"teams_webhook": "test", "type": "microsoft"},
],
"id": "e7d1f462-821b-429c-8086-cca80eeea5e9",
"name": "my_checkpoint",
"result_format": "SUMMARY",
"validation_definitions": [
{"id": "3fb9ce09-a8fb-44d6-8abd-7d699443f6a1", "name": "my_validation_def"}
],
}
checkpoint = Checkpoint.parse_obj(serialized_checkpoint)

# Assert
assert len(checkpoint.actions) == 3
assert isinstance(checkpoint.actions[0], UpdateDataDocsAction)
assert isinstance(checkpoint.actions[1], SlackNotificationAction)
assert isinstance(checkpoint.actions[2], MicrosoftTeamsNotificationAction)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably add another test here to verify that (pseudocode) serialized_checkpoint == serialize(deserialize(checkpoint))



class TestCheckpointResult:
suite_name: str = "my_suite"
Expand Down Expand Up @@ -455,7 +493,9 @@ def test_checkpoint_run_no_actions(self, validation_definition: ValidationDefini

@pytest.mark.unit
def test_checkpoint_run_actions(
self, validation_definition: ValidationDefinition, actions: list[ValidationAction]
self,
validation_definition: ValidationDefinition,
actions: list[CheckpointAction],
):
validation_definitions = [validation_definition]
checkpoint = Checkpoint(
Expand Down