Add a public-aware deserializer for recipient URLs#165
Add a public-aware deserializer for recipient URLs#165Nutomic merged 2 commits intoLemmyNet:mainfrom
Conversation
| Value::String(value) => Url::parse(&value).map_err(D::Error::custom), | ||
| value => Url::deserialize(value).map_err(D::Error::custom), | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
There is no need for a new method here. Instead put this code inside the existing deserialize_one_or_many method. That way nothing needs to be updated in Lemmy (except updating the library version).
There was a problem hiding this comment.
I considered putting this into deserialize_one_or_many(), but kept it as a separate Url-specific helper because deserialize_one_or_many() is generic over T.
If we normalize Public/as:Public inside the generic helper by pre-processing serde_json::Value, this can also affect non-URL uses of the helper, for example Vec<String> or Vec<Value> fields where "Public" is meant to remain a plain string.
Would you still prefer changing deserialize_one_or_many() directly despite that broader behavior change? If so, I can rework the PR in that direction and document the behavior.
There was a problem hiding this comment.
Did you see my other comment? That one would be the simplest solution.
There was a problem hiding this comment.
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, as:Public should work because it can be represented as a Url, but bare Public still seems like it would fail while deserializing to/cc into Vec<Url>, before the visibility check runs.
Is that acceptable? In other words, should this only handle URL-shaped public aliases such as as:Public, and leave bare Public unsupported? Or am I missing nother path where bare Public is accepted before verification?
There was a problem hiding this comment.
Ah that is true, didnt think of it. I tried just now to remove the generic T and it works with the Lemmy code. So then you can delete the existing deserialize_one_or_many method, and rename your new method to deserialize_one_or_many.
There was a problem hiding this comment.
Thanks, that makes sense.
I'll rework this PR in that direction: remove the generic T version of deserialize_one_or_many(), rename the new Url-specific implementation to deserialize_one_or_many(), and update the docs/tests accordingly.I’ll rework this PR in that direction: remove the generic T version of deserialize_one_or_many(), rename the new Url-specific implementation to deserialize_one_or_many(), and update the docs/tests accordingly.
After that, LemmyNet/lemmy#6466 should only need to update the dependency and can keep the existing deserialize_one_or_many call sites.
Update deserialize_one_or_many to deserialize recipient URL fields while accepting `Public` and `as:Public` as aliases for the canonical ActivityStreams public URL. Add focused tests for single and array inputs, and verify that unrelated string fields such as `content` are left unchanged. LemmyNet/lemmy#6465
dahlia
force-pushed
the
fix/public-addressing-aliases
branch
from
a2a6083 to
eb45245
Compare
| } | ||
| Value::String(value) => Url::parse(&value).map_err(D::Error::custom), | ||
| value => Url::deserialize(value).map_err(D::Error::custom), | ||
| }) |
There was a problem hiding this comment.
| }) | |
| .unique() | |
| }) |
No need to keep multiple identical values. The unique function is from itertools.
There was a problem hiding this comment.
Good point, thanks. I applied that in 7cfd394, so equivalent Public aliases are now deduplicated after deserialization.
Drop repeated recipient URLs after deserialization so equivalent public aliases such as `Public`, `as:Public`, and the canonical public URL do not produce duplicate entries. Update the helper documentation and tests to match the deduplicated result.
|
Good job, thank you! |
Update Lemmy to activitypub_federation 0.7.0-beta.11, which includes support for `Public` and `as:Public` recipient aliases. No local deserialization changes are needed now that the upstream fix has been merged and published. LemmyNet/activitypub-federation-rust#165 LemmyNet#6465
Update Lemmy to activitypub_federation 0.7.0-beta.11, which includes support for `Public` and `as:Public` recipient aliases. No local deserialization changes are needed now that the upstream fix has been merged and published. LemmyNet/activitypub-federation-rust#165 #6465
2 participants
Related to LemmyNet/lemmy#6465 and LemmyNet/lemmy#6466 (comment).
ActivityPub allows the public collection to be represented as the canonical IRI,
as:Public, orPublic. As described in that issue, this can break incoming recipient parsing if only the canonical URL is accepted.This PR adds a
Url-specific deserialization helper for recipient fields, instead of changing the behavior of the genericdeserialize_one_or_many()helper. The new helper acceptsPublicandas:Publicand normalizes them to the canonical ActivityStreams public URL.It also adds focused unit tests for single-value and array inputs, plus a regression test to make sure unrelated string fields are left unchanged.
Note
This patch was developed with assistance from GPT-5.4. I reviewed the final changes myself, verified the final behavior locally, and ran the relevant tests before opening this PR.