DEVC-601: Add app logic for meaningful error when mismatched app type is used by kossman · Pull Request #91 · corva-ai/python-sdk

kossman · 2025-01-25T10:36:42Z

Rationale

We have to add changes to be aligned with node-sdk changes
node-sdk Jira task
node-sdk PR

Changes

Mainly, changes are related to two validations:

check app_type declared at manifest.json file and compare it with raw_event_type passed at specific app decorator to base_handler and raise an error if a mismatch happens
check app_type based on the event payload passed to lambda_handler and app decorator itself, the logic here is the same if they aren't matched - raise the error
So thus, changes were encapsulated into function validate_app_type_context which can handle both 1 and 2 corva app use cases.

Under the hood validate_app_type_context has:

validate_manifested_type: will be called only if get_manifested_type() could extract correct app_type name from manifest.json file
validate_event_payload: will be called otherwise. Under the hood get_event_type func will trying to find the appropriate child cls inherited from RawBaseEvent and then - validate matching.

The idea is the following: each existing (and future added) specific event type eventually should be inherited from one of 3 raw Raw{APP_TYPE}Event classes, so we can just check this "belonging"

JIRA ticket

TODO

Update CHANGELOG.md

…PPING

…anager for multiple with blocks

…10 & 3.11

…tora playbook

vladbagmet

praise: Awesome description for the pull request! 👍

question (blocking): Can validation be done inside base_handler?

vladbagmet · 2025-01-28T13:38:36Z

    func: Callable,
    raw_event_type: Type[RawBaseEvent],
    handler: Optional[logging.Handler],
+    app_decorator_type: str,


question (blocking): Can we use raw_event_type to identify type?

Ohh, yeah, great idea! Thanks for catching it! 🙌

vladbagmet · 2025-01-28T13:41:37Z

+from corva.models.stream.raw import RawStreamEvent
+from corva.models.task import RawTaskEvent
+
+BASE_EVENT_CLS_TO_APP_TYPE_MAPPING: Dict[str, Type[RawBaseEvent]] = {


question (bloacking): Do we really need to have this mapping? RawTaskEvent, RawStreamEvent and RawScheduledEvent have the same parent class.

After applying changes proposed here I've removed this BASE_EVENT_CLS_TO_APP_TYPE_MAPPING at all
Thanks!

vladbagmet · 2025-01-28T13:55:30Z

+
+def validate_manifested_type(manifest: Dict[str, Any], app_decorator_type: str) -> None:
+    manifest_app_type = manifest.get("application", {}).get("type")
+    if manifest_app_type and manifest_app_type != app_decorator_type:


question (blocking): What if we have manifest.json without type. Should this validation pass in this case?

Also updated the logic for handle such cases, I believe if app type missed at manifest.json we anyway should try to validate app call basing on event payload.
But at the same time I've also added check for app_type, so if manifest has wrong app_type code will throw an error, please take a look over this test 🙏

vladbagmet · 2025-01-30T08:53:47Z

                if merge_events:
                    aws_event = _merge_events(aws_event, data_transformation_type)
+
+                validate_app_type_context(aws_event, raw_event_type)


question(blocking): Should validation happen before calling anything else?

@vladbagmet I've refactored a bit base_handler body 🙏
But anyway as I see, more convenient way to place validate_app_type_context func right after _merge_events(...) coz in this case merging validation logic called first + it's easier to guess_event_type(aws_event) if the event already merged 🤔 wdyt?

vladbagmet · 2025-01-30T08:55:49Z

+        )
+
+
+@functools.lru_cache(maxsize=None)


question (non-blocking): Is this cache going to be used between lamba function calls? If, so why don't we limit number of cached instances?

you're right, limited to 1, thanks 🙌

vladbagmet · 2025-01-30T08:59:20Z

 import pydantic


+class AppType(str, Enum):


note: I like that we do not have mapping dict now. Also, is there a way to use already existing models for event to identify the app type?

I'm not sure that I've got your point, could you please explain a bit wider 🙏

neatpick: Seems like AppType is type of app defined in manifest.json.

vladbagmet · 2025-01-30T09:03:33Z

    raw_event["event_type"] = "unknown_event_type"
-    with pytest.raises(ValidationError) as e:
+
+    with pytest.raises(RuntimeError) as e:


question (non-blocking): If app type is correct, but schema of this event is corrupted, should ValidationError still be raised?

Seems you're right, I've updated a bit app logic and have reverted the test logic accordingly, thanks for reviewing 🙌

vladbagmet · 2025-02-07T10:18:06Z

+            if merge_events:
+                aws_event = _merge_events(aws_event, data_transformation_type)
+
+            if is_direct_app_call:


question (blocking): Should more specific check goes first?

vladbagmet · 2025-02-07T10:33:49Z

        pass
+
+    @classmethod
+    def get_app_type(cls) -> AppType:


issue(blocking): Event payload types and type defined in manifest.json are not the same. What do you think about removing this method from the interface and do types comparison in "validate_app_type_context" method.

vladbagmet · 2025-02-07T10:37:10Z

+    return leaf_classes
+
+
+def guess_event_type(aws_event: Any) -> Optional[Type[RawBaseEvent]]:


question(non-blocking): Can we rename it to get_event_type?

vladbagmet · 2025-02-07T10:41:48Z

+            )
+
+
+@functools.lru_cache(maxsize=1)


though(non-blocking): Please be advised that this approach can not work between lamba calls.

But anyway, I think we can keep such logic here to be prepared for provisioned_concurrency in the future after changes on packager side are rolled out 🙏

vladbagmet

Blocking things are:

Revert changes (def get_app_type(cls) -> AppType:)
Change order of checks.

kossman · 2025-02-07T11:27:01Z

Blocking things are:

Revert changes (def get_app_type(cls) -> AppType:)

Change order of checks.

@vladbagmet proposed changes are integrated, thanks!

vladbagmet

praise: Great job!

🥇

add app logic for meaningful error when mismatched app type is used

27eed84

kossman self-assigned this Jan 25, 2025

kossman added 16 commits January 25, 2025 14:38

fix patch call for read_manifest func

c216d32

fix lint

fa5e7a5

fix type checkers

4404c62

fix tests for test_apps_event_types.py

21de5cb

revert some changes; optimize usage for BASE_EVENT_CLS_TO_APP_TYPE_MA…

7ee1ddb

…PPING

add more precise tests for new functionality

b7bb304

add matrix testing on different py versions; fix error with context m…

84a68a4

…anager for multiple with blocks

fix workflow

473a152

fix workflow

837556d

trying to fix pytest errors on newer version 6.2.5 for tests on py 3.…

c099de7

…10 & 3.11

fix linting

c13c6a9

small test improvement

5f6b248

ignore errors at matrix job running

463950c

some tests using continue-on-error=true

49837f7

revert temp changes

12f174e

bump new minor version for corva python-sdk: update CHANGELOG.md & an…

c2c25da

…tora playbook

kossman requested review from oleksii-symon-corva-ai and vladbagmet January 27, 2025 12:14

kossman marked this pull request as ready for review January 27, 2025 12:14

vladbagmet requested changes Jan 28, 2025

View reviewed changes

kossman added 3 commits January 28, 2025 19:44

feat: update SDK logic to be more transparent and optimal

c6c541f

feat: fix linting

306b081

fix: linter

2209c20

kossman requested a review from vladbagmet January 28, 2025 16:00

vladbagmet requested changes Jan 30, 2025

View reviewed changes

kossman added 3 commits January 30, 2025 18:54

add some fixes; code improvements

a5c970a

fix format

f442cd0

fix typo for manifest.json scheduled app type

21fce8c

kossman changed the title ~~add app logic for meaningful error when mismatched app type is used~~ feat(DEVC-601): Add app logic for meaningful error when mismatched app type is used Feb 7, 2025

kossman changed the title ~~feat(DEVC-601): Add app logic for meaningful error when mismatched app type is used~~ DEVC-601: Add app logic for meaningful error when mismatched app type is used Feb 7, 2025

vladbagmet reviewed Feb 7, 2025

View reviewed changes

vladbagmet self-requested a review February 7, 2025 10:42

vladbagmet requested changes Feb 7, 2025

View reviewed changes

kossman added 2 commits February 7, 2025 15:19

fix: update code according to last PR comments requested

b221f8b

fix: linting

2dea522

kossman requested a review from vladbagmet February 7, 2025 11:28

vladbagmet approved these changes Feb 7, 2025

View reviewed changes

kossman merged commit 0136cd9 into master Feb 10, 2025

kossman deleted the feature/DEVC-601-meaningful-error-on-mismatched-app-type-used branch February 10, 2025 08:22

		return leaf_classes


		def guess_event_type(aws_event: Any) -> Optional[Type[RawBaseEvent]]:

		)


		@functools.lru_cache(maxsize=None)

		)


		@functools.lru_cache(maxsize=1)

Conversation

kossman commented Jan 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Changes

TODO

Uh oh!

vladbagmet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladbagmet left a comment

Choose a reason for hiding this comment

Uh oh!

kossman commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladbagmet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kossman commented Jan 25, 2025 •

edited

Loading

kossman commented Feb 7, 2025 •

edited

Loading