Skip to content

Static typing: resolved_headers_field is not Optional #4147

Closed

Description

@tonnico

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]:

Activity

  1. added
    triagePending triage from maintainers
    typingStatic typing definition related issues (mypy, pyright, etc.)
    on Apr 18, 2024
  2. rubenfonseca commented on Apr 18, 2024

    @rubenfonseca
    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

  3. added
    triagePending triage from maintainers
    and removed
    triagePending triage from maintainers
    on Apr 18, 2024
  4. leandrodamascena commented on Apr 18, 2024

    @leandrodamascena
    Contributor

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

  5. rubenfonseca commented on Apr 18, 2024

    @rubenfonseca
    Contributor

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

  6. added
    v3Features that will be included in Powertools v3.
    and removed
    triagePending triage from maintainers
    on Apr 18, 2024
  7. added this to the Powertools v3 milestone on Apr 18, 2024
  8. tonnico commented on Apr 18, 2024

    @tonnico
    ContributorAuthor

    @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.

  9. heitorlessa commented on May 7, 2024

    @heitorlessa
    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

  10. heitorlessa commented on May 7, 2024

    @heitorlessa
    Contributor

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

  11. github-actions commented on May 7, 2024

    @github-actions
    Contributor

    ⚠️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.

  12. heitorlessa commented on May 7, 2024

    @heitorlessa
    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.

  13. moved this from Coming soon to Shipped in Powertools for AWS Lambda (Python)on Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    typingStatic typing definition related issues (mypy, pyright, etc.)v3Features that will be included in Powertools v3.

    Type

    No type

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions