feat: add form support and better routing; implement TODO app example #7

m4tx · 2024-08-13T22:37:14Z

Quite a big PR, but at this stage of the project it's difficult to make any significant changes while also separating it into logical blocks.

This adds:

HTTP GET and POST Form support with easy-to-use derive macro
Much better routing, with support for path params, and ability to get a URL for given view name
Simple support for the Askama template engine
A simple TODO app example showcasing the features listed above

m4tx · 2024-08-13T22:40:36Z

examples/todo-list/src/main.rs

+
+async fn remove_todo(request: Request) -> Result<Response, Error> {
+    let todo_id = request.path_param("todo_id").expect("todo_id not found");
+    let todo_id = todo_id.parse::<usize>().expect("todo_id is not a number");


It would probably be nice to have an ergonomic API to get path parameters with type safety (similar to Form I guess), but this is definitely out of scope of this PR.

I think that the fn(request) -> response) was inspired by Django's approach, but I think I'd rather go with Rocket's /Axum's approach of variables as function arguments, I think those seem more convenient to use.

Actually, to be clear and for comparison purposes:

Django uses function parameters to pass URL params (docs)

Rocket uses function parameters to pass URL params (docs)

Axum seems to be using a special tuple type that is passed as a parameter to the function (docs)

So, seems like there are two main approaches, and I'm not quite sure which one would be better for us. I've added a task to the project for now, as this is out of scope of this PR, as I've mentioned: https://github.com/orgs/flareon-rs/projects/2/views/7?pane=issue&itemId=75286758

m4tx · 2024-08-13T22:45:52Z

flareon/src/forms.rs

+    }
+}
+
+impl FormField for CharField {


This implementation will probably be moved into some macro in the next PRs as it's pretty generic, but an in-place implementation is fine as long as we only have one field type (which is the case for now).

codecov · 2024-08-13T22:52:20Z

Codecov Report

Attention: Patch coverage is 55.94059% with 356 lines in your changes missing coverage. Please review.

Files	Patch %	Lines
flareon/src/forms.rs	0.00%	102 Missing ⚠️
flareon/src/router.rs	0.00%	89 Missing ⚠️
examples/todo-list/src/main.rs	0.00%	57 Missing ⚠️
flareon/src/request.rs	0.00%	56 Missing ⚠️
flareon/src/lib.rs	0.00%	23 Missing ⚠️
flareon/src/router/path.rs	90.47%	20 Missing ⚠️
flareon-macros/src/form.rs	98.40%	4 Missing ⚠️
flareon/src/error.rs	0.00%	4 Missing ⚠️
examples/hello-world/src/main.rs	0.00%	1 Missing ⚠️

Flag	Coverage Δ
rust	`50.80% <55.94%> (+39.00%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
flareon-macros/src/lib.rs	`100.00% <100.00%> (+100.00%)`	⬆️
flareon-macros/tests/compile_tests.rs	`100.00% <100.00%> (ø)`
examples/hello-world/src/main.rs	`0.00% <0.00%> (ø)`
flareon-macros/src/form.rs	`98.40% <98.40%> (ø)`
flareon/src/error.rs	`0.00% <0.00%> (ø)`
flareon/src/router/path.rs	`90.47% <90.47%> (ø)`
flareon/src/lib.rs	`0.00% <0.00%> (ø)`
flareon/src/request.rs	`0.00% <0.00%> (ø)`
examples/todo-list/src/main.rs	`0.00% <0.00%> (ø)`
flareon/src/router.rs	`0.00% <0.00%> (ø)`
... and 1 more

seqre

LGTM for now, few comments in the code

seqre · 2024-08-18T04:30:24Z

flareon/src/lib.rs

    let listener = tokio::net::TcpListener::bind(address_str).await.unwrap();

    let handler = |request: axum::extract::Request| async move {
-        pass_to_axum(&project, request)
+        pass_to_axum(&project, Request::new(request, project.clone()))


I don't think cloning project on each request is the way to go

This is not cloning FlareonProject, but rather Arc<FlareonProject>, which is much cheaper: https://github.com/flareon-rs/flareon/pull/7/files/eadabfac46a1702bb174fc0ef5809a49951577b7#diff-da48df69bb298faf68bd58ae6ad3a1e4df72c8d7733bd5849b62d595d9747a80R211

seqre · 2024-08-18T04:35:55Z

flareon/src/lib.rs

 pub mod prelude;
+#[doc(hidden)]
+pub mod private;


If private shouldn't that be pub(crate)?

The answer is inside the module documentation (somewhat, maybe it needs clarification):

/// Re-exports of some of the Flareon dependencies that are used in the macros. /// /// This is to avoid the need to add them as dependencies to the crate that uses /// the macros.

The problem is that when you generate some code inside a proc macro, the code end up being "pasted" inside the consumer's crate, not our crate. In order to use things like async_trait there, either the user has to declare it as their dependency, or we can just create this sort of "public-hidden" module that re-exports stuff that the generated code is using.

By the way - this seems to be a common practice in many libraries, e.g. https://github.com/serde-rs/serde/blob/master/serde/src/lib.rs#L313-L316

seqre · 2024-08-18T04:46:33Z

flareon/src/request.rs

+        self.inner.headers().get(axum::http::header::CONTENT_TYPE)
+    }
+
+    pub async fn form_data(&mut self) -> Result<Bytes, Error> {


I think the name is confusing, wouldn't data just be enough?

Perhaps. But then won't it be confusing when we want to have a method that returns a raw request body? data() sounds very much like it could be just returning raw data, while this method specifically parses the URL/request body as a HTML form, hence the name.

seqre · 2024-08-18T21:56:57Z

examples/todo-list/templates/index.html

+</head>
+<body>
+<h1>TODO List</h1>
+<form id="todo-form" action="{{ flareon::reverse_str!(request, "add-todo") }}" method="post">


Nit, but I think that flareon filters/actions should be in scope by default

Agreed, but I think sadly this will require forking askama library, or at least creating some wrapper proc macro. Forking askama to make it more convenient to use with flareon is on our roadmap, and I think it's out of scope for this PR, so I'd keep it as is for now.

seqre · 2024-08-18T21:58:12Z

examples/todo-list/src/main.rs

+            Route::with_handler_and_name(
+                "/todos/:todo_id/remove",
+                Arc::new(Box::new(remove_todo)),
+                "remove-todo",
+            ),


I assume that /remove is only because we don't have support for DELETE yet, right?

Typically in web development, unless developing a REST service (or some other service that's not directly web-browser-facing), you only ever use GET and POST, really. I think one of the reasons is that it's defined as idempotent, and in case of this TODO app, it's clearly not (successive requests to /todos/0/delete will eventually remove all of the TODOs). There seem to be some very good historical reasons as well, but I'm not sure how important these are today.

seqre · 2024-08-19T03:56:58Z

examples/todo-list/src/main.rs

+
+async fn remove_todo(request: Request) -> Result<Response, Error> {
+    let todo_id = request.path_param("todo_id").expect("todo_id not found");
+    let todo_id = todo_id.parse::<usize>().expect("todo_id is not a number");


I think that the fn(request) -> response) was inspired by Django's approach, but I think I'd rather go with Rocket's /Axum's approach of variables as function arguments, I think those seem more convenient to use.

m4tx requested review from seqre and Iipin August 13, 2024 22:37

m4tx commented Aug 13, 2024

View reviewed changes

m4tx force-pushed the todo-app branch from 4ff0843 to eb37ec4 Compare August 13, 2024 22:43

m4tx commented Aug 13, 2024

View reviewed changes

m4tx force-pushed the todo-app branch 2 times, most recently from f3c13a3 to 5ed7b02 Compare August 13, 2024 23:16

feat: add form support and better routing; implement TODO app example

eadabfa

m4tx force-pushed the todo-app branch from 5ed7b02 to eadabfa Compare August 14, 2024 08:27

seqre reviewed Aug 19, 2024

View reviewed changes

m4tx merged commit 7867952 into master Aug 19, 2024
18 checks passed

m4tx deleted the todo-app branch August 19, 2024 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add form support and better routing; implement TODO app example #7

feat: add form support and better routing; implement TODO app example #7

m4tx commented Aug 13, 2024

m4tx Aug 13, 2024

seqre Aug 19, 2024

m4tx Aug 19, 2024

m4tx Aug 13, 2024

codecov bot commented Aug 13, 2024 •

edited

Loading

seqre left a comment

seqre Aug 18, 2024

m4tx Aug 19, 2024

seqre Aug 18, 2024

m4tx Aug 19, 2024 •

edited

Loading

m4tx Aug 19, 2024

seqre Aug 18, 2024

m4tx Aug 19, 2024

seqre Aug 18, 2024

m4tx Aug 19, 2024

seqre Aug 18, 2024

m4tx Aug 19, 2024 •

edited

Loading

seqre Aug 19, 2024

feat: add form support and better routing; implement TODO app example #7

feat: add form support and better routing; implement TODO app example #7

Conversation

m4tx commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 13, 2024 • edited Loading

Codecov Report

seqre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4tx Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4tx Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 13, 2024 •

edited

Loading

m4tx Aug 19, 2024 •

edited

Loading

m4tx Aug 19, 2024 •

edited

Loading