Skip to content

feat(event_handler): add cookies as 1st class citizen in v2 #1487

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

Merged
merged 10 commits into from
Sep 2, 2022

Conversation

rubenfonseca
Copy link
Contributor

@rubenfonseca rubenfonseca commented Aug 31, 2022

Issue number: #1192

Summary

This PR adds first class support for Cookies. After the last PR where we added multiple header values and cookies, we noticed we could improve the experience of Cookies by having a strongly typed class based approach.

carbon (15)

Changes

Please provide a summary of what's being changed

I've added a class for Cookie modeling the most common options that are used today according to the RFC.

It also updated and extends the E2E tests to be more comprehensive and generic.

User experience

Please share what the user experience looks like before and after this change

Before, the user would have to encode the cookie himself, producing a string with the desired output:

return Response(..., cookies=["session_id=12345; Secure; HttpOnly"])

This would be error prone, as getting the string right can be the source of bugs.

With the new class based Cookie, this becomes simpler:

return Response(..., cookies=[Cookie(name="session_id", value="12345", secure=True, http_only=True])

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 31, 2022
@github-actions github-actions bot added the feature New feature or functionality label Aug 31, 2022
@rubenfonseca rubenfonseca force-pushed the feat/cookies-v2 branch 2 times, most recently from 97bd70f to e93ae12 Compare August 31, 2022 08:55
@rubenfonseca
Copy link
Contributor Author

@heitorlessa @leandrodamascena would appreciate early feedback on this one before I continue to apply changes

Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super clean, I loved it. Some suggestions and two questions on why not enabling httpOnly and Secure attrs by default (there's probably good reasons that I prolly don't know)

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Sep 2, 2022
@rubenfonseca rubenfonseca marked this pull request as ready for review September 2, 2022 14:13
@rubenfonseca rubenfonseca requested a review from a team as a code owner September 2, 2022 14:13
@rubenfonseca rubenfonseca requested review from am29d and removed request for a team September 2, 2022 14:13
assert "MonsterCookie" in response.cookies.keys()
assert "CookieMonster" in response.cookies.keys()
# ALB sorts the header values randomly, so we have to re-order them for comparison here
returned_value = ", ".join(sorted(response.headers[key].split(", ")))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for us: maybe we should have an utility function for this as we'll forget at some point (syntax et al)

@rubenfonseca rubenfonseca changed the title feat(event_handler): add cookies as 1st class citizen V2 feat(event_handler): add cookies as 1st class citizen Sep 2, 2022
@rubenfonseca rubenfonseca changed the title V2 feat(event_handler): add cookies as 1st class citizen feat(event_handler): add cookies as 1st class citizen in v2 Sep 2, 2022
@rubenfonseca rubenfonseca merged commit 64513d9 into aws-powertools:v2 Sep 2, 2022
@rubenfonseca rubenfonseca deleted the feat/cookies-v2 branch September 2, 2022 15:21
github-actions bot pushed a commit that referenced this pull request Sep 2, 2022
heitorlessa added a commit that referenced this pull request Sep 9, 2022
* develop:
  chore(deps-dev): bump aws-cdk-lib from 2.40.0 to 2.41.0 (#1507)
  update changelog with latest changes
  feat(tracer): support methods with the same name (ABCs) by including fully qualified name in v2 (#1486)
  chore(deps-dev): bump aws-cdk-aws-apigatewayv2-integrations-alpha from 2.39.1a0 to 2.40.0a0 (#1496)
  chore(deps-dev): bump mkdocs-material from 8.4.2 to 8.4.3 (#1504)
  chore(deps): bump pydantic from 1.10.1 to 1.10.2 (#1502)
  update changelog with latest changes
  feat(data-classes): add KafkaEvent and KafkaEventRecord (#1485)
  chore(deps-dev): bump pytest from 7.1.2 to 7.1.3 (#1497)
  update changelog with latest changes
  feat(event_handler): add cookies as 1st class citizen in v2 (#1487)
  chore(deps-dev): bump black from 22.6.0 to 22.8.0 (#1494)
  chore(deps-dev): bump aws-cdk-lib from 2.39.1 to 2.40.0 (#1495)
  chore(maintenance): add discord link to first PR and first issue (#1493)
  update changelog with latest changes
  refactor(batch): remove legacy sqs_batch_processor (#1492)
  chore(deps): bump pydantic from 1.10.0 to 1.10.1 (#1491)
  chore(deps-dev): bump flake8-variables-names from 0.0.4 to 0.0.5 (#1490)
Tankanow pushed a commit to Tankanow/aws-lambda-powertools-python that referenced this pull request Sep 13, 2022
Tankanow pushed a commit to Tankanow/aws-lambda-powertools-python that referenced this pull request Sep 13, 2022
rubenfonseca added a commit that referenced this pull request Sep 14, 2022
rubenfonseca added a commit that referenced this pull request Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature New feature or functionality size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants