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

Static typing: resolved_headers_field is not Optional #4147

Closed
Wurstnase opened this issue Apr 18, 2024 · 8 comments · Fixed by #4148
Closed

Static typing: resolved_headers_field is not Optional #4147

Wurstnase opened this issue Apr 18, 2024 · 8 comments · Fixed by #4148
Labels
typing Static typing definition related issues (mypy, pyright, etc.) v3 Features that will be included in Powertools v3.

Comments

@Wurstnase
Copy link
Contributor

Wurstnase commented Apr 18, 2024

Static type checker used

pyright/pylance

AWS Lambda function runtime

3.12

Powertools for AWS Lambda (Python) version

latest

Static type checker info

Expression of type "Dict[str, Any] | None" cannot be assigned to declared type "dict[str, list[str]]"
  Type "Dict[str, Any] | None" cannot be assigned to type "dict[str, list[str]]"
    "None" is incompatible with "dict[str, list[str]]"

Code snippet

import json
from aws_lambda_powertools.event_handler.router import ALBRouter
router = ALBRouter()
@router.get("/")
def get_headers() -> str:
    headers: dict[str, list[str]] = router.current_event.resolved_headers_field
    return json.dumps(headers)


### Possible Solution

Remove `Optional` 

```diff
- def resolved_headers_field(self) -> Optional[Dict[str, Any]]:
+ def resolved_headers_field(self) -> Dict[str, Any]:
@Wurstnase Wurstnase added triage Pending triage from maintainers typing Static typing definition related issues (mypy, pyright, etc.) labels Apr 18, 2024
@rubenfonseca
Copy link
Contributor

Hi @Wurstnase I'm not sure I follow this one. resolved_headers_field is currently marked as returning an Optional, meaning that in some cases it can return None. Shouldn't you handle that in your code instead of changing the API signature?

One could argue that checking the implementation, the field is always present. But due to Hyrum's Law I would prefer to add this to our v3 branch (more on that earlier next week).

@leandrodamascena I would appreciate your thoughts here too

@rubenfonseca rubenfonseca added triage Pending triage from maintainers breaking-change Breaking change and removed triage Pending triage from maintainers labels Apr 18, 2024
@rubenfonseca rubenfonseca moved this from Triage to Ideas in Powertools for AWS Lambda (Python) Apr 18, 2024
@rubenfonseca rubenfonseca linked a pull request Apr 18, 2024 that will close this issue
7 tasks
@leandrodamascena
Copy link
Contributor

Yes! I agree with you @rubenfonseca that we should avoid doing this now and tag this issue/pr as eligible for Powertools V3.

@rubenfonseca
Copy link
Contributor

Hope that's ok with you @Wurstnase. I'll add this to our v3 queue. I promise v3 is not far ahead ;)

@rubenfonseca rubenfonseca added v3 Features that will be included in Powertools v3. and removed triage Pending triage from maintainers labels Apr 18, 2024
@rubenfonseca rubenfonseca moved this from Ideas to Backlog in Powertools for AWS Lambda (Python) Apr 18, 2024
@rubenfonseca rubenfonseca added this to the Powertools v3 milestone Apr 18, 2024
@Wurstnase
Copy link
Contributor Author

Wurstnase commented Apr 18, 2024

@rubenfonseca @leandrodamascena
I can't see how this code ever can become None?

def resolved_headers_field(self) -> Optional[Dict[str, Any]]:
        headers: Dict[str, Any] = {}

        if self.multi_value_headers:
            headers = self.multi_value_headers
        else:
            headers = self.headers
        return {key.lower(): value for key, value in headers.items()}

As you can see, finally we return a dict at any time. It is impossible to return None.
If something is None it will raise an exception.

At least this could become an empty dict, but never None.

@heitorlessa
Copy link
Contributor

Nico is correct, headers will be an empty dict in the worst case -

That said, there's a few other places for its associated PR to be statically correct., Sending fixes today

@heitorlessa heitorlessa removed the breaking-change Breaking change label May 7, 2024
@heitorlessa
Copy link
Contributor

fixes pushed, tests suite ran successfully. Pending PR CI suite

Copy link
Contributor

github-actions bot commented May 7, 2024

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

@heitorlessa
Copy link
Contributor

Everyone - appreciate the help and cautious in raising the bar for everyone's experience. Merged, ran e2e tests, and pushed additional fixes to complete the PR.

Bottom line: headers are present, when it is not we default to {}, so we were overly cautious on Optional there.

@heitorlessa heitorlessa moved this from Coming soon to Shipped in Powertools for AWS Lambda (Python) Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typing Static typing definition related issues (mypy, pyright, etc.) v3 Features that will be included in Powertools v3.
Projects
Status: Shipped
Development

Successfully merging a pull request may close this issue.

4 participants