Feature request: query_string_parameters should never be None

[a3kov]

Use case

When there are no query string params, app.current_event.query_string_parameters is None which is unexpected.

Solution/User Experience

Proper solution is to make it to behave like an empty dict or to be an actual empty dict.

• avoids extra None checks
• removes one point of surprise which was totally unnecessary

Alternative solutions

No response

Acknowledgment

• This feature request meets Lambda Powertools Tenets
• Should this be considered in other Lambda Powertools languages? i.e. Java, TypeScript

[boring-cyborg]

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

[heitorlessa]

Hi @a3kov, thank you for raising this. When you say it's "unexpected", could you expand on that please? I've just tried on both PyCharm and VSCode including Mypy and they flag that app.current_event.query_string_parameters could be None

That said, I agree that we could provide a better experience by supporting a sentinel value like we do in other cases like get_header_value - I'm not entirely sure on the best typing when a sentinel is present (@overload?); cc @peterschutt.

Thank you!!

[peterschutt]

Cheers for the ping @heitorlessa!

Is the parallel you are drawing to get_header_value() the case_sensitive flag? If I get your drift you're talking about an accessor func or method that would always return an empty dict in place of None if given some sentinel? If so, sure overload would do it.

On the overall issue I've got an example event log in front of me and it has "queryStringParameters":null in it - so the property definitely isn't returning anything silly.

If I was OP I'd probably just create a local reference with query = app.current_event.query_string_parameters or {}.

[heitorlessa]

Yes @peterschutt. Like a get_query_string_parameters, where you'd have a parameter to define a sentinel value:

params = app.curent.event.get_query_string_parameters("message", default={}) # dict return
my_param = app.curent.event.get_query_string_parameters("message", default="") # str in this case

Challenge here is that we can't control what the sentinel value would be, so I suspect we could solve with an @overload and a generic type for the sentinel, like os.getenv does: https://github.com/python/typeshed/blob/master/stdlib/os/__init__.pyi#L496

In your experience, is that the best way to handle it?

If so, I'd be open to have a PR to add get_query_string_parameters under BaseProxyEvent so API Gateway, ALB, and the upcoming Function URL can benefit.

[heitorlessa]

And yes, I also agree that defining a local or {} is better in the current case since we are explicit with our return type being Optional.

The only added benefit of having a get_query_string_parameters function is saving an "or" for those not used with static typing and the tooling around it - i'm torn, maybe need more customer demand for this, and can update the docs with an or {} when accessing query strings in the meantime

[a3kov]

@heitorlessa I meant unexpected coming from web frameworks. It's just not practical to have it as None sometimes

[peterschutt]

As @a3kov is thinking about it from a web framework perspective, an analogous solution could be a method on the resolver that always returns a dict, e.g., APIGatewayResolver.query_parameters() -> dict. However, I can't see a precedent for that kind of thing, and it could be a slippery slope of needing to add methods for accessing other things like path params, headers, etc.

maybe need more customer demand for this, and can update the docs with an or {} when accessing query strings in the meantime

Always harder to remove code than to add it:)

[sthuber90]

I understand it would be a nicer experience if you could always do app.current_event.query_string_parameters.get("id") and not experience his error:

AttributeError: 'NoneType' object has no attribute 'get'

However, I see the or {} as a good enough solution. Alternatively, you can also use get_query_string_value. Even if no query parameters have been provided this call will not fail and you can set a default value to return if the value would be None.

# returns ""
app.current_event.get_query_string_value(name="id", default_value="")

# returns "dummy_id"
app.current_event.get_query_string_value(name="id", default_value="dummy_id")

[ppavlovdev]

@a3kov hi!

I think it's a good scenario for monads (you should be scared here lol). In this particular case I'd use Maybe monad with function binding/piping. Take a look at this project that implements and, what is more important, describes monads in a very "gentle" way.

So let's suppose we use this library in our project/lambda. How does our code can look like:

import json
import os
from typings import Optional

import boto3
from returns.maybe import Maybe


def dynamodb_query(qs: dict) -> dict:
    dynamo = boto3.resource("dynamodb")
    table = dynamo.Table(os.getenv("TABLE_NAME", "table_name"))
    return table.query(**qs)


@app.get('/')
def list() -> Response:
    qs: Optional[dict] = app.current_event.query_string_parameters
    qs: Maybe[dict] = Maybe.from_optional(qs)  # Some(dict) or Nothing
    
    # magic starts here
    # it will execute `dynamodb_query` only if `qs` != `Nothing`
    query_result: Maybe[dict] = qs.bind_optional(dynamodb_query)

    response_body = {
        "items": query_result.value_or([])
    }
    return Response(200, "application/json", json.dumps(response_body))

cc @heitorlessa @peterschutt @sthuber90

[sthuber90]

@ppavlovdev Thank you for introducing me to returns. Seems to be a very interesting project. However, for powertools, I feel the use case to be too specific to justify adding a dependency for it. Also, as there are have been other options provided before, I would refrain from adding something like Maybe. Personally, I cannot see how it would help in this case.

[ppavlovdev]

@sthuber90 I think I described my idea not very well sorry. I didn't mean to integrate returns right into powertools. It's too specific as you said. I just introduced to the author of this issue another way how to deal with "extra None checks" and "one point of surprise which was totally unnecessary".

[a3kov]

The point is not whether or not its easy to check manually (it is). The point is if you want to have better experience for developers. I understand if somebody was checking for None and you make it a dict, it will break their code.
I am not using this project, so I am not emotionally attached to the subject anymore.

[leandrodamascena]

Since it's a potential breaking change, I'm adding this issue to the Powertools v3 milestone and we can revisit it when we plan for v3.

[ericbn]

This should be solved by the more general #2605. PR is here: #4606.

[leandrodamascena]

@ericbn is working on PR #4606 and this PR closes this issue.

[leandrodamascena]

Closed via #4606

[github-actions]

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