slank/awsgi

new select_impl breaks if requestContext not present

Closed this issue · 5 comments

I'm not sure whether requestContext is always provided from lambda event, but we have a use case where we're creating the event dict ourselves which did not contain a requestContext. New code to support elb tries to access event['requestContext'] which fails in that case. Would be easy enough to use .get(), I'll submit a PR.

slank commented

Hey, @chadawagner. Thank you very much for your contribution.

I have some hesitation with this approach. In the case of requestContext, I think it makes sense to choose the APIGW implementation as a default. For the other changes you've made in your PR, particularly for REQUEST_METHOD, I suspect failure is the correct thing to do as absence of some of those values indicates a bad request from which a sensible response cannot be constructed. I would rather fail than return an incorrect response.

Thanks for the quick feedback!

Ok, if APIGW for sure always sends requestContext we can just add it to our event, but since that lookup was just added and we only care whether it contains elb but aren't using it otherwise, it seemed safer to not introduce a new potential exception.

I was on the fence about the others. I wasn't going to change them but then looked at how werkzeug is handling those and borrowed their defaults. Seems like at least query params could be optional, but I'm ok with requiring all of them since it was already that way.

Either way we can work with it, so whatever you think best. Thanks!

slank commented

Thanks. Would you mind resubmitting your PR, changing only select_impl?

Sure, I just updated the existing PR.

slank commented

Thanks!