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

Feature request: Return empty Dict or List in Event Source Data Classes instead of None #2605

Closed
1 of 2 tasks
ericbn opened this issue Jun 28, 2023 · 10 comments · Fixed by #4606
Closed
1 of 2 tasks
Labels
breaking-change Breaking change feature-request feature request help wanted Could use a second pair of eyes/hands need-customer-feedback Requires more customers feedback before making or revisiting a decision need-rfc revisit Maintainer to provide update or revisit prioritization in the next cycle v3 Features that will be included in Powertools v3.

Comments

@ericbn
Copy link
Contributor

ericbn commented Jun 28, 2023

Use case

Arguably, it might be easier for users if an empty Dict is returned instead of None for path_parameters in APIGatewayProxyEventV2, for example.

Instead of:

@event_source(data_class=APIGatewayProxyEventV2)
def lambda_handler(event: APIGatewayProxyEventV2, context) -> None:
    parameters = event.path_parameters or {}
    handle(parameters["id"])

it would become:

@event_source(data_class=APIGatewayProxyEventV2)
def lambda_handler(event: APIGatewayProxyEventV2, context) -> None:
    handle(event.path_parameters["id"])

Ultimately this approach could be followed for any return value that is either an Optional[Dict] or an Optional[List], returning an empty Dict or List instead of None in all cases:

  • utilities/data_classes/api_gateway_proxy_event.py
    • APIGatewayEventAuthorizer
      • def claims(self) -> Dict[str, Any]
      • def scopes(self) -> List[str]
    • APIGatewayProxyEvent
      • def multi_value_query_string_parameters(self) -> Dict[str, List[str]]
      • def path_parameters(self) -> Dict[str, str]
      • def stage_variables(self) -> Dict[str, str]
    • RequestContextV2AuthorizerIam
      • def cognito_amr(self) -> List[str]
    • RequestContextV2Authorizer
      • def jwt_claim(self) -> Dict[str, Any]
      • def jwt_scopes(self) -> List[str]
      • def get_lambda(self) -> Dict[str, Any]
    • APIGatewayProxyEventV2
      • def cookies(self) -> List[str]
      • def path_parameters(self) -> Dict[str, str]
      • def stage_variables(self) -> Dict[str, str]

and so on...

The bottom line is: Does it matter for users to differentiate between None and an empty Dict or List for any Event Source Data Classe property? If it does for a particular one, it an exception to a rule or the rule?

This change would be backwards compatible with the previous code.

This approach was already implemented in only one place so far, as far as I could check:

  • utilities/data_classes/common.py
    • BaseProxyEvent
      • def headers(self) -> Dict[str, str]

Solution/User Experience

Solution can be the same as for the headers in the BaseProxyEvent:

@property
def headers(self) -> Dict[str, str]:
return self.get("headers") or {}

Alternative solutions

No response

Acknowledgment

@rubenfonseca
Copy link
Contributor

Hi @ericbn thank you for sending this proposal! Two notes:

  1. Do you agree that code that backwards compatibility can be ensured since existing code that relies on None as a valid value will still function correctly?
  2. Providing specific use cases and examples where this feature significantly improves the developer experience would strengthen the proposal.

It seems to be this would be a big breaking change anyways. So we might have to open an RFC to discuss this with the larger community and consider the breaking change for v3.

@rubenfonseca rubenfonseca self-assigned this Jun 29, 2023
@rubenfonseca rubenfonseca added need-customer-feedback Requires more customers feedback before making or revisiting a decision revisit Maintainer to provide update or revisit prioritization in the next cycle and removed triage Pending triage from maintainers labels Jun 29, 2023
@rubenfonseca rubenfonseca moved this from Triage to Pending customer in Powertools for AWS Lambda (Python) Jun 29, 2023
@heitorlessa heitorlessa added the breaking-change Breaking change label Jul 10, 2023
@heitorlessa heitorlessa moved this from Pending customer to On hold in Powertools for AWS Lambda (Python) Jul 10, 2023
@ericbn
Copy link
Contributor Author

ericbn commented Jul 11, 2023

Hi. Totally agree this is a breaking change for existing code that relies on None as a valid value. Good point. I see the change in the headers method in BaseProxyEvent was done exactly to keep backward compatibility.

I propose this change is favor of an API with multiple methods like get_query_string_value and get_header_value in BaseProxyEvent.

def get_query_string_value(self, name: str, default_value: Optional[str] = None) -> Optional[str]:
"""Get query string value by name
Parameters
----------
name: str
Query string parameter name
default_value: str, optional
Default value if no value was found by name
Returns
-------
str, optional
Query string parameter value
"""
return get_query_string_value(
query_string_parameters=self.query_string_parameters,
name=name,
default_value=default_value,
)

# Maintenance: missing @overload to ensure return type is a str when default_value is set
def get_header_value(
self,
name: str,
default_value: Optional[str] = None,
case_sensitive: Optional[bool] = False,
) -> Optional[str]:
"""Get header value by name
Parameters
----------
name: str
Header name
default_value: str, optional
Default value if no value was found by name
case_sensitive: bool
Whether to use a case-sensitive look up
Returns
-------
str, optional
Header value
"""
return get_header_value(
headers=self.headers,
name=name,
default_value=default_value,
case_sensitive=case_sensitive,
)

Pros:

  • No need to create extra methods in the data classes.
  • User can use the List and Dict API directly, for example to choose between event.query_string_parameters["foo"] or event.query_string_parameters.get("foo") or event.query_string_parameters.get("foo", "default_foo") in case the key is required or not, or to use other functions like first(event.cookies).

Cons(?):

  • As the List and Dict APIs are used directly, a parameter like case_sensitive in get_header_value would have to be replaced by returning a custom CaseInsensitiveDict.

@leandrodamascena
Copy link
Contributor

leandrodamascena commented Aug 17, 2023

Hello @ericbn! We've updated our roadmap and included this feature request. We are planning to release this feature in Powertools v3.

Roadmap for v3: https://docs.powertools.aws.dev/lambda/python/latest/roadmap/#v3

@leandrodamascena leandrodamascena moved this from On hold to Backlog in Powertools for AWS Lambda (Python) Aug 17, 2023
@ericbn
Copy link
Contributor Author

ericbn commented Aug 20, 2023

Hi @leandrodamascena, cool! I'll be happy to provide a PR if that sounds good. Thanks for considering and accepting the feature request!

@leandrodamascena
Copy link
Contributor

leandrodamascena commented Aug 20, 2023

Hello @ericbn! We definitely want you to submit a PR and contribute to the project, this idea of yours brings more consistency to the use of a data class. I just would like to ask is that you wait until we create the Powertools v3 RFC and have the plan defined. If you send the PR now it will take a long time without updating and it will be hard to merge in the future.

I created a new label v3 and added it to this issue, so it's easier for us to track what we need to include in the RFC for Powertools v3.

Thank you.

@leandrodamascena leandrodamascena added the v3 Features that will be included in Powertools v3. label Aug 20, 2023
@ericbn
Copy link
Contributor Author

ericbn commented Aug 23, 2023

Sure, I’ll wait. Makes perfect sense. Looking forward for my first contribution with a PR to the project!

@heitorlessa
Copy link
Contributor

Took a long time (stability/backwards compatibility) but we've started implementing for v3: #4189

@heitorlessa heitorlessa moved this from Pending customer to Working on it in Powertools for AWS Lambda (Python) Jun 10, 2024
@heitorlessa heitorlessa added the help wanted Could use a second pair of eyes/hands label Jun 10, 2024
@ericbn
Copy link
Contributor Author

ericbn commented Jun 10, 2024

Oi @heitorlessa. I'll be happy to work on this as a contribution to the project.

@leandrodamascena
Copy link
Contributor

Closed via #4606

@github-project-automation github-project-automation bot moved this from Working on it to Coming soon in Powertools for AWS Lambda (Python) Jul 29, 2024
Copy link
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Breaking change feature-request feature request help wanted Could use a second pair of eyes/hands need-customer-feedback Requires more customers feedback before making or revisiting a decision need-rfc revisit Maintainer to provide update or revisit prioritization in the next cycle v3 Features that will be included in Powertools v3.
Projects
Status: Coming soon
Development

Successfully merging a pull request may close this issue.

4 participants