Allow extended parser models to pass mypy

[ClaytonJY]

Is your feature request related to a problem? Please describe.

The docs example for extending parser models (https://awslabs.github.io/aws-lambda-powertools-python/latest/utilities/parser/#extending-built-in-models) doesn't pass mypy, because the Order class is not compatible with the EventBridgeModel annotation detail: Dict[str, Any].

error: Incompatible types in assignment (expression has type "Order", base class "EventBridgeModel" defined the type as "Dict[str, Any]")

I would like to be able to both extend this model and have my code pass type checks without # type: ignore or similar.

I suspect this affects other parser models, like the Message field of SnsNotificationModel.

Describe the solution you'd like

One solution is to change only the detail type annotation, e.g. Union[Dict[str, Any], BaseModel], but there may be other things to add to that list and it doesn't feel great; there could be many other types that would parse fine and could be listed there, right? Or is that it, because you either use EventBridgeModel directly (Dict) or you subclass it (BaseModel)?

Is there anything that could be done with typing.Protocol, to check the class has certain methods regardless of name?

There might also be an argument to not change the type, but note in the docs that this will fail type-checks and instruct how to silence mypy in such instances.

Describe alternatives you've considered

I tried various solutions which didn't work, like adding dict or TypedDict to the parent class list for Order (incompatible methods), or changing it to TypedDict (same error as above).

Additional context

I'm using the pydantic.mypy extension.

[boring-cyborg]

Thanks for opening your first issue here! We'll come back to you as soon as we can.

[heitorlessa]

hey @ClaytonJY thanks a lot for flagging this. I'm not entirely sure how to solve this yet, we'd have to maybe experiment with a generic type bound to BaseModel.

cc @ran-isenberg in case he knows as this should be a fairly common problem in Pydantic world

[ran-isenberg]

I didn't encounter this issue yet to be honest.

[ClaytonJY]

FWIW I'm a very new user of this package, and since posting this have come to avoid the issue a different way: use the envelope and never bother subclassing that model. Still, given the benefits of this package around typing, it would be nice if examples in the docs like this were type compliant.

Happy to help, but may need to be pointed in the right direction. Very curious to see where this discussion goes, and thank you for a package that has cut at least half of my code just a few hours after discovering it 🙏

[ran-isenberg]

FWIW I'm a very new user of this package, and since posting this have come to avoid the issue a different way: use the envelope and never bother subclassing that model. Still, given the benefits of this package around typing, it would be nice if examples in the docs like this were type compliant.

Happy to help, but may need to be pointed in the right direction. Very curious to see where this discussion goes, and thank you for a package that has cut at least half of my code just a few hours after discovering it 🙏

Subclassing has a lot of advantages -if you require both metadata and business logic (body/details) attributes.

I guess mypy doesnt like that we change the type of the baseclass attribute. I'm not sure how you can avoid that, that's the whole point of the parsing trick we do there.

Union[Dict[str, Any], BaseModel] is fairly accurate but I'm afraid it might break something because I do want it to parse it a Dict for the regular envelope/model extraction flows.

One "ugly" solution is to add # type: ignore to the relevant files.

BTW, I use pylint, and it's was fine with the correct definition.

[heitorlessa]

Hey! Once we address two P1 issues (Batch, Idempotency), I’ll try replicate with mypy. Clayton - could you share what mypy config you’re using when you have a chance? In theory, subclassing shouldn’t cause this issue with typing, as you’re replacing it. If it indeed does and there isn’t an elegant solution, happy to make a generic for all data fields (detail, body, etc) where you could pass a str or a type bounded to BaseModel.

…

On Sun, 5 Dec 2021 at 07:50, Ran Isenberg ***@***.***> wrote: FWIW I'm a very new user of this package, and since posting this have come to avoid the issue a different way: use the envelope and never bother subclassing that model. Still, given the benefits of this package around typing, it would be nice if examples in the docs like this were type compliant. Happy to help, but may need to be pointed in the right direction. Very curious to see where this discussion goes, and thank you for a package that has cut at least half of my code just a few hours after discovering it 🙏 Subclassing has a lot of advantages -if you require both metadata and business logic (body/details) attributes. I guess mypy doesnt like that we change the type of the baseclass attribute. I'm not sure how you can avoid that, that's the whole point of the parsing trick we do there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#849 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZPQBBYIWANW3IPL5TAJTTUPMDRZANCNFSM5I4SQ7RQ> .

[heitorlessa]

One thing I might have missed to ask is whether you’re using Pydantic mypy plugin, otherwise Mypy could indeed complain of the type change when subclassing - forgot to mention that in the docs https://pydantic-docs.helpmanual.io/mypy_plugin/

…

On Sun, 5 Dec 2021 at 08:52, Heitor Lessa ***@***.***> wrote: Hey! Once we address two P1 issues (Batch, Idempotency), I’ll try replicate with mypy. Clayton - could you share what mypy config you’re using when you have a chance? In theory, subclassing shouldn’t cause this issue with typing, as you’re replacing it. If it indeed does and there isn’t an elegant solution, happy to make a generic for all data fields (detail, body, etc) where you could pass a str or a type bounded to BaseModel. On Sun, 5 Dec 2021 at 07:50, Ran Isenberg ***@***.***> wrote: > FWIW I'm a very new user of this package, and since posting this have > come to avoid the issue a different way: use the envelope and never bother > subclassing that model. Still, given the benefits of this package around > typing, it would be nice if examples in the docs like this were type > compliant. > > Happy to help, but may need to be pointed in the right direction. Very > curious to see where this discussion goes, and thank you for a package that > has cut at least half of my code just a few hours after discovering it 🙏 > > Subclassing has a lot of advantages -if you require both metadata and > business logic (body/details) attributes. > > I guess mypy doesnt like that we change the type of the baseclass > attribute. I'm not sure how you can avoid that, that's the whole point of > the parsing trick we do there. > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#849 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAZPQBBYIWANW3IPL5TAJTTUPMDRZANCNFSM5I4SQ7RQ> > . >

[ClaytonJY]

@heitorlessa I no longer have access to that exact project where I discovered this, but I'm fairly certain the only mypy config I had set was using the pydantic plugin. I may have fed some command-line args to mypy like ignoring un-annotated imports. I'm quite certain I wasn't doing anything exotic!

I could whip up a minimal reprex if needed, would that help?

[heitorlessa]

Yes please! That would speed things up on our end when we get to it this week

…

On Mon, 6 Dec 2021 at 02:37, Clayton Yochum ***@***.***> wrote: @heitorlessa <https://github.com/heitorlessa> I no longer have access to that exact project where I discovered this, but I'm fairly certain the only mypy config I had set was using the pydantic plugin. I may have fed some command-line args to mypy like ignoring un-annotated imports. I'm quite certain I wasn't doing anything exotic! I could whip up a minimal reprex if needed, would that help? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#849 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZPQBDQI4GNS76ROMKBP73UPQHWTANCNFSM5I4SQ7RQ> .

[ClaytonJY]

@heitorlessa here's that reprex, as a new repo: https://github.com/ClaytonJY/powertools-type-error-example

thanks for caring!

[heitorlessa]

Looking at this today @ClaytonJY thank you so much again for creating the repro, it makes a lot easier for us :)

[heitorlessa]

As expected, a generic type bound to BaseModel in an Union makes MyPy happy. If the actual payload is passed and no override happens, it correctly detects detail as a dictionary.

It'd be impractical to allow any other field to be overridden by type, so I'll go ahead and ensure all built-in Models can be overridden on fields where the payload is (e.g. detail in EventBridge, body in API Gateway, etc.)

---

# Model = TypeVar("Model", bound=BaseModel)

from datetime import datetime
from typing import Any, Dict, List, Optional, Union

from pydantic import BaseModel, Field

from aws_lambda_powertools.utilities.parser.types import Model


class EventBridgeModel(BaseModel):
    version: str
    id: str  # noqa: A003,VNE003
    source: str
    account: str
    time: datetime
    region: str
    resources: List[str]
    detail_type: str = Field(None, alias="detail-type")
    detail: Union[Dict[str, Any], Model]
    replay_name: Optional[str] = Field(None, alias="replay-name")

[heitorlessa]

PR available - should be available in the next release: #883

[heitorlessa]

Now released as part of 1.23 version