feat: add embeds support - #41
Conversation
|
Resolves #40 |
|
Nice work! I'll try to get around to testing and reviewing this soon. Just know that I might be a little slow to do so because of the holidays. |
|
Take your time, we all have a life beside doing stuff on GitHub ^^ |
liamw1
left a comment
There was a problem hiding this comment.
Thanks for working on this! A few general notes:
-
It looks like the submitted code hasn't been formatted, so be sure to run
cargo fmtbefore committing. -
I made some comments relaying clippy warning, as I like to keep the codebase free of clippy warnings. These don't appear when compiling the code normally, so just do a quick
cargo clippywhen finishing up and try to address any feedback it gives. Occasionally there will be specific warnings that are obnoxious, unhelpful, or require contorting the code to silence. In those cases, we can discuss adding them to the list of allowed warnings defined at the top ofmain.rs.
As a heads up, I've added a CI job that automatically runs clippy on PRs, but I don't expect it to pass in this case. That's because there were some style warnings in tests that weren't addressed onmasterthat have recently been fixed. These warnings only appear when runningcargo clippy --all-targets, so just address the ones that are fromcargo clippy.
- Most of the remaining work here is figuring out how to remove these
unwrapcalls. These are great for prototyping, but as you probably know they aren't ideal for production code. The server can technically handle panics just fine, but they produce huge stack traces instead of nice errors with descriptions. Sometimes removing anunwrapcan be pretty tricky, but most can be removed with one of a few simple strategies:
a) If theNoneorErrorvariant is impossible in bug-free code, then it can be replaced withexpect("msg")
b) If a reasonable default can be used in place of aNoneorError, it can be replaced withunwrap_or(default)
c) Otherwise, it's best to propagate the error with the?operator
| width: config::get().thumbnails.post_width, | ||
| height: config::get().thumbnails.post_height, | ||
| } | ||
| } |
There was a problem hiding this comment.
Since we only use id and thumbnail_url from post_info, querying for PostInfo is a bit overkill. We can actually get away with just the post_id because we can create a PostHash object from an ID, which has a thumbnail_url method. So maybe the role of get_post_info can be changed to just check if the post exists in the database.
This approach also has the benefit of not having to call unwrap() on post_info fields.
There was a problem hiding this comment.
The oembed spec requires a width and height (section 2.3.4.1). So while it pains my heart to waste cpu cycles on querying for a PostInfo object, it's probably the best solution for now.
There was a problem hiding this comment.
Nevermind, forget what I said. I haven't touched the code for a bit and got confused. I will finish everything after I'm back from the office today.
| // todo | ||
| author_name: None, | ||
| provider_name: config::get().public_info.name.to_string(), | ||
| provider_url: config::get().domain.as_deref().unwrap().to_string(), |
There was a problem hiding this comment.
The default config file doesn't have a domain field, so this unwrap will panic. Consider using unwrap_or with some reasonable default instead. Better yet, you could factor out
oxibooru/server/src/api/password_reset.rs
Lines 44 to 55 in 000a035
|
Holy macaroni, thank you ever so much for taking your time to write comments and suggestions for all the warnings. I'll do some research on best practices and will then push the fixes. |
|
No problem! I found the Rust book to be a great resource when learning Rust. No need to read through all of it, as there are many concepts that transfer from other languages. I found the section on error handling to be very helpful, especially coming from languages that primarily handle errors via exceptions. |
Some of this code is probably quite badly written.
This is pretty much only a proof of concept. The following parts might need to be changed:
The AppState Struct and the way it is shared with every route (I had no idea how to share the content of the index.htm file with the /index/post/{post_id} route. The docs recommend using an AppState)
There's lots of beautiful (and totally unsafe) unwrap() calls.
The code needs to check the following things
3.1) if the index.htm file exists
3.2) If the user wants embeds enabled (this should probably be a config variable?)
3.3) if anonymous users have the permission to see posts (this potentially allows leaking of thumbnails otherwise)
The post author currently isn't being displayed
The PostInfo Struct probably shouldn't have public properties?
I used the following docker-compose.yml
This is my nginx.conf:
These are the only values I changed inside the config.toml