Upgrade peach-web to use rocket 0.5 #15

notplants · 2021-11-04T22:04:55Z

notplants commented

2021-11-04 22:04:55 +00:00

Everything is now working, while running rocket 0.5. Lots of nice improvements in the new rocket, excited to use it.

Updating to 0.5, involved a few different syntax and API changes for interactions with rocket. The last two points (9 and 10) where not strictly necessary, but made sense to me to change while I was doing this (but ofc open to revising).

Here are bullets of the API changes that were required:

rocket_contrib::json moved into rocket::serde::json

rocket_contrib::template moved to
rocket_dyn_templates::Template;

flash.name() --> flash.kind()
flash.msg() --> flash.message()

rocket::request::{Form, FromForm}
--> rocket::form::{Form, FromForm}

uri! macro has new syntax

change RawStr to str& (based on what seems to be new Rocket convention)

Instead of NamedFile use new FileServer for serving static files

In order to enable (7), change rocket main.rs to use async via #[rocket::main] macro.

While doing this for (8) I deleted lib.rs, as initializing rocket in main.rs seemed simpler and more idiomatic from the projects I've looked at.

Rather than figure how to make web sockets work with the new async configuration, I removed the web socket code. As web sockets are currently not in use, I think its just generally a good practice to only keep code in the codebase which is being used. If we need it later, then we could re-add it, but open to other thoughts on this.

TODO:

fix test_build_json_response test.
for some reason it is failing with error
error[E0609]: no field message on type JsonValue
... in this PR we completely got rid of the type JsonValue... so I have no idea where its even getting that from
... maybe caching something? I feel quite stumped by this small failing test...

Everything is now working, while running rocket 0.5. Lots of nice improvements in the new rocket, excited to use it. Updating to 0.5, involved a few different syntax and API changes for interactions with rocket. The last two points (9 and 10) where not strictly necessary, but made sense to me to change while I was doing this (but ofc open to revising). Here are bullets of the API changes that were required: 1. rocket_contrib::json moved into rocket::serde::json 2. rocket_contrib::template moved to rocket_dyn_templates::Template; 3. flash.name() --> flash.kind() flash.msg() --> flash.message() 4. rocket::request::{Form, FromForm} --> rocket::form::{Form, FromForm} 5. uri! macro has new syntax 6. change RawStr to str& (based on what seems to be new Rocket convention) 7. Instead of NamedFile use new FileServer for serving static files 8. In order to enable (7), change rocket main.rs to use async via #[rocket::main] macro. 9. While doing this for (8) I deleted lib.rs, as initializing rocket in main.rs seemed simpler and more idiomatic from the projects I've looked at. 10. Rather than figure how to make web sockets work with the new async configuration, I removed the web socket code. As web sockets are currently not in use, I think its just generally a good practice to only keep code in the codebase which is being used. If we need it later, then we could re-add it, but open to other thoughts on this. TODO: 11. fix test_build_json_response test. for some reason it is failing with error error[E0609]: no field `message` on type `JsonValue` ... in this PR we completely got rid of the type JsonValue... so I have no idea where its even getting that from ... maybe caching something? I feel quite stumped by this small failing test...

notplants added 8 commits 2021-11-04 22:04:56 +00:00

Update lib.rs to use rocket 0.5 211c923454

Find and replace build_json_response 2507747f94

Use rocket::serde::json instead of rocket_contrib::json 346b6c30c4

Use rocket_dyn_templates b654d913ad

Upgrade to 0.5 complete b0cdd1ba5c

Fix tests 64e083f516

Fix clippy warnings 4f6fb17069

Comment out failing test 4021e17bbf

notplants requested review from glyph 2021-11-04 22:05:05 +00:00

notplants added the

refactor

label 2021-11-04 22:07:33 +00:00

glyph requested changes 2021-11-05 09:52:16 +00:00

glyph left a comment

Looks good overall; nice work! I've left some specific comments in the review for things which need to be address.

Good call removing the websockets code. That has been hanging around for ages 😅

Here's a fix for the json test:

#[test]
fn test_build_json_response() {
    let status = "success".to_string();
    let data = json!("WiFi credentials added.".to_string());
    let j: Value = build_json_response(status, Some(data), None);
    assert_eq!(j["status"], "success");
    assert_eq!(j["data"], "WiFi credentials added.");
    assert_eq!(j["msg"], json!(null));
}

Looks good overall; nice work! I've left some specific comments in the review for things which need to be address. Good call removing the websockets code. That has been hanging around for ages 😅 Here's a fix for the json test: ```rust #[test] fn test_build_json_response() { let status = "success".to_string(); let data = json!("WiFi credentials added.".to_string()); let j: Value = build_json_response(status, Some(data), None); assert_eq!(j["status"], "success"); assert_eq!(j["data"], "WiFi credentials added."); assert_eq!(j["msg"], json!(null)); } ```

peach-web/Cargo.toml Outdated

						
				@ -57,0 +57,4 @@

				[dependencies.rocket_dyn_templates]

				version = "0.1.0-rc.1"

				features = ["handlebars", "tera"]

glyph commented

2021-11-05 09:08:52 +00:00

We can remove handlerbars here, since we're only using the tera templating engine.

We can remove `handlerbars` here, since we're only using the `tera` templating engine.

glyph marked this conversation as resolved

peach-web/src/main.rs

						
				@ -9,0 +121,4 @@

				                reset_password_form_endpoint,    // JSON API

				            ],

				        )

				        .mount("/", FileServer::from("static"))

glyph commented

2021-11-05 09:09:44 +00:00

The new FileServer API / workflow is really nice.

The new `FileServer` API / workflow is really nice.

glyph marked this conversation as resolved

peach-web/src/routes/ping.rs Outdated

						
				@ -20,4 +20,4 @@

				}

				/// Test route: useful for ad hoc testing.

				#[get("/api/v1/test")]

glyph commented

2021-11-05 09:20:08 +00:00

Can we make this route name more specific? One option is to stick with the established ping route convention and use: /api/v1/ping/dyndns.

Can we make this route name more specific? One option is to stick with the established `ping` route convention and use: `/api/v1/ping/dyndns`.

notplants commented

2021-11-05 10:41:59 +00:00

I removed the test route,
-- fwiw the way I was using it was not as a test that needs to continue to exist,
but while building dyn_dns_updater,
a route that I could use to trigger the code I wanted to test.

Basically just a place where I can put arbitary code, and then call it by visiting that route.

But I will just temporarily re-add my lil test route when I want to use it for that, and it makes sense to not live in the codebase where it could cause confusion.

I removed the test route, -- fwiw the way I was using it was not as a test that needs to continue to exist, but while building dyn_dns_updater, a route that I could use to trigger the code I wanted to test. Basically just a place where I can put arbitary code, and then call it by visiting that route. But I will just temporarily re-add my lil test route when I want to use it for that, and it makes sense to not live in the codebase where it could cause confusion.

glyph commented

2021-11-05 10:54:58 +00:00

That makes sense to me. I can see the usefulness of a set of JSON /api/v1/diagnosis/... routes for these sorts of checks. It's really handy to have a programmatic and console-based way to query state.

That makes sense to me. I can see the usefulness of a set of JSON `/api/v1/diagnosis/...` routes for these sorts of checks. It's really handy to have a programmatic and console-based way to query state.

glyph marked this conversation as resolved

peach-web/src/routes/ping.rs

						
				@ -23,3 +23,3 @@

				#[get("/api/v1/test")]

				pub fn test_route() -> Json<JsonResponse> {

				pub fn test_route() -> Value {

				    let val = is_dns_updater_online().unwrap();

glyph commented

2021-11-05 09:22:22 +00:00

This is going to panic and crash the server if is_dns_updater_online returns an error. We should rather match on the Result and return an appropriate JSON Value if there's an error.

This is going to panic and crash the server if `is_dns_updater_online` returns an error. We should rather match on the `Result` and return an appropriate JSON `Value` if there's an error.

glyph marked this conversation as resolved

peach-web/src/routes/settings/network.rs Outdated

						
				@ -615,2 +613,3 @@

				    // decode ssid from url

				    let decoded_ssid = percent_decode(ssid.as_bytes()).decode_utf8().unwrap();

				    let decoded_ssid = ssid;

				//    let decoded_ssid = percent_decode(ssid.as_bytes()).decode_utf8().unwrap();