-
Notifications
You must be signed in to change notification settings - Fork 58
Add a public-aware deserializer for recipient URLs #165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,21 @@ | ||
| //! Serde deserialization functions which help to receive differently shaped data | ||
|
|
||
| use serde::{Deserialize, Deserializer}; | ||
| use activitystreams_kinds::public; | ||
| use serde::{de::Error, Deserialize, Deserializer}; | ||
| use serde_json::Value; | ||
| use url::Url; | ||
|
|
||
| /// Deserialize JSON single value or array into Vec. | ||
| /// Deserialize JSON single value or array into `Vec<Url>`. | ||
| /// | ||
| /// Useful if your application can handle multiple values for a field, but another federated | ||
| /// platform only sends a single one. | ||
| /// | ||
| /// Also accepts common `Public` aliases for recipient fields. Some implementations send `Public` | ||
| /// or `as:Public` instead of the canonical `https://www.w3.org/ns/activitystreams#Public` URL | ||
| /// in fields such as `to` and `cc`. | ||
| /// | ||
| /// ``` | ||
| /// # use activitypub_federation::kinds::public; | ||
| /// # use activitypub_federation::protocol::helpers::deserialize_one_or_many; | ||
| /// # use url::Url; | ||
| /// #[derive(serde::Deserialize)] | ||
|
|
@@ -25,24 +33,38 @@ use serde::{Deserialize, Deserializer}; | |
| /// "https://lemmy.ml/u/bob" | ||
| /// ]}"#)?; | ||
| /// assert_eq!(multiple.to.len(), 2); | ||
| /// Ok::<(), anyhow::Error>(()) | ||
| pub fn deserialize_one_or_many<'de, T, D>(deserializer: D) -> Result<Vec<T>, D::Error> | ||
| /// | ||
| /// let note: Note = serde_json::from_str(r#"{"to": ["Public", "as:Public"]}"#)?; | ||
| /// assert_eq!(note.to, vec![public(), public()]); | ||
| /// # Ok::<(), anyhow::Error>(()) | ||
| /// ``` | ||
| pub fn deserialize_one_or_many<'de, D>(deserializer: D) -> Result<Vec<Url>, D::Error> | ||
| where | ||
| T: Deserialize<'de>, | ||
| D: Deserializer<'de>, | ||
| { | ||
| #[derive(Deserialize)] | ||
| #[serde(untagged)] | ||
| enum OneOrMany<T> { | ||
| One(T), | ||
| Many(Vec<T>), | ||
| enum OneOrMany { | ||
| Many(Vec<Value>), | ||
| One(Value), | ||
| } | ||
|
|
||
| let result: OneOrMany<T> = Deserialize::deserialize(deserializer)?; | ||
| Ok(match result { | ||
| OneOrMany::Many(list) => list, | ||
| let result: OneOrMany = Deserialize::deserialize(deserializer)?; | ||
| let values = match result { | ||
| OneOrMany::One(value) => vec![value], | ||
| }) | ||
| OneOrMany::Many(values) => values, | ||
| }; | ||
|
|
||
| values | ||
| .into_iter() | ||
| .map(|value| match value { | ||
| Value::String(value) if matches!(value.as_str(), "Public" | "as:Public") => { | ||
| Ok(public()) | ||
| } | ||
| Value::String(value) => Url::parse(&value).map_err(D::Error::custom), | ||
| value => Url::deserialize(value).map_err(D::Error::custom), | ||
| }) | ||
| .collect() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no need for a new method here. Instead put this code inside the existing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered putting this into If we normalize Would you still prefer changing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you see my other comment? That one would be the simplest solution.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I see how that would make the Lemmy-side change much smaller. One thing I want to confirm before changing direction: if we only extend the visibility check, Is that acceptable? In other words, should this only handle URL-shaped public aliases such as
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah that is true, didnt think of it. I tried just now to remove the generic
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, that makes sense. I'll rework this PR in that direction: remove the generic After that, LemmyNet/lemmy#6466 should only need to update the dependency and can keep the existing |
||
| } | ||
|
|
||
| /// Deserialize JSON single value or single element array into single value. | ||
|
|
@@ -140,6 +162,11 @@ where | |
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::deserialize_one_or_many; | ||
| use activitystreams_kinds::public; | ||
| use anyhow::Result; | ||
| use serde::Deserialize; | ||
|
|
||
| #[test] | ||
| fn deserialize_one_multiple_values() { | ||
| use crate::protocol::helpers::deserialize_one; | ||
|
|
@@ -155,4 +182,75 @@ mod tests { | |
| ); | ||
| assert!(note.is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn deserialize_one_or_many_single_public_aliases() -> Result<()> { | ||
| use url::Url; | ||
|
|
||
| #[derive(Deserialize)] | ||
| struct Note { | ||
| #[serde(deserialize_with = "deserialize_one_or_many")] | ||
| to: Vec<Url>, | ||
| } | ||
|
|
||
| for alias in ["Public", "as:Public"] { | ||
| let note = serde_json::from_str::<Note>(&format!(r#"{{"to": "{alias}"}}"#))?; | ||
| assert_eq!(note.to, vec![public()]); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn deserialize_one_or_many_array() -> Result<()> { | ||
| use url::Url; | ||
|
|
||
| #[derive(Deserialize)] | ||
| struct Note { | ||
| #[serde(deserialize_with = "deserialize_one_or_many")] | ||
| to: Vec<Url>, | ||
| } | ||
|
|
||
| let note = serde_json::from_str::<Note>( | ||
| r#"{ | ||
| "to": [ | ||
| "https://example.com/c/main", | ||
| "Public", | ||
| "as:Public", | ||
| "https://www.w3.org/ns/activitystreams#Public" | ||
| ] | ||
| }"#, | ||
| )?; | ||
|
|
||
| assert_eq!( | ||
| note.to, | ||
| vec![ | ||
| Url::parse("https://example.com/c/main")?, | ||
| public(), | ||
| public(), | ||
| public(), | ||
| ] | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn deserialize_one_or_many_leaves_other_strings_unchanged() -> Result<()> { | ||
| use url::Url; | ||
|
|
||
| #[derive(Deserialize)] | ||
| struct Note { | ||
| #[serde(deserialize_with = "deserialize_one_or_many")] | ||
| to: Vec<Url>, | ||
| content: String, | ||
| } | ||
|
|
||
| let note = serde_json::from_str::<Note>(r#"{"to": "Public", "content": "Public"}"#)?; | ||
|
|
||
| assert_eq!(note.to, vec![public()]); | ||
| assert_eq!(note.content, "Public"); | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to keep multiple identical values. The unique function is from
itertools.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks. I applied that in 7cfd394, so equivalent
Publicaliases are now deduplicated after deserialization.