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 1 commit
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
24 changes: 22 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, # noqa: TCH001
MicrosoftTeamsNotificationAction, # noqa: TCH001
OpsgenieAlertAction, # noqa: TCH001
PagerdutyAlertAction, # noqa: TCH001
SlackNotificationAction, # noqa: TCH001
SNSNotificationAction, # noqa: TCH001
StoreValidationResultAction, # noqa: TCH001
UpdateDataDocsAction, # noqa: TCH001
)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than ignoring all these warnings I would add these (or their immediate parent) here.

runtime-evaluated-base-classes = [
# NOTE: ruff is unable to detect that these are subclasses of pydantic.BaseModel
"pydantic.BaseModel",
"great_expectations.datasource.fluent.fluent_base_model.FluentBaseModel",
"great_expectations.datasource.fluent.interfaces.Datasource",
"great_expectations.datasource.fluent.sql_datasource.SQLDatasource",
]

Copy link
Member

@Kilo59 Kilo59 Apr 3, 2024

Choose a reason for hiding this comment

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

Actually sorry, you need Checkpoint here, or pydantic.v1.BaseModel not these actions.

"pydantic.BaseModel",
"pydantic.v1.BaseModel",
"great_expectations.compatibility.pydantic.BaseModel",
# if adding these doesn't fix it you can add the Checkpoint model directly.

from great_expectations.compatibility.pydantic import BaseModel, root_validator, validator
from great_expectations.core.expectation_validation_result import (
ExpectationSuiteValidationResult, # noqa: TCH001
Expand Down Expand Up @@ -45,7 +54,18 @@ class Checkpoint(BaseModel):

name: str
validation_definitions: List[ValidationDefinition]
actions: List[ValidationAction]
actions: List[
Union[
EmailAction,
MicrosoftTeamsNotificationAction,
OpsgenieAlertAction,
PagerdutyAlertAction,
SlackNotificationAction,
SNSNotificationAction,
StoreValidationResultAction,
UpdateDataDocsAction,
]
]
result_format: ResultFormat = ResultFormat.SUMMARY
id: Union[str, None] = None

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
32 changes: 32 additions & 0 deletions tests/checkpoint/test_v1_checkpoint.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from great_expectations.checkpoint.actions import (
MicrosoftTeamsNotificationAction,
SlackNotificationAction,
UpdateDataDocsAction,
ValidationAction,
)
from great_expectations.checkpoint.v1_checkpoint import Checkpoint, CheckpointResult
Expand Down Expand Up @@ -367,6 +368,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