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

Cookieparser attributes #409

Merged
merged 9 commits into from
May 7, 2022
Merged

Conversation

dranikpg
Copy link
Member

Crows cookie_parser currently doesn't support setting any attributes on cookies (most importantly expires and path). I've added support for all available HTTP cookie attributes.

This helps #406

Cookie parsers context allows now to set attributes in a builder style:

auto& cookie_ctx = app.get_context<crow::CookieParser>(req);
cookie_ctx.set_cookie("key", "value")
    .max_age(60)
    .path("/")
    .secure();

I've chosen boost for timepoints and durations as Crow already depends on it and std::time_t/std::tm are just horribly difficult to use.

@The-EDev
Copy link
Member

I haven't looked too deeply into the code but have you considered std::chrono or is it not applicable here?

@dranikpg
Copy link
Member Author

have you considered std::chrono?

I didn't want to resort to 3rd party libraries initially so I looked at all of std (up to c++ 11/14)

chrono

Chrono is more for tracking time than working with dates. Most of its features on the docs were added in c++20 when it was refreshed to support dates properly. Chronos timepoint is the only 11/14 struct to hold a datetime and it doesnt support any operations on it. The duration type is bound to a clock.

c-style

Date operations in chrono are possible with a group of c-style functions and helper structs: std::tm, std::time_t, mktime. Like even creating a date. Those are really inconvenient and hard to use. At least they support put_time and get_time for reading/writing them (and thats all the cookieparser needs).


As a whole, std::tm is convertible from both chrono and boost and is the universal c++ way to store daytime. We could really strip boost out and use only it. It'd be probably the best way as of a second thought. Potential users just should be aware to use boost time for creating/managing dates themselves (or chrono with c++20).

Copy link
Member

@The-EDev The-EDev left a comment

Choose a reason for hiding this comment

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

In addition to the comments, it might be useful to create some documentation on how to use this middleware. (I can do that, you don't have to do it if you don't want to)

Thanks a lot for the work you've done here!

include/crow/middlewares/cookie_parser.h Show resolved Hide resolved
include/crow/middlewares/cookie_parser.h Outdated Show resolved Hide resolved
include/crow/middlewares/cookie_parser.h Outdated Show resolved Hide resolved
include/crow/middlewares/cookie_parser.h Outdated Show resolved Hide resolved
@dranikpg
Copy link
Member Author

dranikpg commented May 2, 2022

I've added an example for the cookie parser, as there previously weren't any.
As for the docs, I'd maybe suggest maintaining one page for all small additional middleware (cors, cookies) instead of having separate tiny pages for them all. This would make it easier to navigate around

Copy link
Member

@The-EDev The-EDev left a comment

Choose a reason for hiding this comment

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

I apologize, I just now got around to looking at the documentation. Aside from the comments, mkdocs.yml needs to be changed as follows for the new guide to be built:

@@ -55,7 +55,7 @@ nav:
             - Writing Tests: guides/testing.md
         - Using Crow:
             - HTTP Authorization: guides/auth.md
-            - CORS Setup: guides/CORS.md
+            - Included Middlewares: guides/included-middleware.md
         - Server setup:
             - Proxies: guides/proxies.md
             - Systemd run on startup: guides/syste.md

Aside from that everything seems to be in order :)

docs/guides/included-middleware.md Show resolved Hide resolved
docs/guides/included-middleware.md Outdated Show resolved Hide resolved
docs/guides/included-middleware.md Outdated Show resolved Hide resolved
Copy link
Member

@The-EDev The-EDev left a comment

Choose a reason for hiding this comment

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

Everything seems to be in order.

Thank you very much for your work!

@The-EDev The-EDev force-pushed the cookieparser-attributes branch from b9b2592 to 84effa5 Compare May 7, 2022 09:09
@The-EDev The-EDev merged commit 59b8f43 into CrowCpp:master May 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants