Use URL type in most outstanding struct fields #1468
Conversation
This fixes all known remaining cases where url fields are stored as plain strings, with the exception of form fields where empty strings are used as sentinels (see `diesel_option_overwrite_to_url`). Tested for regressions in the federated docker setup attempting to exercise all changed fields, including through apub federation. Fixes LemmyNet#1385
This also then fixes LemmyNet#602
crates/apub/src/http/mod.rs
Outdated
| @@ -46,12 +46,14 @@ pub async fn get_activity( | |||
| context: web::Data<LemmyContext>, | |||
| ) -> Result<HttpResponse<Body>, LemmyError> { | |||
| let settings = Settings::get(); | |||
| let activity_id = format!( | |||
| let activity_id = url::Url::parse(&format!( | |||
There was a problem hiding this comment.
Very minor, but please import the type instead of doing this.
crates/apub/src/http/mod.rs
Outdated
| "{}/activities/{}/{}", | ||
| settings.get_protocol_and_hostname(), | ||
| info.type_, | ||
| info.id | ||
| ); | ||
| )) | ||
| .unwrap() |
crates/apub/src/objects/user.rs
Outdated
| avatar, | ||
| banner, | ||
| avatar: avatar.map(|outer| outer.map(|inner| inner.into())), | ||
| banner: banner.map(|outer| outer.map(|inner| inner.into())), |
There was a problem hiding this comment.
This can probably be simplified, at least use shorter variable names.
crates/db_queries/src/lib.rs
Outdated
| // An empty string is an erase | ||
| Some(unwrapped) => { | ||
| if !unwrapped.eq("") { | ||
| Some(Some(url::Url::parse(unwrapped)?.into())) |
There was a problem hiding this comment.
Import Url.
Edit: Ah its probably because of lemmy_db_schema::Url, thats annoying. I guess we should rename our Url type, but dunno whats a good alternative name. LemmyUrl?
There was a problem hiding this comment.
yeah, that's the reason, I guess this newtype is needed to workaround the orphan rule? maybe DbUrl would be most self-explanatory?
There was a problem hiding this comment.
That sounds good. You can change it as part of this PR if you dont mind.
|
CI didnt run, it seems like it was misconfigured. I changed the settings and hopefully it should run for your next push. Thanks for the PR! |
|
Don't worry about the conflicts, we'll fix them once the above changes are made ( and the CI passes ) |
- Fixed some unwraps and err message formatting - Bumped the `url` library to 2.2.1 to fix a bug with serde error messages - Add unit tests for the two diesel option override functions - Fix migration teardown by adding a no-op
|
i went ahead and merged since the build was unhappy with the conflicts, it seems to compile at least |
|
hmm clippy is blocking on some warnings unrelated to this change. I was hitting this locally too so I've been committing with |
|
Looks good, thank you :) |
…n-pwd-change * origin/main: revert Compose file version from 3.3 to 2.2 Adding more mem limits bump memory limit of iframely Remove extra category_id s . Fixes LemmyNet#1429 Fixing wrong user_ and community icon and banner urls. Remove category from activitypub context Adding a password length check to other API actions. (LemmyNet#1474) Update test script Use URL type in most outstanding struct fields (LemmyNet#1468) Forbid usage of unwrap Upgrade Rust version Rewrite settings implementation. Fixes LemmyNet#1270 (LemmyNet#1433) Rename `lemmy_structs` to `lemmy_api_structs` # Conflicts: # crates/db_schema/src/source/user.rs
This fixes all known remaining cases where url fields are stored as
plain strings, with the exception of form fields where empty strings
are used as sentinels (see
diesel_option_overwrite_to_url).This also adds a small migration cleaning up bad
post.urldata,converting all instances of empty strings to nulls.
Tested for regressions in the federated docker setup attempting to
exercise all changed fields, including through apub federation.
Fixes #1385 and fixes #602