backend architecture

[markus2330]

We should have full architecture description of how the code in the backend should be structured.

Please use arc42 and a similar style as already started (and also used in Elektra) with Markdown files per chapter.

[markus2330]

@kitzbergerg Did you already read through the whole backend code? Are there obvious improvements?

[kitzbergerg]

I haven't looked into it in detail, but I noticed a few things.
One thing I remember is: We should use From<Error> or .map_err() more often to convert errors and then use ?. There are quite a few long and somewhat unreadable match statements that could be removed with changes like that.

How do we want to handle that? Can I just start and change the code how I think it would make sense and book it to either this ticket or #60. While doing so I would also create a style guide. Although I'll try to mostly use clippy lints.

For a higher level overview we could use https://github.com/rust-lang/mdBook and for code documentation cargo doc. Both generate webpages that look in my opinion quite nice (mdBook is used for the rust book and cargo doc is what is used in crates.io)

[markus2330]

"Obvious" improvements (like making code shorter/more readable/...) you can directly do (simply create PR(s)). For bigger restructuring it is needed to first create architecture/docu/decisions so that I can give my okay first.

For a higher level overview we could use https://github.com/rust-lang/mdBook and for code documentation cargo doc.

Sounds good to me! 🚀

[kitzbergerg]

@markus2330 Is there a reason why we use routes instead of macros (see https://actix.rs/docs/getting-started). Otherwise I might refactor that as well in a sensible way.

[markus2330]

I don't think there is a special reason for this. (@buenaflor?)

Feel free to refactor if it leads to easier comprehensible code.

[kitzbergerg]

Do we want path normalization? (See https://actix.rs/docs/url-dispatch#path-normalization-and-redirecting-to-slash-appended-routes)

Currently there we get 404 if we call e.g. http://localhost:8080/api/seeds/.
With normalization both http://localhost:8080/api/seeds and http://localhost:8080/api/seeds/ would match (as well as e.g. http://localhost:8080/api//seeds/)

[markus2330]

@kitzbergerg If it doesn't cost performance or doesn't require much additional code it is handy for manual testing (when using curl etc.).

[kitzbergerg]

Another thing I noticed is that diesel is not async. So every call to the database blocks a thread in actix's/tokio's executor. Unless there is a good reason to use diesel in combination with actix we might want to consider switching to an async sql/postgres crate. I have heard some good things about sqlx.

This however need further investigation. Unless PermaplanT should really be able to scale with lots of concurrent connections this probably doesn't make a difference.

[kitzbergerg]

Just found this: https://github.com/actix/examples/blob/f8e3570bd16bcf816070af20f210ce1b4f2fca69/diesel/src/main.rs#L64-L70. Maybe I can build something out of that (although that seems a lot more complicated than just using a different crate).

There is also a lot of discussion here about async diesel: diesel-rs/diesel#399.
Another option would be to switch to diesel_async (see here for doc).

[kitzbergerg]

Another thing I noticed is that diesel is not async. So every call to the database blocks a thread in actix's/tokio's executor. Unless there is a good reason to use diesel in combination with actix we might want to consider switching to an async sql/postgres crate. I have heard some good things about sqlx.

This however need further investigation. Unless PermaplanT should really be able to scale with lots of concurrent connections this probably doesn't make a difference.

According to actix doc we should use web::block: https://actix.rs/docs/databases

[markus2330]

Thank you, these are great investigations!

This however need further investigation. Unless PermaplanT should really be able to scale with lots of concurrent connections this probably doesn't make a difference.

If PermaplanT gets successful, we hopefully have to deal with many concurrent connections. Uptime is most important (no crashes, no restarts etc.), good performance is 2nd most important non-functional goal (after UI related goals in README.md). These non-functional goals specific to the backend should be also part of the mdBook.

I have heard some good things about sqlx.

Can you please write a decision for the compile-time checked queries in doc/decisions. Just that we have documented pros/cons of Diesel and alternatives. Changing to sqlx is not totally off the table if it really is strongly superior. The main questions are if it also supports migrations and postgis datatypes like Diesel does (https://lib.rs/crates/postgis_diesel).

https://github.com/actix/examples/blob/f8e3570bd16bcf816070af20f210ce1b4f2fca69/diesel/src/main.rs#L64-L70.
According to actix doc we should use web::block: https://actix.rs/docs/databases

These two ways should also be listed as possibilities in the decision.

[kitzbergerg]

@markus2330 I think we can close this in favour of #158.
I'll create a new issue for the ORM decision.